Extend Authenticode signature verification

omaha/base/constants.h

@@ -281,6 +281,14 @@ const TCHAR* const kRegValueAutoUpdateJitterMs      = _T("AutoUpdateJitterMs");
 const TCHAR* const kRegValueProxyHost               = _T("ProxyHost");
 const TCHAR* const kRegValueProxyPort               = _T("ProxyPort");
 const TCHAR* const kRegValueMID                     = _T("mid");
+const TCHAR* const kRegValueAlwaysVerifyAuthenticodeSignatures =
+    _T("AlwaysVerifyAuthenticodeSignatures");
+
+// If AlwaysVerifyAuthenticodeSignatures is non-zero in registry (see above),
+// then update binaries with the following extensions are Authenticode-verified:
+const TCHAR* const kAuthenticodeSignedExtensions =
+    _T("exe msi dll sys cab ocx xpi xap cat jar ps1 psm1 psd1 ps1xml psc1 acm ")
+    _T("ax cpl drv efi mui scr sys tsp");
 
 #if defined(HAS_DEVICE_MANAGEMENT)
 const TCHAR* const kRegValueNameDeviceManagementUrl = _T("DeviceManagementUrl");

omaha/common/config_manager.cc

@@ -1807,6 +1807,14 @@ bool ConfigManager::AlwaysAllowCrashUploads() const {
   return always_allow_crash_uploads != 0;
 }
 
+bool ConfigManager::AlwaysVerifyAuthenticodeSignatures() const {
+  DWORD always_verify_file_signatures = 0;
+  RegKey::GetValue(MACHINE_REG_UPDATE_DEV,
+                   kRegValueAlwaysVerifyAuthenticodeSignatures,
+                   &always_verify_file_signatures);
+  return always_verify_file_signatures != 0;
+}
+
 int ConfigManager::MaxCrashUploadsPerDay() const {
   DWORD num_uploads = 0;
   if (FAILED(RegKey::GetValue(MACHINE_REG_UPDATE_DEV,

omaha/common/config_manager.h

@@ -460,6 +460,11 @@ class ConfigManager {
   // build flavor or other configuration parameters.
   bool AlwaysAllowCrashUploads() const;
 
+  // Returns true if Authenticode signatures of update binaries should always be
+  // verified. If this is false (the default), then signatures are only verified
+  // in code red scenarios and in app commands.
+  bool AlwaysVerifyAuthenticodeSignatures() 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"
@@ -475,10 +476,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 +523,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 +533,17 @@ 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;
+  if (ConfigManager::Instance()->AlwaysVerifyAuthenticodeSignatures()) {
+    hr = EnsureSignatureIsValid(*source_file_path);
+    if (FAILED(hr)) {
+      CORE_LOG(LE, (_T("[EnsureSignatureIsValid failed][%s][0x%08x]"),
+                    package->filename(), hr));
+      return SIGS_E_INVALID_SIGNATURE;
+    }
+  }
+
+  hr = package_cache()->Put(
       key, source_file, package->expected_hash());
   if (hr != SIGS_E_INVALID_SIGNATURE) {
     if (FAILED(hr)) {
@@ -552,6 +565,22 @@ 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.
+    CString ext_lower(ext);
+    ext_lower.MakeLower();
+    const TCHAR* delim = _T(" ");
+    CString extensions = delim + CString(kAuthenticodeSignedExtensions) + delim;
+    if (extensions.Find(delim + ext_lower + delim) != -1) {
+      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.
   //
@@ -127,6 +130,7 @@ class DownloadManager : public DownloadManagerInterface {
                                    const CString& filename,
                                    Package* package,
                                    State* state);
+  HRESULT EnsureSignatureIsValid(const CString& file_path);
 
   bool is_machine() 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"
@@ -87,6 +88,9 @@ class DownloadManagerTest : public AppTestBase {
     AppTestBase::SetUp();
 
     CleanupFiles();
+    RegKey::GetValue(MACHINE_REG_UPDATE_DEV,
+                     kRegValueAlwaysVerifyAuthenticodeSignatures,
+                     &verify_file_signatures_);
 
     download_manager_.reset(new DownloadManager(is_machine_));
     EXPECT_SUCCEEDED(download_manager_->Initialize());
@@ -95,6 +99,16 @@ class DownloadManagerTest : public AppTestBase {
   virtual void TearDown() {
     download_manager_.reset();
     CleanupFiles();
+    if (verify_file_signatures_) {
+      EXPECT_SUCCEEDED(RegKey::SetValue(
+          MACHINE_REG_UPDATE_DEV,
+          kRegValueAlwaysVerifyAuthenticodeSignatures,
+          verify_file_signatures_));
+    } else {
+      EXPECT_SUCCEEDED(
+          RegKey::DeleteValue(MACHINE_REG_UPDATE_DEV,
+                              kRegValueAlwaysVerifyAuthenticodeSignatures));
+    }
 
     AppTestBase::TearDown();
   }
@@ -111,6 +125,7 @@ class DownloadManagerTest : public AppTestBase {
 
   const CString cache_path_;
   std::unique_ptr<DownloadManager> download_manager_;
+  DWORD verify_file_signatures_ = 0; // Saved from registry
 };
 
 
@@ -1177,6 +1192,53 @@ TEST_F(DownloadManagerUserTest, GetPackage) {
   EXPECT_SUCCEEDED(DeleteDirectory(dir));
 }
 
+TEST_F(DownloadManagerUserTest, CachePackage) {
+  ASSERT_SUCCEEDED(RegKey::SetValue(MACHINE_REG_UPDATE_DEV,
+                                    kRegValueAlwaysVerifyAuthenticodeSignatures,
+                                    (DWORD)1));
+
+  const TCHAR* kFiles[] = {_T("SaveArguments.exe"),
+                           _T("old_google_certificate.dll"),
+                           _T("sha2_0c15be4a15bb0903c901b1d6c265302f.msi")};
+  HRESULT kExpected[] =   {S_OK,
+                           SIGS_E_INVALID_SIGNATURE,
+                           S_OK};
+
+  ASSERT_EQ(arraysize(kFiles), arraysize(kExpected));
+
+  App* app = NULL;
+  ASSERT_SUCCEEDED(app_bundle_->createApp(CComBSTR(kAppGuid1), &app));
+
+  for (size_t i = 0; i < arraysize(kFiles); ++i) {
+    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), kFiles[i]));
+    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());
+
+    ASSERT_SUCCEEDED(app->next_version()->AddPackage(kFiles[i],
+                                                     file_size,
+                                                     CString(hash.c_str())));
+
+    Package* package = app->next_version()->GetPackage(i);
+    hr = download_manager_->CachePackage(package, &file, &file_path);
+    EXPECT_EQ(kExpected[i], hr) << kFiles[i];
+  }
+}
+
 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,