Skip to content

Commit

Permalink
Fix broken unit test CertInfoTest.CertInfo.
Browse files Browse the repository at this point in the history
The SHA1 codesigning certificate for Google has changed.

SHA1 code signing is needed for XP support (which Google Update needs)
but it is not needed for the open source upstream. However, code
changes were needed to support a SHA1 certificate with a different
subject name (Google Inc vs. Google LLC).
  • Loading branch information
sorinj committed Dec 6, 2019
1 parent c0ebc40 commit e50f101
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 51 deletions.
13 changes: 10 additions & 3 deletions omaha/base/const_code_signing.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,19 @@ namespace omaha {

// The company and organization names expected in the code
// signing certificates which are trusted.
const TCHAR* const kCertificateSubjectName = _T("Google Inc");
const TCHAR* const kLegacyCertificateSubjectName = _T("Google Inc");
const TCHAR* const kSha1CertificateSubjectName = _T("Google LLC");
const TCHAR* const kSha256CertificateSubjectName = _T("Google LLC");

// The Omaha certificate thumbprint. Used by unit tests.
const TCHAR* const kCertificateThumbprint =
_T("1a6ac0549a4a44264deb6ff003391da2f285b19f");
_T("a3958ae522f3c54b878b20d7b0f63711e08666b2");
const TCHAR* const kSha256CertificateThumbprint =
_T("cb7e84887f3c6015fe7edfb4f8f36df7dc10590e");

// The SHA256 hash of the Omaha certificate RSA public key.
const TCHAR* const kCertificatePublicKeyHash =
_T("d49de35a2e9fdbed09e2b9a6c1243df414d6aac13690ab221b0017a5cbe1351f");
_T("6cb128676c6d0b49d3e8918bd835888694333da7540a0994261c0ec0b3516f9d");
const TCHAR* const kSha256CertificatePublicKeyHash =
_T("03e27c19d222043a8f0c64181c23c9339cc84a7ec4ebff8a19adb7caefb0c709");

Expand Down Expand Up @@ -81,6 +82,12 @@ _T("cd623b2bf2c06940bd480b6bcf4a5c9e1cbe94626fbfa127d001bf19ae5ba9fe"),
// thumbprint=1a6ac0549a4a44264deb6ff003391da2f285b19f
// serial=14F8FDD167F92402B1570B5DC495C815
// SHA1 Fingerprint=1A:6A:C0:54:9A:4A:44:26:4D:EB:6F:F0:03:39:1D:A2:F2:85:B1:9F
_T("d49de35a2e9fdbed09e2b9a6c1243df414d6aac13690ab221b0017a5cbe1351f"),

// Omaha certificate: sha1 (11/07/2019 to 11/16/2022).
// thumbprint=a3958ae522f3c54b878b20d7b0f63711e08666b2
// serial=06aea76bac46a9e8cfe6d29e45aaf033
// SHA1 Fingerprint=A3:95:8A:E5:22:F3:C5:4B:87:8B:20:D7:B0:F6:37:11:E0:86:66:B2
kCertificatePublicKeyHash,

// Omaha and Chrome certificate: sha256 (11/06/2018 to 11/17/2021).
Expand Down
10 changes: 1 addition & 9 deletions omaha/base/signaturevalidator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,6 @@ void CertList::FindFirstCert(const CertInfo** result_cert_info,
const std::vector<CString>& company_name_to_match,
const CString &orgn_unit_to_match,
const CString &trust_authority_to_match,
bool allow_test_variant,
bool check_cert_is_valid_now) const {
if (!result_cert_info) {
return;
Expand All @@ -362,17 +361,12 @@ void CertList::FindFirstCert(const CertInfo** result_cert_info,
cert_iter != cert_list_.end();
++cert_iter) {
// Find a match between the name of the certificate and one of the
// company names provided as a paramer..
// company names provided as a parameter.
bool names_match = false;
for (size_t i = 0; i != company_name_to_match.size(); ++i) {
const TCHAR* certificate_company_name =
(*cert_iter)->issuing_company_name_;
names_match = company_name_to_match[i] == certificate_company_name;
if (!names_match && allow_test_variant) {
CString test_variant = company_name_to_match[i];
test_variant += _T(" (TEST)");
names_match = test_variant == certificate_company_name;
}
if (names_match) {
break;
}
Expand Down Expand Up @@ -440,7 +434,6 @@ void ExtractAllCertificatesFromSignature(const wchar_t* signed_file,
// VerifyAuthenticodeSignature that adds WTD_LIFETIME_SIGNING_FLAG.
HRESULT VerifyCertificate(const wchar_t* signed_file,
const std::vector<CString>& subject,
bool allow_test_variant,
bool check_cert_is_valid_now,
const std::vector<CString>* expected_hashes) {
CertList cert_list;
Expand All @@ -454,7 +447,6 @@ HRESULT VerifyCertificate(const wchar_t* signed_file,
subject,
CString(),
CString(),
allow_test_variant,
check_cert_is_valid_now);
if (required_cert == NULL) {
return GOOPDATE_E_SIGNATURE_NOT_TRUSTED_SUBJECT;
Expand Down
8 changes: 2 additions & 6 deletions omaha/base/signaturevalidator.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,11 @@ class CertList {
}

// FindFirstCert() finds the first certificate that exactly matches the given
// criteria. If allow_test_variant is true, the company name will also be
// deemed valid if it equals company_name_to_match + " (TEST)".
// criteria.
void FindFirstCert(const CertInfo** result_cert_info,
const std::vector<CString>& company_name_to_match,
const CString &orgn_unit_to_match,
const CString &trust_authority_to_match,
bool allow_test_variant,
bool check_cert_is_valid_now) const;

typedef std::vector<CertInfo*> CertInfoList;
Expand All @@ -189,8 +187,7 @@ void ExtractAllCertificatesFromSignature(const wchar_t* signed_file,
CertList* cert_list);

// Returns true if the subject of the certificate exactly matches the first CN
// name. If the 'allow_test_variant' parameter is true, the function tries to
// match the "subject (TEST)" string.
// name.
//
// The function enforces an additional check against the public key of the
// certificate. Pinning to specific public keys mitigates the risk of accepting
Expand All @@ -201,7 +198,6 @@ void ExtractAllCertificatesFromSignature(const wchar_t* signed_file,
// call.
HRESULT VerifyCertificate(const wchar_t* signed_file,
const std::vector<CString>& subject,
bool allow_test_variant,
bool check_cert_is_valid_now,
const std::vector<CString>* expected_hashes);

Expand Down
70 changes: 41 additions & 29 deletions omaha/base/signaturevalidator_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,58 +27,61 @@ namespace omaha {

namespace {

bool VerifySigneeIsGoogle(const wchar_t* signed_file) {
const TCHAR* const kTestCertificateSubjectName = _T("Google Inc (TEST)");

bool VerifySigneeIs(const wchar_t* subject_name,
const wchar_t* signed_file) {
std::vector<CString> subject;
subject.push_back(kSha256CertificateSubjectName);
subject.push_back(kCertificateSubjectName);
subject.push_back(subject_name);
return SUCCEEDED(
VerifyCertificate(signed_file,
subject,
true, // Allow test variant.
false, // Check certificate is valid now.
NULL));
}

} // namespace

// Checks Omaha Thawte certificate sha1 (11/28/2016 to 11/21/2019).
// Checks Omaha certificate sha1 (11/07/2019 to 11/16/2022).
TEST(CertInfoTest, CertInfo) {
const TCHAR kRelativePath[] =
_T("unittest_support\\sha1_14F8FDD167F92402B1570B5DC495C815.sys");
_T("unittest_support\\sha1_06aea76bac46a9e8cfe6d29e45aaf033.sys");

CString executable_full_path(app_util::GetCurrentModuleDirectory());
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kSha1CertificateSubjectName,
executable_full_path));

CertList cert_list;
ExtractAllCertificatesFromSignature(executable_full_path, &cert_list);

// ExtractAllCertificatesFromSignature() gets the certificate chain for the
// first signature and the certificate chain for the corresponding timestamp,
// excluding the root certificates.
// The following certificates are enumerated from SaveArguments.exe signed
// Thursday, April 14, 2016 3:57:37 PM:
// * "Google Inc" hash 1a6ac0549a4a44264deb6ff003391da2f285b19f.
// * "Thawte Code Signing CA - G2" hash
// 808d62642b7d1c4a9a83fd667f7a2a9d243fb1c7.
// * "COMODO SHA-1 Time Stamping Signer" hash
// 03a5b14663eb12023091b84a6d6a68bc871de66b.
EXPECT_EQ(3, cert_list.size());
// The following certificates are enumerated from GoogleUpdate.exe signed
// Friday, November 15, 2019 4:04:23 PM:
// * "DigiCert Assured ID CA-1" hash
// 19a09b5a36f4dd99727df783c17a51231a56c117.
// * "DigiCert Assured ID Code Signing CA-1" hash
// 409aa4a74a0cda7c0fee6bd0bb8823d16b5f1875.
// * "DigiCert Timestamp Responder" hash
// 614d271d9102e30169822487fde5de00a352b01d.
// * "Google LLC" hash a3958ae522f3c54b878b20d7b0f63711e08666b2.
EXPECT_EQ(4, cert_list.size());

const CertInfo* cert_info = NULL;
std::vector<CString> subject;
subject.push_back(kCertificateSubjectName);
subject.push_back(kSha1CertificateSubjectName);
cert_list.FindFirstCert(&cert_info,
subject,
CString(),
CString(),
false, // Do not allow test variant.
true); // Check if the certificate is valid now.
ASSERT_TRUE(cert_info);

EXPECT_STREQ(kCertificateSubjectName, cert_info->issuing_company_name_);
EXPECT_STREQ(kSha1CertificateSubjectName, cert_info->issuing_company_name_);
EXPECT_STREQ(kCertificateThumbprint, cert_info->thumbprint_);
EXPECT_STREQ(kCertificatePublicKeyHash, cert_info->public_key_hash_);
}
Expand Down Expand Up @@ -112,7 +115,6 @@ TEST(CertInfoTest, CertInfo_Sha256) {
subject,
CString(),
CString(),
false, // Do not allow test variant.
true); // Check if the certificate is valid now.
ASSERT_TRUE(cert_info);

Expand All @@ -128,7 +130,8 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_OfficiallySigned) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kSha1CertificateSubjectName,
executable_full_path));
}

// Tests a certificate subject containing multiple CNs such as:
Expand All @@ -142,7 +145,8 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_TestSigned_MultipleCN) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kTestCertificateSubjectName,
executable_full_path));
}

TEST(SignatureValidatorTest,
Expand All @@ -154,7 +158,8 @@ TEST(SignatureValidatorTest,
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kTestCertificateSubjectName,
executable_full_path));
}

