Skip to content

Commit

Permalink
Enable payload Authenticode verification
Browse files Browse the repository at this point in the history 
Previously, it was disabled and could be enabled by a registry key.
Now, it is the other way around.
  • Loading branch information
@mherrmann
mherrmann committed Aug 5, 2021
1 parent 14994ac commit f64c29a
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 64 deletions.
8 changes: 4 additions & 4 deletions omaha/base/constants.h
View file
Expand Up @@ -281,11 +281,11 @@ 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 kRegValueVerifyPayloadAuthenticodeSignature =
_T("VerifyPayloadAuthenticodeSignature");
const TCHAR* const kRegValueDisablePayloadAuthenticodeVerification =
_T("DisablePayloadAuthenticodeVerification");

// If VerifyPayloadAuthenticodeSignature is non-zero in registry (see above),
// then update binaries with the following extensions are Authenticode-verified:
// Update binaries with the following extensions are Authenticode-verified
// unless DisablePayloadAuthenticodeVerification above is non-zero in registry.
const TCHAR* const kAuthenticodeSignedExtensions[] = {
_T("exe"), _T("msi"), _T("dll"), _T("sys"), _T("cab"), _T("ocx"),
_T("xpi"), _T("xap"), _T("cat"), _T("jar"), _T("ps1"), _T("psm1"),
Expand Down
8 changes: 4 additions & 4 deletions omaha/common/config_manager.cc
View file
Expand Up @@ -1808,11 +1808,11 @@ bool ConfigManager::AlwaysAllowCrashUploads() const {
}

bool ConfigManager::VerifyPayloadAuthenticodeSignature() const {
DWORD result_from_registry = 0;
DWORD disabled_in_registry = 0;
RegKey::GetValue(MACHINE_REG_UPDATE_DEV,
kRegValueVerifyPayloadAuthenticodeSignature,
&result_from_registry);
return result_from_registry != 0;
kRegValueDisablePayloadAuthenticodeVerification,
&disabled_in_registry);
return disabled_in_registry == 0;
}

int ConfigManager::MaxCrashUploadsPerDay() const {
Expand Down
5 changes: 2 additions & 3 deletions omaha/common/config_manager.h
View file
Expand Up @@ -460,9 +460,8 @@ 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.
// Returns whether the Authenticode signature of update payloads should be
// verified.
bool VerifyPayloadAuthenticodeSignature() const;

// Returns the number of crashes to upload per day.
Expand Down
117 changes: 64 additions & 53 deletions omaha/goopdate/download_manager_unittest.cc
View file
Expand Up @@ -89,8 +89,14 @@ class DownloadManagerTest : public AppTestBase {

CleanupFiles();
RegKey::GetValue(MACHINE_REG_UPDATE_DEV,
kRegValueVerifyPayloadAuthenticodeSignature,
&verify_file_signatures_);
kRegValueDisablePayloadAuthenticodeVerification,
&disable_payload_authenticode_verification_);
if (disable_payload_authenticode_verification_) {
// Make sure Payload verification is enabled (the default).
EXPECT_SUCCEEDED(RegKey::DeleteValue(
MACHINE_REG_UPDATE_DEV,
kRegValueDisablePayloadAuthenticodeVerification));
}

download_manager_.reset(new DownloadManager(is_machine_));
EXPECT_SUCCEEDED(download_manager_->Initialize());
Expand All @@ -99,22 +105,55 @@ class DownloadManagerTest : public AppTestBase {
virtual void TearDown() {
download_manager_.reset();
CleanupFiles();
if (verify_file_signatures_) {
if (disable_payload_authenticode_verification_) {
EXPECT_SUCCEEDED(RegKey::SetValue(
MACHINE_REG_UPDATE_DEV,
kRegValueVerifyPayloadAuthenticodeSignature,
verify_file_signatures_));
kRegValueDisablePayloadAuthenticodeVerification,
disable_payload_authenticode_verification_));
} else {
EXPECT_SUCCEEDED(
RegKey::DeleteValue(MACHINE_REG_UPDATE_DEV,
kRegValueVerifyPayloadAuthenticodeSignature));
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);
}
Expand All @@ -125,7 +164,7 @@ class DownloadManagerTest : public AppTestBase {

const CString cache_path_;
std::unique_ptr<DownloadManager> download_manager_;
DWORD verify_file_signatures_ = 0; // Saved from registry
DWORD disable_payload_authenticode_verification_ = 0; // Saved from registry
};


Expand Down Expand Up @@ -1193,53 +1232,25 @@ TEST_F(DownloadManagerUserTest, GetPackage) {
}

TEST_F(DownloadManagerUserTest, CachePackage) {
ASSERT_SUCCEEDED(RegKey::SetValue(MACHINE_REG_UPDATE_DEV,
kRegValueVerifyPayloadAuthenticodeSignature,
(DWORD)1));

const TCHAR* kFiles[] = {_T("SaveArguments.exe"),
_T("old_google_certificate.dll"),
_T("sha2_0c15be4a15bb0903c901b1d6c265302f.msi"),
// Ensure unexpected extensions don't crash:
_T("declaration.txt")};
HRESULT kExpected[] = {S_OK,
GOOPDATEDOWNLOAD_E_AUTHENTICODE_VERIFICATION_FAILED,
S_OK,
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];
}
TestCachePackage(app, _T("SaveArguments.exe"), S_OK);
const TCHAR* kFileWithOldCertificate = _T("old_google_certificate.dll");
TestCachePackage(app,
kFileWithOldCertificate,
GOOPDATEDOWNLOAD_E_AUTHENTICODE_VERIFICATION_FAILED);
TestCachePackage(app, _T("sha2_0c15be4a15bb0903c901b1d6c265302f.msi"), S_OK);

// Make sure that unexpected file extensions are handled gracefully:
TestCachePackage(app, _T("declaration.txt"), S_OK);

// Test that disabling verification makes a previously failing file succeed:
EXPECT_SUCCEEDED(RegKey::SetValue(
MACHINE_REG_UPDATE_DEV,
kRegValueDisablePayloadAuthenticodeVerification,
1UL));
TestCachePackage(app, kFileWithOldCertificate, S_OK);
}

TEST_F(DownloadManagerUserTest, GetPackage_NotPresent) {
Expand Down

0 comments on commit f64c29a

Please sign in to comment.