High firefox Logic Error 🔧 Commit mapped

Overview

High
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impacthigh
DescriptionMozilla Firefox's update mechanism allowed a medium-integrity user process to interfere with the SYSTEM-level updater by manipulating the file-locking behavior. By injecting code into the user-privileged process, an attacker could bypass intended access controls, allowing SYSTEM-level file operations on paths controlled by a non-privileged user and enabling privilege escalation.
ComponentToolkit
Bug ClassLogic Error
Tracker1917536
Fix commit72e0b1b4f11b (firefox) +102/-82
CISA KEVNot listed
CreditedDong-uk Kim (@justlikebono)
Disclosed2025-04-29

Changed Functions

FunctionChangeNotes
if
toolkit/mozapps/update/updater/updater.cpp
modified

Files Changed

  • toolkit/mozapps/update/updater/updater-common.build
  • toolkit/mozapps/update/updater/updater.cpp
  • toolkit/xre/WinTokenUtils.cpp
  • toolkit/xre/WinTokenUtils.h
diff --git a/toolkit/mozapps/update/updater/updater-common.build b/toolkit/mozapps/update/updater/updater-common.build
index 6c6d0adf6f5..600c03de7cd 100644
--- a/toolkit/mozapps/update/updater/updater-common.build
+++ b/toolkit/mozapps/update/updater/updater-common.build
@@ -36,6 +36,7 @@ if CONFIG["MOZ_VERIFY_MAR_SIGNATURE"]:
 if CONFIG["OS_ARCH"] == "WINNT":
     have_progressui = 1
     srcs += [
+        "/toolkit/xre/WinTokenUtils.cpp",
         "loaddlls.cpp",
         "progressui_win.cpp",
     ]
diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
index 89fa8387176..f490a8a4524 100644
--- a/toolkit/mozapps/update/updater/updater.cpp
+++ b/toolkit/mozapps/update/updater/updater.cpp
@@ -55,6 +55,7 @@
 #ifdef XP_WIN
 #  include "mozilla/Maybe.h"
 #  include "mozilla/WinHeaderOnlyUtils.h"
+#  include "mozilla/WinTokenUtils.h"
 #  include <climits>
 #endif  // XP_WIN
 
