Extend Authenticode signature verification (#296)

* Extend Authenticode signature verification

The previous implementation only verified Authenticode signatures of
update binaries in rare cases, namely code red checks and app commands.
This commit adds a new registry DWORD value:

    UpdateDev\AlwaysVerifyAuthenticodeSignatures

When set to a non-zero value, then update binaries are also verified in
normal install / update cycles.

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

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)
 

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,

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;
 

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<const Package*>(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,

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;

omaha/goopdate/download_manager_unittest.cc

@@ -16,6 +16,7 @@
 // TODO(omaha): why so many dependencies for this unit test?
 
 #include <atlstr.h>
+#include <vector>
 #include <windows.h>
 
 #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<byte> 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<DownloadManager> 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));

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));

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,

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'])