Firefox · Toolkit
CVE-2025-2817
Logic Error in Toolkit
Overview
High
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Changed Functions
| Function | Change | Notes |
|---|---|---|
iftoolkit/mozapps/update/updater/updater.cpp |
modified |
Files Changed
toolkit/mozapps/update/updater/updater-common.buildtoolkit/mozapps/update/updater/updater.cpptoolkit/xre/WinTokenUtils.cpptoolkit/xre/WinTokenUtils.h
Patch
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…
References
On This Page