@@ -130,14 +131,14 @@ BOOL PathGetSiblingFilePath(LPWSTR destinationBuffer, LPCWSTR siblingFilePath,
 // Closes the handle if valid and if the updater is elevated returns with the
 // return code specified. This prevents multiple launches of the callback
 // application by preventing the elevated process from launching the callback.
-#  define EXIT_WHEN_ELEVATED(path, handle, retCode) \
-    {                                               \
-      if (handle != INVALID_HANDLE_VALUE) {         \
-        CloseHandle(handle);                        \
-      }                                             \
-      if (NS_tremove(path) && errno != ENOENT) {    \
-        return retCode;                             \
-      }                                             \
+#  define EXIT_WHEN_ELEVATED(handle, retCode) \
+    {                                         \
+      if (handle != INVALID_HANDLE_VALUE) {   \
+        CloseHandle(handle);                  \
+      }                                       \
+      if (gIsElevated) {                      \
+        return retCode;                       \
+      }                                       \
     }
 #endif
 
@@ -2881,7 +2882,6 @@ int LaunchCallbackAndPostProcessApps(int argc, NS_tchar** argv,
                                      int callbackIndex
 #ifdef XP_WIN
                                      ,
-                                     const WCHAR* elevatedLockFilePath,
                                      HANDLE updateLockFileHandle
 #elif XP_MACOSX
                                      ,
@@ -2940,7 +2940,7 @@ int LaunchCallbackAndPostProcessApps(int argc, NS_tchar** argv,
       LOG(("Not launching Windows post update process because !gSucceeded"));
     }
 
-    EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 0);
+    EXIT_WHEN_ELEVATED(updateLockFileHandle, 0);
 #elif XP_MACOSX
     if (!gIsElevated) {
       if (gSucceeded) {
@@ -3123,18 +3123,31 @@ int NS_main(int argc, NS_tchar** argv) {
   gPatchDirPath[MAXPATHLEN - 1] = NS_T('\0');
 
 #ifdef XP_WIN
-  NS_tchar elevatedLockFilePath[MAXPATHLEN] = {NS_T('\0')};
-  NS_tsnprintf(elevatedLockFilePath,
-               sizeof(elevatedLockFilePath) / sizeof(elevatedLockFilePath[0]),
-               NS_T("%s\\update_elevated.lock"), gPatchDirPath);
-  gUseSecureOutputPath =
-      sUsingService || (NS_tremove(elevatedLockFilePath) && errno != ENOENT);
-
-  // Even if a file has no sharing access, you can still get its attributes
-  // If we are running elevated, this file will exist, having been opened by
-  // the unelevated updater that started this one.
-  gIsElevated =
-      GetFileAttributesW(elevatedLockFilePath) != INVALID_FILE_ATTRIBUTES;
+  auto isAdmin = mozilla::UserHasAdminPrivileges();
+  if (isAdmin.isErr()) {
+    fprintf(stderr,
+            "Failed to query if the current process has admin privileges.\n");
+    return 1;
+  }
+  auto isLocalSystem = mozilla::UserIsLocalSystem();
+  if (isLocalSystem.isErr()) {
+    fprintf(
+        stderr,
+        "Failed to query if the current process has LocalSystem privileges.\n");
+    return 1;
+  }
+
+  // While is it technically redundant to check LocalSystem in addition to Admin
+  // given the former contains privileges of the latter, we have opt to verify
+  // both. A few reasons for this decision include the off chance that the
+  // Windows security model changes in the future and weird system setups where
+  // someone has modified the group lists in surprising ways.
+  //
+  // We use this to detect if we were launched from the Maintenance Service
+  // under LocalSystem or UAC under the user's account, and therefore can
+  // proceed with an install to `Program Files` or `Program Files(x86)`.
+  gIsElevated = isAdmin.unwrap() || isLocalSystem.unwrap();
+  gUseSecureOutputPath = sUsingService || gIsElevated;
 #elif defined(XP_MACOSX)
     // This is only ever true on macOS and Windows. We don't currently have a
     // way of elevating on other platforms.
@@ -3587,26 +3600,9 @@ int NS_main(int argc, NS_tchar** argv) {
            (noServiceFallback || forceServiceFallback))) {
         LOG(("Can't open lock file - seems like we need elevation"));
 
-        HANDLE elevatedFileHandle;
-        if (NS_tremove(elevatedLockFilePath) && errno != ENOENT) {
-          LOG(("Unable to create elevated lock file! Exiting"));
-          output_finish();
-          return 1;
-        }
-
-        elevatedFileHandle = CreateFileW(
-            elevatedLockFilePath, GENERIC_READ | GENERIC_WRITE, 0, nullptr,
-            OPEN_ALWAYS, FILE_FLAG_DELETE_ON_CLOSE, nullptr);
-        if (elevatedFileHandle == INVALID_HANDLE_VALUE) {
-          LOG(("Unable to create elevated lock file! Exiting"));
-          output_finish();
-          return 1;
-        }
-
         auto cmdLine = mozilla::MakeCommandLine(argc - 1, argv + 1);
         if (!cmdLine) {
           LOG(("Failed to make command line! Exiting"));
-          CloseHandle(elevatedFileHandle);
           output_finish();
           return 1;
         }
@@ -3846,17 +3842,6 @@ int NS_main(int argc, NS_tchar** argv) {
             // And we don't have a good way of accepting the prompt in
             // automation.
             sinfo.lpVerb = L"open";
-            // This handle is what lets the updater that we spawn below know
-            // that it's the elevated updater. We are going to close it so that
-            // it doesn't know that and will run un-elevated. Doing this make
-            // this makes for an imperfect test of the service fallback
-            // functionality because it changes how the (usually) elevated
-            // updater runs. One of the effects of this is that the secure
-            // output files will not be used. So that functionality won't really
-            // be covered by testing. But we can't really have the updater run
-            // elevated, because that would require a UAC, which we have no way
-            // to deal with in automation.
-            CloseHandle(elevatedFileHandle);
             // We need to let go of the update lock to let the un-elevated
             // updater we are about to spawn update.
             if (updateLockFileHandle != INVALID_HANDLE_VALUE) {
@@ -3927,8 +3912,6 @@ int NS_main(int argc, NS_tchar** argv) {
           }
         }
 
-        CloseHandle(elevatedFileHandle);
-
         if (updateLockFileHandle != INVALID_HANDLE_VALUE) {
           CloseHandle(updateLockFileHandle);
         }
@@ -4016,7 +3999,7 @@ int NS_main(int argc, NS_tchar** argv) {
       WriteStatusFile(WRITE_ERROR_APPLY_DIR_PATH);
       LOG(("NS_main: unable to find apply to dir: " LOG_S, gWorkingDirPath));
       output_finish();
-      EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
+      EXIT_WHEN_ELEVATED(updateLockFileHandle, 1);
       if (argc > callbackIndex) {
         LaunchCallbackApp(argv[5], argc - callbackIndex, argv + callbackIndex,
                           sUsingService);
@@ -4070,7 +4053,7 @@ int NS_main(int argc, NS_tchar** argv) {
         WriteStatusFile(WRITE_ERROR_CALLBACK_PATH);
         LOG(("NS_main: unable to find callback file: " LOG_S, targetPath));
         output_finish();
-        EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
+        EXIT_WHEN_ELEVATED(updateLockFileHandle, 1);
         if (argc > callbackIndex) {
           LaunchCallbackApp(argv[5], argc - callbackIndex, argv + callbackIndex,
                             sUsingService);
@@ -4119,7 +4102,7 @@ int NS_main(int argc, NS_tchar** argv) {
 
           // Don't attempt to launch the callback when the callback path is
           // longer than expected.
-          EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
+          EXIT_WHEN_ELEVATED(updateLockFileHandle, 1);
           return 1;
         }
 
@@ -4136,7 +4119,7 @@ int NS_main(int argc, NS_tchar** argv) {
                " into place at " LOG_S,
                argv[callbackIndex], gCallbackBackupPath));
           output_finish();
-          EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
+          EXIT_WHEN_ELEVATED(updateLockFileHandle, 1);
           LaunchCallbackApp(argv[callbackIndex], argc - callbackIndex,
                             argv + callbackIndex, sUsingService);
           return 1;
@@ -4211,7 +4194,7 @@ int NS_main(int argc, NS_tchar** argv) {
                    gCallbackBackupPath));
Loading diff…