TEST(SignatureValidatorTest, VerifySigneeIsGoogle_OmahaTestSigned) {
Expand All @@ -165,7 +170,8 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_OmahaTestSigned) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kTestCertificateSubjectName,
executable_full_path));
}

TEST(SignatureValidatorTest, VerifySigneeIsGoogle_Sha256) {
Expand All @@ -176,13 +182,15 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_Sha256) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kLegacyCertificateSubjectName,
executable_full_path));

executable_full_path = app_util::GetCurrentModuleDirectory();
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
_T("unittest_support\\chrome_setup.exe")));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kSha256CertificateSubjectName,
executable_full_path));
}

TEST(SignatureValidatorTest, VerifySigneeIsGoogle_DualSigned_Sha1AndSha256) {
Expand All @@ -192,7 +200,8 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_DualSigned_Sha1AndSha256) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kLegacyCertificateSubjectName,
executable_full_path));
}

// The certificate was valid when it was used to sign the executable, but it has
Expand All @@ -205,7 +214,8 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_SignedWithNowExpiredCert) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_TRUE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_TRUE(VerifySigneeIs(kLegacyCertificateSubjectName,
executable_full_path));
}

TEST(SignatureValidatorTest, VerifySigneeIsGoogle_TestSigned_NoCN) {
Expand All @@ -216,7 +226,8 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_TestSigned_NoCN) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_FALSE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_FALSE(VerifySigneeIs(kLegacyCertificateSubjectName,
executable_full_path));
}

