From e6d3f78a05d19aa53e8d9afba8ab1bb11524e7ad Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Sun, 25 May 2014 21:58:53 -0400 Subject: [PATCH] Bug 945192. 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 | 91 +++++++++++ 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 +- toolkit/mozapps/update/updater/moz.build | 2 + toolkit/mozapps/update/updater/updater.cpp | 151 +++++++++++++++--- 10 files changed, 247 insertions(+), 30 deletions(-) diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp index 50e4beddb11..dd74aa4047d 100644 --- a/toolkit/components/maintenanceservice/workmonitor.cpp +++ b/toolkit/components/maintenanceservice/workmonitor.cpp @@ -30,8 +30,6 @@ 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 80049f78644..0165e286efa 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -3321,11 +3321,17 @@ "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": "never", + "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, + "description": "Updater: the status of the latest update performed" + }, "UPDATER_UPDATES_ENABLED": { "expires_in_version": "never", "kind": "boolean", diff --git a/toolkit/mozapps/update/common/errors.h b/toolkit/mozapps/update/common/errors.h index 360f2e3f253..b7974016518 100644 --- a/toolkit/mozapps/update/common/errors.h +++ b/toolkit/mozapps/update/common/errors.h @@ -69,6 +69,11 @@ #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 74ae4ca283f..4ed7385e603 100644 --- a/toolkit/mozapps/update/common/uachelper.cpp +++ b/toolkit/mozapps/update/common/uachelper.cpp @@ -4,6 +4,7 @@ #include #include +#include #include "uachelper.h" #include "updatelogging.h" @@ -220,3 +221,93 @@ 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 6481ff5b5ee..cad7ebabd0e 100644 --- a/toolkit/mozapps/update/common/uachelper.h +++ b/toolkit/mozapps/update/common/uachelper.h @@ -12,12 +12,15 @@ 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 45ab0921213..ae3e177cb1b 100644 --- a/toolkit/mozapps/update/common/updatehelper.cpp +++ b/toolkit/mozapps/update/common/updatehelper.cpp @@ -33,7 +33,6 @@ 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 9e21a9c646e..5ba2b0c0a58 100644 --- a/toolkit/mozapps/update/common/updatehelper.h +++ b/toolkit/mozapps/update/common/updatehelper.h @@ -17,6 +17,10 @@ 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 4942982bdb7..5770ec92891 100644 --- a/toolkit/mozapps/update/nsUpdateService.js +++ b/toolkit/mozapps/update/nsUpdateService.js @@ -162,6 +162,7 @@ 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; @@ -1477,7 +1478,8 @@ function handleUpdateFailure(update, errorCode) { return true; } - if (update.errorCode == ELEVATION_CANCELED) { + if (update.errorCode == ELEVATION_CANCELED || + update.errorCode == SECURE_LOCATION_UPDATE_ERROR) { writeStatusFile(getUpdatesDir(), update.state = STATE_PENDING); return true; } @@ -2383,7 +2385,7 @@ UpdateService.prototype = { if (parts.length > 1) { result = parseInt(parts[1]) || INVALID_UPDATER_STATUS_CODE; } - Services.telemetry.getHistogramById("UPDATER_STATUS_CODES").add(result); + Services.telemetry.getHistogramById("UPDATER_ALL_STATUS_CODES").add(result); } catch(e) { // Don't allow any exception to be propagated. Cu.reportError(e); diff --git a/toolkit/mozapps/update/updater/moz.build b/toolkit/mozapps/update/updater/moz.build index 153217539d6..f1ee7649927 100644 --- a/toolkit/mozapps/update/updater/moz.build +++ b/toolkit/mozapps/update/updater/moz.build @@ -41,6 +41,8 @@ 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 ce6af32b9f3..35664316e9a 100644 --- a/toolkit/mozapps/update/updater/updater.cpp +++ b/toolkit/mozapps/update/updater/updater.cpp @@ -111,6 +111,7 @@ 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 @@ -2801,35 +2802,141 @@ int NS_main(int argc, NS_tchar **argv) } } - // 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; - bool result = ShellExecuteEx(&sinfo); - free(cmdLine); - - if (result) { - WaitForSingleObject(sinfo.hProcess, INFINITE); - CloseHandle(sinfo.hProcess); + // 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); } else { - WriteStatusFile(ELEVATION_CANCELED); + // This should never happen, but just in case + result = PathGetSiblingFilePath(secureDirPath, argv[0], L"tmp_update"); } + + 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 = L"runas"; + sinfo.nShow = SW_SHOWNORMAL; + + bool result = ShellExecuteEx(&sinfo); + free(cmdLine); + + if (result) { + WaitForSingleObject(sinfo.hProcess, INFINITE); + 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) {