diff --git a/omaha/base/constants.h b/omaha/base/constants.h index 6ea7d7a0e..db0722796 100644 --- a/omaha/base/constants.h +++ b/omaha/base/constants.h @@ -282,6 +282,17 @@ const TCHAR* const kRegValueProxyHost = _T("ProxyHost"); const TCHAR* const kRegValueProxyPort = _T("ProxyPort"); const TCHAR* const kRegValueMID = _T("mid"); +const TCHAR* const kRegValueDisablePayloadAuthenticodeVerification = + _T("DisablePayloadAuthenticodeVerification"); + +// File extensions that can be verified with an Authenticode signature. +const TCHAR* const kAuthenticodeVerifiableExtensions[] = { + _T("exe"), _T("msi"), _T("dll"), _T("sys"), _T("cab"), _T("ocx"), + _T("xpi"), _T("xap"), _T("cat"), _T("jar"), _T("ps1"), _T("psm1"), + _T("psd1"), _T("ps1xml"), _T("psc1"), _T("acm "), _T("ax"), _T("cpl"), + _T("drv"), _T("efi"), _T("mui"), _T("scr"), _T("sys"), _T("tsp") +}; + #if defined(HAS_DEVICE_MANAGEMENT) const TCHAR* const kRegValueNameDeviceManagementUrl = _T("DeviceManagementUrl"); #endif diff --git a/omaha/base/error.h b/omaha/base/error.h index f6a358dca..59bc70065 100644 --- a/omaha/base/error.h +++ b/omaha/base/error.h @@ -194,6 +194,9 @@ const ULONG kFacilityOmaha = 67; #define GOOPDATEDOWNLOAD_E_CACHING_FAILED \ MAKE_OMAHA_HRESULT(SEVERITY_ERROR, 0x50D) +#define GOOPDATEDOWNLOAD_E_AUTHENTICODE_VERIFICATION_FAILED \ + MAKE_OMAHA_HRESULT(SEVERITY_ERROR, 0x50E) + #define GOOPDATEDOWNLOAD_E_FAILED_MOVE \ MAKE_HRESULT(SEVERITY_ERROR, FACILITY_ITF, 0x5FF) diff --git a/omaha/common/config_manager.cc b/omaha/common/config_manager.cc index 80d2e2ac0..a06edd3a1 100644 --- a/omaha/common/config_manager.cc +++ b/omaha/common/config_manager.cc @@ -1800,6 +1800,18 @@ bool ConfigManager::AlwaysAllowCrashUploads() const { return always_allow_crash_uploads != 0; } +bool ConfigManager::ShouldVerifyPayloadAuthenticodeSignature() const { +#ifdef VERIFY_PAYLOAD_AUTHENTICODE_SIGNATURE + DWORD disabled_in_registry = 0; + RegKey::GetValue(MACHINE_REG_UPDATE_DEV, + kRegValueDisablePayloadAuthenticodeVerification, + &disabled_in_registry); + return disabled_in_registry == 0; +#else + return false; +#endif // VERIFY_PAYLOAD_AUTHENTICODE_SIGNATURE +} + int ConfigManager::MaxCrashUploadsPerDay() const { DWORD num_uploads = 0; if (FAILED(RegKey::GetValue(MACHINE_REG_UPDATE_DEV, diff --git a/omaha/common/config_manager.h b/omaha/common/config_manager.h index 1be515780..8ff9abb06 100644 --- a/omaha/common/config_manager.h +++ b/omaha/common/config_manager.h @@ -427,6 +427,10 @@ class ConfigManager { // build flavor or other configuration parameters. bool AlwaysAllowCrashUploads() const; + // Returns whether the Authenticode signature of update payloads should be + // verified. + bool ShouldVerifyPayloadAuthenticodeSignature() const; + // Returns the number of crashes to upload per day. int MaxCrashUploadsPerDay() const; diff --git a/omaha/goopdate/download_manager.cc b/omaha/goopdate/download_manager.cc index e6cdcc04d..01325dffb 100644 --- a/omaha/goopdate/download_manager.cc +++ b/omaha/goopdate/download_manager.cc @@ -41,6 +41,7 @@ #include "omaha/base/utils.h" #include "omaha/common/config_manager.h" #include "omaha/common/const_goopdate.h" +#include "omaha/common/google_signaturevalidator.h" #include "omaha/goopdate/model.h" #include "omaha/goopdate/package_cache.h" #include "omaha/goopdate/server_resource.h" @@ -217,6 +218,7 @@ CString DownloadManager::GetMessageForError(const ErrorContext& error_context, case GOOPDATEDOWNLOAD_E_FILE_SIZE_ZERO: case GOOPDATEDOWNLOAD_E_FILE_SIZE_SMALLER: case GOOPDATEDOWNLOAD_E_FILE_SIZE_LARGER: + case GOOPDATEDOWNLOAD_E_AUTHENTICODE_VERIFICATION_FAILED: VERIFY_SUCCEEDED(formatter.LoadString(IDS_DOWNLOAD_HASH_MISMATCH, &message)); break; @@ -475,10 +477,11 @@ HRESULT DownloadManager::DoDownloadPackageFromUrl(const CString& url, // We copy the file to the Package Cache unimpersonated, since the package // cache is in a privileged location. - hr = CallAsSelfAndImpersonate2(this, + hr = CallAsSelfAndImpersonate3(this, &DownloadManager::CachePackage, static_cast(package), - &source_file); + &source_file, + &filename); if (FAILED(hr)) { OPT_LOG(LE, (_T("[DownloadManager::CachePackage failed][%#x]"), hr)); } @@ -521,7 +524,8 @@ HRESULT DownloadManager::PurgeAppLowerVersions(const CString& app_id, } HRESULT DownloadManager::CachePackage(const Package* package, - File* source_file) { + File* source_file, + const CString* source_file_path) { ASSERT1(package); ASSERT1(source_file); @@ -530,7 +534,18 @@ HRESULT DownloadManager::CachePackage(const Package* package, const CString package_name(package->filename()); PackageCache::Key key(app_id, version, package_name); - HRESULT hr = package_cache()->Put( + HRESULT hr = E_UNEXPECTED; + + if (ConfigManager::Instance()->ShouldVerifyPayloadAuthenticodeSignature()) { + hr = EnsureSignatureIsValid(*source_file_path); + if (FAILED(hr)) { + CORE_LOG(LE, (_T("[EnsureSignatureIsValid failed][%s][0x%08x]"), + package->filename(), hr)); + return GOOPDATEDOWNLOAD_E_AUTHENTICODE_VERIFICATION_FAILED; + } + } + + hr = package_cache()->Put( key, source_file, package->expected_hash()); if (hr != SIGS_E_INVALID_SIGNATURE) { if (FAILED(hr)) { @@ -552,6 +567,21 @@ HRESULT DownloadManager::CachePackage(const Package* package, return hr; } +HRESULT DownloadManager::EnsureSignatureIsValid(const CString& file_path) { + const TCHAR* ext = ::PathFindExtension(file_path); + ASSERT1(ext); + if (*ext != _T('\0')) { + ext++; // Skip the dot. + for (size_t i = 0; i < arraysize(kAuthenticodeVerifiableExtensions); ++i) { + if (CString(kAuthenticodeVerifiableExtensions[i]).CompareNoCase(ext) + == 0) { + return VerifyGoogleAuthenticodeSignature(file_path, true); + } + } + } + return S_OK; +} + // The file is initially downloaded to a temporary unique name, to account // for the case where the same file is downloaded by multiple callers. HRESULT DownloadManager::BuildUniqueFileName(const CString& filename, diff --git a/omaha/goopdate/download_manager.h b/omaha/goopdate/download_manager.h index e896ba956..f3c40c586 100644 --- a/omaha/goopdate/download_manager.h +++ b/omaha/goopdate/download_manager.h @@ -42,7 +42,8 @@ class DownloadManagerInterface { virtual HRESULT PurgeAppLowerVersions(const CString& app_id, const CString& version) = 0; virtual HRESULT CachePackage(const Package* package, - File* source_file) = 0; + File* source_file, + const CString* source_file_path) = 0; virtual HRESULT DownloadApp(App* app) = 0; virtual HRESULT GetPackage(const Package* package, const CString& dir) const = 0; @@ -62,7 +63,9 @@ class DownloadManager : public DownloadManagerInterface { virtual HRESULT PurgeAppLowerVersions(const CString& app_id, const CString& version); - virtual HRESULT CachePackage(const Package* package, File* source_file); + virtual HRESULT CachePackage(const Package* package, + File* source_file, + const CString* source_file_path); // Downloads the specified app and stores its packages in the package cache. // @@ -128,6 +131,8 @@ class DownloadManager : public DownloadManagerInterface { Package* package, State* state); + HRESULT EnsureSignatureIsValid(const CString& file_path); + bool is_machine() const; CString package_cache_root() const; diff --git a/omaha/goopdate/download_manager_unittest.cc b/omaha/goopdate/download_manager_unittest.cc index 9035486ff..b12a4f949 100644 --- a/omaha/goopdate/download_manager_unittest.cc +++ b/omaha/goopdate/download_manager_unittest.cc @@ -16,6 +16,7 @@ // TODO(omaha): why so many dependencies for this unit test? #include +#include #include #include "omaha/base/app_util.h" @@ -88,6 +89,16 @@ class DownloadManagerTest : public AppTestBase { CleanupFiles(); + RegKey::GetValue(MACHINE_REG_UPDATE_DEV, + kRegValueDisablePayloadAuthenticodeVerification, + &disable_payload_authenticode_verification_); + if (disable_payload_authenticode_verification_) { + // Ensure the registry is in a clean state w.r.t. payload verification. + EXPECT_SUCCEEDED(RegKey::DeleteValue( + MACHINE_REG_UPDATE_DEV, + kRegValueDisablePayloadAuthenticodeVerification)); + } + download_manager_.reset(new DownloadManager(is_machine_)); EXPECT_SUCCEEDED(download_manager_->Initialize()); } @@ -96,11 +107,55 @@ class DownloadManagerTest : public AppTestBase { download_manager_.reset(); CleanupFiles(); + if (disable_payload_authenticode_verification_) { + EXPECT_SUCCEEDED(RegKey::SetValue( + MACHINE_REG_UPDATE_DEV, + kRegValueDisablePayloadAuthenticodeVerification, + disable_payload_authenticode_verification_)); + } else { + EXPECT_SUCCEEDED(RegKey::DeleteValue( + MACHINE_REG_UPDATE_DEV, + kRegValueDisablePayloadAuthenticodeVerification)); + } + AppTestBase::TearDown(); } virtual void CleanupFiles() = 0; + void TestCachePackage(App* app, + const TCHAR* unittest_support_file_name, + HRESULT expected_result) { + CString file_path(app_util::GetCurrentModuleDirectory()); + ASSERT_TRUE(::PathAppend(CStrBuf(file_path, MAX_PATH), + _T("unittest_support"))); + ASSERT_TRUE(::PathAppend(CStrBuf(file_path, MAX_PATH), + unittest_support_file_name)); + ASSERT_TRUE(File::Exists(file_path)); + + File file; + HRESULT hr = file.OpenShareMode(file_path, false, false, FILE_SHARE_READ); + ASSERT_SUCCEEDED(hr); + + uint32 file_size(0); + ASSERT_SUCCEEDED(file.GetLength(&file_size)); + + CryptoHash crypto; + std::vector hash_out; + ASSERT_HRESULT_SUCCEEDED(crypto.Compute(file_path, 0, &hash_out)); + std::string hash; + b2a_hex(&hash_out[0], &hash, hash_out.size()); + + AppVersion* version = app->next_version(); + ASSERT_SUCCEEDED(version->AddPackage(unittest_support_file_name, + file_size, + CString(hash.c_str()))); + + Package* package = version->GetPackage(version->GetNumberOfPackages() - 1); + hr = download_manager_->CachePackage(package, &file, &file_path); + EXPECT_EQ(expected_result, hr) << unittest_support_file_name; + } + static void SetAppStateCheckingForUpdate(App* app) { SetAppStateForUnitTest(app, new fsm::AppStateCheckingForUpdate); } @@ -111,6 +166,7 @@ class DownloadManagerTest : public AppTestBase { const CString cache_path_; std::unique_ptr download_manager_; + DWORD disable_payload_authenticode_verification_ = 0; // Saved from registry }; @@ -1177,6 +1233,29 @@ TEST_F(DownloadManagerUserTest, GetPackage) { EXPECT_SUCCEEDED(DeleteDirectory(dir)); } +TEST_F(DownloadManagerUserTest, CachePackage) { + App* app = NULL; + ASSERT_SUCCEEDED(app_bundle_->createApp(CComBSTR(kAppGuid1), &app)); + + TestCachePackage(app, _T("SaveArguments.exe"), S_OK); + TestCachePackage(app, _T("sha2_0c15be4a15bb0903c901b1d6c265302f.msi"), S_OK); + // Make sure that unexpected file extensions are handled gracefully: + TestCachePackage(app, _T("declaration.txt"), S_OK); + +#ifdef VERIFY_PAYLOAD_AUTHENTICODE_SIGNATURE + const TCHAR* kFileWithOldCertificate = _T("old_google_certificate.dll"); + TestCachePackage(app, + kFileWithOldCertificate, + GOOPDATEDOWNLOAD_E_AUTHENTICODE_VERIFICATION_FAILED); + // Test that disabling verification makes the previously failing file succeed: + EXPECT_SUCCEEDED(RegKey::SetValue( + MACHINE_REG_UPDATE_DEV, + kRegValueDisablePayloadAuthenticodeVerification, + 1UL)); + TestCachePackage(app, kFileWithOldCertificate, S_OK); +#endif // VERIFY_PAYLOAD_AUTHENTICODE_SIGNATURE +} + TEST_F(DownloadManagerUserTest, GetPackage_NotPresent) { App* app = NULL; ASSERT_SUCCEEDED(app_bundle_->createApp(CComBSTR(kAppGuid1), &app)); diff --git a/omaha/goopdate/worker.cc b/omaha/goopdate/worker.cc index 8bb48e6fb..b898ff9e5 100644 --- a/omaha/goopdate/worker.cc +++ b/omaha/goopdate/worker.cc @@ -872,7 +872,9 @@ HRESULT Worker::CacheOfflinePackages(AppBundle* app_bundle) { return hr; } - hr = download_manager_->CachePackage(package, &offline_package_file); + hr = download_manager_->CachePackage(package, + &offline_package_file, + &offline_package_path); if (FAILED(hr)) { CORE_LOG(LE, (_T("[CachePackage failed][%s][%s][0x%x][%Iu]"), app->app_guid_string(), offline_package_path, hr, j)); diff --git a/omaha/goopdate/worker_unittest.cc b/omaha/goopdate/worker_unittest.cc index 6f02a35d0..7de16469e 100644 --- a/omaha/goopdate/worker_unittest.cc +++ b/omaha/goopdate/worker_unittest.cc @@ -157,8 +157,8 @@ class MockDownloadManager : public DownloadManagerInterface { HRESULT()); MOCK_METHOD2(PurgeAppLowerVersions, HRESULT(const CString&, const CString&)); - MOCK_METHOD2(CachePackage, - HRESULT(const Package*, File*)); + MOCK_METHOD3(CachePackage, + HRESULT(const Package*, File*, const CString*)); MOCK_METHOD1(DownloadApp, HRESULT(App* app)); MOCK_METHOD1(DownloadPackage, diff --git a/omaha/main.scons b/omaha/main.scons index 611b84bae..1d4ed56ef 100644 --- a/omaha/main.scons +++ b/omaha/main.scons @@ -286,6 +286,8 @@ DeclareBit('analysis', 'Turns on static analysis') DeclareBit('suppress_light_validation', 'Suppress WiX Linker ICE validation') DeclareBit('has_device_management', 'True if including cloud-based device management') +DeclareBit('verify_payload_authenticode_signature', + 'Whether to verify the Authenticode signature of payloads') # Build official installers if --official_installers is on the command line. win_env.SetBitFromOption('official_installers', False) @@ -365,6 +367,9 @@ win_env.SetBitFromOption('analysis', False) # non-interactive account or a non-Admin account: http://goo.gl/UyDzeP. win_env.SetBitFromOption('suppress_light_validation', False) +# Whether the Authenticode signature of update payloads should be verified. +win_env.SetBitFromOption('verify_payload_authenticode_signature', False) + # # Set up version info. # @@ -413,6 +418,11 @@ if win_env.Bit('has_device_management'): CPPDEFINES = ['HAS_LEGACY_DM_CLIENT'], ) +if win_env.Bit('verify_payload_authenticode_signature'): + win_env.Append( + CPPDEFINES = ['VERIFY_PAYLOAD_AUTHENTICODE_SIGNATURE'], + ) + # Make sure python.exe can be located. win_env.AppendENVPath('PATH', os.environ['OMAHA_PYTHON_DIR'])