TEST(SignatureValidatorTest, VerifySigneeIsGoogle_TestSigned_WrongCN) {
Expand All @@ -227,7 +238,8 @@ TEST(SignatureValidatorTest, VerifySigneeIsGoogle_TestSigned_WrongCN) {
ASSERT_TRUE(::PathAppend(CStrBuf(executable_full_path, MAX_PATH),
kRelativePath));
ASSERT_TRUE(File::Exists(executable_full_path));
EXPECT_FALSE(VerifySigneeIsGoogle(executable_full_path));
EXPECT_FALSE(VerifySigneeIs(kLegacyCertificateSubjectName,
executable_full_path));
}

TEST(SignatureValidatorTest, VerifyAuthenticodeSignature) {
Expand Down
5 changes: 2 additions & 3 deletions omaha/common/google_signaturevalidator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,12 @@ HRESULT VerifyGoogleAuthenticodeSignature(const CString& filename,

std::vector<CString> subject;
subject.push_back(kSha256CertificateSubjectName);
subject.push_back(kCertificateSubjectName);
subject.push_back(kSha1CertificateSubjectName);
subject.push_back(kLegacyCertificateSubjectName);

const bool allow_test_variant = false;
const bool check_cert_is_valid_now = false;
hr = VerifyCertificate(filename,
subject,
allow_test_variant,
check_cert_is_valid_now,
expected_hashes.empty() ? NULL : &expected_hashes);
if (FAILED(hr)) {
Expand Down
3 changes: 2 additions & 1 deletion omaha/common/omaha_customization_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ TEST(OmahaCustomizationTest, Constants_Filenames) {
}

TEST(OmahaCustomizationTest, Constants_Certificate) {
EXPECT_STREQ(_T("Google Inc"), kCertificateSubjectName);
EXPECT_STREQ(_T("Google LLC"), kSha1CertificateSubjectName);
EXPECT_STREQ(_T("Google LLC"), kSha256CertificateSubjectName);
}

TEST(OmahaCustomizationTest, Constants_OmahaAppId_String) {
Expand Down
4 changes: 4 additions & 0 deletions omaha/testing/build.scons
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ unittest_support = env.Replicate('$STAGING_DIR/unittest_support/', [
# serial=14F8FDD167F92402B1570B5DC495C815
'unittest_support/sha1_14F8FDD167F92402B1570B5DC495C815.sys',

# Omaha certificate: sha1 (11/07/2019 to 11/16/2022).
# serial=06aea76bac46a9e8cfe6d29e45aaf033
'unittest_support/sha1_06aea76bac46a9e8cfe6d29e45aaf033.sys',

'unittest_support/Sha1_4c40dba5f988fae57a57d6457495f98b_and_sha2_2a9c21acaaa63a3c58a7b9322bee948d.exe',

# Omaha and Chrome certificate: sha256 (12/15/2015 to 12/16/2018).
Expand Down
Binary file modified omaha/testing/unittest_support/SaveArguments.exe
Binary file not shown.
Binary file not shown.

0 comments on commit e50f101

Please sign in to comment.