From 2175404baccd08ee59826787f0b173b2cf1175c9 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Mon, 15 Sep 2014 20:29:35 -0400 Subject: [PATCH] Backout bug 945192 for intermittent failures. r=rstrong --- .../maintenanceservice/workmonitor.cpp | 2 + toolkit/components/telemetry/Histograms.json | 8 +- toolkit/mozapps/update/common/errors.h | 5 - toolkit/mozapps/update/common/uachelper.cpp | 95 --------- toolkit/mozapps/update/common/uachelper.h | 7 +- .../mozapps/update/common/updatehelper.cpp | 1 + toolkit/mozapps/update/common/updatehelper.h | 4 - toolkit/mozapps/update/nsUpdateService.js | 6 +- .../tests/unit_aus_update/head_update.js | 3 - toolkit/mozapps/update/updater/moz.build | 2 - toolkit/mozapps/update/updater/updater.cpp | 194 ++++-------------- 11 files changed, 47 insertions(+), 280 deletions(-) diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp index dd74aa4047dd..50e4beddb110 100644 --- a/toolkit/components/maintenanceservice/workmonitor.cpp +++ b/toolkit/components/maintenanceservice/workmonitor.cpp @@ -30,6 +30,8 @@ static const int TIME_TO_WAIT_ON_UPDATER = 15 * 60 * 1000; char16_t* MakeCommandLine(int argc, char16_t **argv); BOOL WriteStatusFailure(LPCWSTR updateDirPath, int errorCode); +BOOL PathGetSiblingFilePath(LPWSTR destinationBuffer, LPCWSTR siblingFilePath, + LPCWSTR newFileName); /* * Read the update.status file and sets isApplying to true if diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index 6b3350c673b1..3ed0456ba79c 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -3337,15 +3337,9 @@ "description": "Updater: The interval in days between the previous and the current background update check when the check was timer initiated" }, "UPDATER_STATUS_CODES": { - "expires_in_version": "35", - "kind": "enumerated", - "n_values": 50, - "description": "Updater: the status of the latest update performed" - }, - "UPDATER_ALL_STATUS_CODES": { "expires_in_version": "never", "kind": "enumerated", - "n_values": 200, + "n_values": 50, "description": "Updater: the status of the latest update performed" }, "UPDATER_UPDATES_ENABLED": { diff --git a/toolkit/mozapps/update/common/errors.h b/toolkit/mozapps/update/common/errors.h index b79740165187..360f2e3f2539 100644 --- a/toolkit/mozapps/update/common/errors.h +++ b/toolkit/mozapps/update/common/errors.h @@ -69,11 +69,6 @@ #define WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID 47 #define WRITE_ERROR_SHARING_VIOLATION_NOPID 48 -#define FOTA_FILE_OPERATION_ERROR 49 -#define FOTA_RECOVERY_ERROR 50 - -#define SECURE_LOCATION_UPDATE_ERROR 51 - // The following error codes are only used by updater.exe // when a fallback key exists and XPCShell tests are being run. #define FALLBACKKEY_UNKNOWN_ERROR 100 diff --git a/toolkit/mozapps/update/common/uachelper.cpp b/toolkit/mozapps/update/common/uachelper.cpp index 7e29e05ce26d..74ae4ca283f1 100644 --- a/toolkit/mozapps/update/common/uachelper.cpp +++ b/toolkit/mozapps/update/common/uachelper.cpp @@ -4,7 +4,6 @@ #include #include -#include #include "uachelper.h" #include "updatelogging.h" @@ -162,15 +161,11 @@ UACHelper::DisableUnneededPrivileges(HANDLE token, BOOL result = TRUE; for (size_t i = 0; i < count; i++) { if (SetPrivilege(token, unneededPrivs[i], FALSE)) { -#ifdef UPDATER_LOG_PRIVS LOG(("Disabled unneeded token privilege: %s.", unneededPrivs[i])); -#endif } else { -#ifdef UPDATER_LOG_PRIVS LOG(("Could not disable token privilege value: %s. (%d)", unneededPrivs[i], GetLastError())); -#endif result = FALSE; } } @@ -225,93 +220,3 @@ UACHelper::CanUserElevate() return canElevate; } - -/** - * Denies write access for everyone on the specified path. - * - * @param path The file path to modify the DACL on - * @param originalACL out parameter, set only if successful. - * caller must free. - * @return true on success - */ -bool -UACHelper::DenyWriteACLOnPath(LPCWSTR path, PACL *originalACL, - PSECURITY_DESCRIPTOR *sd) -{ - // Get the old security information on the path. - // Note that the actual buffer to be freed is contained in *sd. - // originalACL points within *sd's buffer. - *originalACL = nullptr; - *sd = nullptr; - DWORD result = - GetNamedSecurityInfoW(path, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, - nullptr, nullptr, originalACL, nullptr, sd); - if (result != ERROR_SUCCESS) { - *sd = nullptr; - *originalACL = nullptr; - return false; - } - - // Adjust the security for everyone to deny write - EXPLICIT_ACCESSW ea; - ZeroMemory(&ea, sizeof(EXPLICIT_ACCESSW)); - ea.grfAccessPermissions = FILE_APPEND_DATA | FILE_WRITE_ATTRIBUTES | - FILE_WRITE_DATA | FILE_WRITE_EA; - ea.grfAccessMode = DENY_ACCESS; - ea.grfInheritance = NO_INHERITANCE; - ea.Trustee.TrusteeForm = TRUSTEE_IS_NAME; - ea.Trustee.TrusteeType = TRUSTEE_IS_GROUP; - ea.Trustee.ptstrName = L"EVERYONE"; - PACL dacl = nullptr; - result = SetEntriesInAclW(1, &ea, *originalACL, &dacl); - if (result != ERROR_SUCCESS) { - LocalFree(*sd); - *originalACL = nullptr; - *sd = nullptr; - return false; - } - - // Update the path to have a the new DACL - result = SetNamedSecurityInfoW(const_cast(path), SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, nullptr, nullptr, - dacl, nullptr); - LocalFree(dacl); - return result == ERROR_SUCCESS; -} - -/** - * Determines if the specified directory only has updater.exe inside of it, - * and nothing else. - * - * @param inputPath the directory path to search - * @return true if updater.exe is the only file in the directory - */ -bool -UACHelper::IsDirectorySafe(LPCWSTR inputPath) -{ - WIN32_FIND_DATAW findData; - HANDLE findHandle = nullptr; - - WCHAR searchPath[MAX_PATH + 1] = { L'\0' }; - wsprintfW(searchPath, L"%s\\*.*", inputPath); - - findHandle = FindFirstFileW(searchPath, &findData); - if(findHandle == INVALID_HANDLE_VALUE) { - return false; - } - - // Enumerate the files and if we find anything other than the current - // directory, the parent directory, or updater.exe. Then fail. - do { - if(wcscmp(findData.cFileName, L".") != 0 && - wcscmp(findData.cFileName, L"..") != 0 && - wcscmp(findData.cFileName, L"updater.exe") != 0) { - FindClose(findHandle); - return false; - } - } while(FindNextFileW(findHandle, &findData)); - FindClose(findHandle); - - return true; -} - diff --git a/toolkit/mozapps/update/common/uachelper.h b/toolkit/mozapps/update/common/uachelper.h index cad7ebabd0ec..6481ff5b5ee4 100644 --- a/toolkit/mozapps/update/common/uachelper.h +++ b/toolkit/mozapps/update/common/uachelper.h @@ -12,15 +12,12 @@ public: static HANDLE OpenLinkedToken(HANDLE token); static BOOL DisablePrivileges(HANDLE token); static bool CanUserElevate(); - static bool IsDirectorySafe(LPCWSTR inputPath); - static bool DenyWriteACLOnPath(LPCWSTR path, PACL *originalACL, - PSECURITY_DESCRIPTOR *sd); private: static BOOL SetPrivilege(HANDLE token, LPCTSTR privs, BOOL enable); - static BOOL DisableUnneededPrivileges(HANDLE token, + static BOOL DisableUnneededPrivileges(HANDLE token, LPCTSTR *unneededPrivs, size_t count); - static LPCTSTR PrivsToDisable[]; + static LPCTSTR PrivsToDisable[]; }; #endif diff --git a/toolkit/mozapps/update/common/updatehelper.cpp b/toolkit/mozapps/update/common/updatehelper.cpp index ae3e177cb1b3..45ab09212132 100644 --- a/toolkit/mozapps/update/common/updatehelper.cpp +++ b/toolkit/mozapps/update/common/updatehelper.cpp @@ -33,6 +33,7 @@ using mozilla::MakeUnique; using mozilla::UniquePtr; WCHAR* MakeCommandLine(int argc, WCHAR **argv); +BOOL PathAppendSafe(LPWSTR base, LPCWSTR extra); /** * Obtains the path of a file in the same directory as the specified file. diff --git a/toolkit/mozapps/update/common/updatehelper.h b/toolkit/mozapps/update/common/updatehelper.h index 5ba2b0c0a583..9e21a9c646e5 100644 --- a/toolkit/mozapps/update/common/updatehelper.h +++ b/toolkit/mozapps/update/common/updatehelper.h @@ -17,10 +17,6 @@ BOOL DoesFallbackKeyExist(); BOOL IsLocalFile(LPCWSTR file, BOOL &isLocal); DWORD StartServiceCommand(int argc, LPCWSTR* argv); BOOL IsUnpromptedElevation(BOOL &isUnpromptedElevation); -BOOL PathGetSiblingFilePath(LPWSTR destinationBuffer, - LPCWSTR siblingFilePath, - LPCWSTR newFileName); -BOOL PathAppendSafe(LPWSTR base, LPCWSTR extra); #define SVC_NAME L"MozillaMaintenance" diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js index fe3a3ff2de02..b4c3af66e8ec 100644 --- a/toolkit/mozapps/update/nsUpdateService.js +++ b/toolkit/mozapps/update/nsUpdateService.js @@ -162,7 +162,6 @@ const WRITE_ERROR_SHARING_VIOLATION_NOPROCESSFORPID = 47; const WRITE_ERROR_SHARING_VIOLATION_NOPID = 48; const FOTA_FILE_OPERATION_ERROR = 49; const FOTA_RECOVERY_ERROR = 50; -const SECURE_LOCATION_UPDATE_ERROR = 51; const CERT_ATTR_CHECK_FAILED_NO_UPDATE = 100; const CERT_ATTR_CHECK_FAILED_HAS_UPDATE = 101; @@ -1478,8 +1477,7 @@ function handleUpdateFailure(update, errorCode) { return true; } - if (update.errorCode == ELEVATION_CANCELED || - update.errorCode == SECURE_LOCATION_UPDATE_ERROR) { + if (update.errorCode == ELEVATION_CANCELED) { writeStatusFile(getUpdatesDir(), update.state = STATE_PENDING); return true; } @@ -2385,7 +2383,7 @@ UpdateService.prototype = { if (parts.length > 1) { result = parseInt(parts[1]) || INVALID_UPDATER_STATUS_CODE; } - Services.telemetry.getHistogramById("UPDATER_ALL_STATUS_CODES").add(result); + Services.telemetry.getHistogramById("UPDATER_STATUS_CODES").add(result); } catch(e) { // Don't allow any exception to be propagated. Cu.reportError(e); diff --git a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js index d7e312828eab..f68292310ac3 100644 --- a/toolkit/mozapps/update/tests/unit_aus_update/head_update.js +++ b/toolkit/mozapps/update/tests/unit_aus_update/head_update.js @@ -1508,18 +1508,15 @@ function runUpdate(aExpectedExitValue, aExpectedStatus, aCallback) { if (gDisableReplaceFallback) { env.set("MOZ_NO_REPLACE_FALLBACK", "1"); } - env.set("MOZ_EMULATE_ELEVATION_PATH", "1"); let process = AUS_Cc["@mozilla.org/process/util;1"]. createInstance(AUS_Ci.nsIProcess); process.init(updateBin); - process.run(true, args, args.length); if (gDisableReplaceFallback) { env.set("MOZ_NO_REPLACE_FALLBACK", ""); } - env.set("MOZ_EMULATE_ELEVATION_PATH", ""); let status = readStatusFile(); if (process.exitValue != aExpectedExitValue || status != aExpectedStatus) { diff --git a/toolkit/mozapps/update/updater/moz.build b/toolkit/mozapps/update/updater/moz.build index f1ee7649927f..153217539d6c 100644 --- a/toolkit/mozapps/update/updater/moz.build +++ b/toolkit/mozapps/update/updater/moz.build @@ -41,8 +41,6 @@ if CONFIG['OS_ARCH'] == 'WINNT': 'shlwapi', 'crypt32', 'advapi32', - 'ole32', - 'rpcrt4', ] else: USE_LIBS += [ diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp index 01fd9f6c6f64..ce6af32b9f3b 100644 --- a/toolkit/mozapps/update/updater/updater.cpp +++ b/toolkit/mozapps/update/updater/updater.cpp @@ -111,7 +111,6 @@ static bool sUseHardLinks = true; #ifdef XP_WIN #include "updatehelper.h" -#include // Closes the handle if valid and if the updater is elevated returns with the // return code specified. This prevents multiple launches of the callback @@ -2394,9 +2393,8 @@ int NS_main(int argc, NS_tchar **argv) bool useService = false; bool testOnlyFallbackKeyExists = false; bool noServiceFallback = getenv("MOZ_NO_SERVICE_FALLBACK") != nullptr; - bool emulateElevation = getenv("MOZ_EMULATE_ELEVATION_PATH") != nullptr; putenv(const_cast("MOZ_NO_SERVICE_FALLBACK=")); - putenv(const_cast("MOZ_EMULATE_ELEVATION_PATH=")); + // We never want the service to be used unless we build with // the maintenance service. #ifdef MOZ_MAINTENANCE_SERVICE @@ -2486,6 +2484,12 @@ int NS_main(int argc, NS_tchar **argv) return 1; } + if (sStagedUpdate) { + LOG(("Performing a staged update")); + } else if (sReplaceRequest) { + LOG(("Performing a replace request")); + } + #ifdef MOZ_WIDGET_GONK const char *prioEnv = getenv("MOZ_UPDATER_PRIO"); if (prioEnv) { @@ -2614,15 +2618,13 @@ int NS_main(int argc, NS_tchar **argv) return 1; } - if (!emulateElevation) { - updateLockFileHandle = CreateFileW(updateLockFilePath, - GENERIC_READ | GENERIC_WRITE, - 0, - nullptr, - OPEN_ALWAYS, - FILE_FLAG_DELETE_ON_CLOSE, - nullptr); - } + updateLockFileHandle = CreateFileW(updateLockFilePath, + GENERIC_READ | GENERIC_WRITE, + 0, + nullptr, + OPEN_ALWAYS, + FILE_FLAG_DELETE_ON_CLOSE, + nullptr); NS_tsnprintf(elevatedLockFilePath, sizeof(elevatedLockFilePath)/sizeof(elevatedLockFilePath[0]), @@ -2773,8 +2775,7 @@ int NS_main(int argc, NS_tchar **argv) // If the service can't be used when staging and update, make sure that // the UAC prompt is not shown! In this case, just set the status to // pending and the update will be applied during the next startup. - // When emulateElevation is true fall through to the elevation code path. - if (!useService && sStagedUpdate && !emulateElevation) { + if (!useService && sStagedUpdate) { if (updateLockFileHandle != INVALID_HANDLE_VALUE) { CloseHandle(updateLockFileHandle); } @@ -2800,145 +2801,35 @@ int NS_main(int argc, NS_tchar **argv) } } - DWORD returnCode = 0; - - // If we didn't want to use the service at all, or if an update was - // already happening, or launching the service command failed, then + // If we didn't want to use the service at all, or if an update was + // already happening, or launching the service command failed, then // launch the elevated updater.exe as we do without the service. // We don't launch the elevated updater in the case that we did have // write access all along because in that case the only reason we're - // using the service is because we are testing. - if (!useService && !noServiceFallback && + // using the service is because we are testing. + if (!useService && !noServiceFallback && updateLockFileHandle == INVALID_HANDLE_VALUE) { + SHELLEXECUTEINFO sinfo; + memset(&sinfo, 0, sizeof(SHELLEXECUTEINFO)); + sinfo.cbSize = sizeof(SHELLEXECUTEINFO); + sinfo.fMask = SEE_MASK_FLAG_NO_UI | + SEE_MASK_FLAG_DDEWAIT | + SEE_MASK_NOCLOSEPROCESS; + sinfo.hwnd = nullptr; + sinfo.lpFile = argv[0]; + sinfo.lpParameters = cmdLine; + sinfo.lpVerb = L"runas"; + sinfo.nShow = SW_SHOWNORMAL; - // Get a unique directory name to secure - RPC_WSTR guidString = RPC_WSTR(L""); - GUID guid; - HRESULT hr = CoCreateGuid(&guid); - BOOL result = TRUE; - bool safeToUpdate = true; - WCHAR secureUpdaterPath[MAX_PATH + 1] = { L'\0' }; - WCHAR secureDirPath[MAX_PATH + 1] = { L'\0' }; - if (SUCCEEDED(hr)) { - UuidToString(&guid, &guidString); - result = PathGetSiblingFilePath(secureDirPath, argv[0], - reinterpret_cast(guidString)); - RpcStringFree(&guidString); + bool result = ShellExecuteEx(&sinfo); + free(cmdLine); + + if (result) { + WaitForSingleObject(sinfo.hProcess, INFINITE); + CloseHandle(sinfo.hProcess); } else { - // This should never happen, but just in case - result = PathGetSiblingFilePath(secureDirPath, argv[0], L"tmp_update"); + WriteStatusFile(ELEVATION_CANCELED); } - - if (!result) { - fprintf(stderr, "Could not obtain secure update directory path"); - safeToUpdate = false; - } - - // If it's still safe to update, create the directory - if (safeToUpdate) { - result = CreateDirectoryW(secureDirPath, nullptr); - if (!result) { - fprintf(stderr, "Could not create secure update directory"); - safeToUpdate = false; - } - } - - // If it's still safe to update, get the new updater path - if (safeToUpdate) { - wcsncpy(secureUpdaterPath, secureDirPath, MAX_PATH); - result = PathAppendSafe(secureUpdaterPath, L"updater.exe"); - if (!result) { - fprintf(stderr, "Could not obtain secure updater file name"); - safeToUpdate = false; - } - } - - // If it's still safe to update, copy the file in - if (safeToUpdate) { - result = CopyFileW(argv[0], secureUpdaterPath, TRUE); - if (!result) { - fprintf(stderr, "Could not copy updater to secure location"); - safeToUpdate = false; - } - } - - // If it's still safe to update, restrict access to the directory item - // itself so that the directory cannot be deleted and re-created, - // nor have its properties modified. Note that this does not disallow - // adding items inside the directory. - HANDLE handle = INVALID_HANDLE_VALUE; - if (safeToUpdate) { - handle = CreateFileW(secureDirPath, GENERIC_READ, FILE_SHARE_READ, - nullptr, OPEN_EXISTING, - FILE_FLAG_BACKUP_SEMANTICS, nullptr); - safeToUpdate = handle != INVALID_HANDLE_VALUE; - } - - // If it's still safe to update, deny write access completely to the - // directory. - PACL originalACL = nullptr; - PSECURITY_DESCRIPTOR sd = nullptr; - if (safeToUpdate) { - safeToUpdate = UACHelper::DenyWriteACLOnPath(secureDirPath, - &originalACL, &sd); - } - - // If it's still safe to update, verify that there is only updater.exe - // in the directory and nothing else. - if (safeToUpdate) { - if (!UACHelper::IsDirectorySafe(secureDirPath)) { - safeToUpdate = false; - } - } - - if (!safeToUpdate) { - fprintf(stderr, "Will not proceed to copy secure updater because it " - "is not safe to do so."); - WriteStatusFile(SECURE_LOCATION_UPDATE_ERROR); - } else { - SHELLEXECUTEINFO sinfo; - memset(&sinfo, 0, sizeof(SHELLEXECUTEINFO)); - sinfo.cbSize = sizeof(SHELLEXECUTEINFO); - sinfo.fMask = SEE_MASK_FLAG_NO_UI | - SEE_MASK_FLAG_DDEWAIT | - SEE_MASK_NOCLOSEPROCESS; - sinfo.hwnd = nullptr; - sinfo.lpFile = secureUpdaterPath; - sinfo.lpParameters = cmdLine; - sinfo.lpVerb = emulateElevation ? L"open" : L"runas"; - sinfo.nShow = SW_SHOWNORMAL; - - bool result = ShellExecuteEx(&sinfo); - free(cmdLine); - - if (result) { - WaitForSingleObject(sinfo.hProcess, INFINITE); - // Bubble the elevated updater return code to this updater - GetExitCodeProcess(sinfo.hProcess, &returnCode); - CloseHandle(sinfo.hProcess); - } else { - WriteStatusFile(ELEVATION_CANCELED); - } - } - - // All done, revert back the permissions. - if (originalACL) { - SetNamedSecurityInfoW(const_cast(secureDirPath), SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION, nullptr, nullptr, - originalACL, nullptr); - } - if (sd) { - LocalFree(sd); - } - - // Done with the directory, no need to lock it. - if (INVALID_HANDLE_VALUE != handle) { - CloseHandle(handle); - } - - // We no longer need the secure updater and directory - DeleteFileW(secureUpdaterPath); - RemoveDirectoryW(secureDirPath); } if (argc > callbackIndex) { @@ -2953,7 +2844,7 @@ int NS_main(int argc, NS_tchar **argv) // We didn't use the service and we did run the elevated updater.exe. // The elevated updater.exe is responsible for writing out the // update.status file. - return returnCode; + return 0; } else if(useService) { // The service command was launched. The service is responsible for // writing out the update.status file. @@ -2975,13 +2866,6 @@ int NS_main(int argc, NS_tchar **argv) } #endif - if (sStagedUpdate) { - LOG(("Performing a staged update")); - } - else if (sReplaceRequest) { - LOG(("Performing a replace request")); - } - #if defined(MOZ_WIDGET_GONK) // In gonk, the master b2g process sets its umask to 0027 because // there's no reason for it to ever create world-readable files. @@ -3286,7 +3170,7 @@ int NS_main(int argc, NS_tchar **argv) } } } - EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, gSucceeded ? 0 : 1); + EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 0); #endif /* XP_WIN */ #ifdef XP_MACOSX if (gSucceeded) {