diff --git a/components/policy/core/common/cloud/cloud_policy_validator.cc b/components/policy/core/common/cloud/cloud_policy_validator.cc index 6a5e1a622bd0e..f41974db1e593 100644 --- a/components/policy/core/common/cloud/cloud_policy_validator.cc +++ b/components/policy/core/common/cloud/cloud_policy_validator.cc @@ -454,8 +454,9 @@ bool CloudPolicyValidatorBase::CheckNewPublicKeyVerificationSignature() { verification_key_.value(), policy_->new_public_key_verification_data_signature(), em::PolicyFetchRequest::SHA256_RSA) && - CheckDomainInPublicKeyVerificationData( - policy_->new_public_key_verification_data())) { + CheckPublicKeyVerificationData( + policy_->new_public_key_verification_data(), + policy_->new_public_key())) { UMA_HISTOGRAM_ENUMERATION(kMetricKeySignatureVerification, MetricKeySignatureVerification::kSuccess); // Signature verification succeeded - return success to the caller. @@ -542,8 +543,9 @@ std::string CloudPolicyValidatorBase::ExtractDomainFromPolicy() { return domain; } -bool CloudPolicyValidatorBase::CheckDomainInPublicKeyVerificationData( - const std::string& new_public_key_verification_data) { +bool CloudPolicyValidatorBase::CheckPublicKeyVerificationData( + const std::string& new_public_key_verification_data, + const std::string& expected_public_key) { em::PublicKeyVerificationData public_key_data; if (!public_key_data.ParseFromString(new_public_key_verification_data)) { LOG_POLICY(ERROR, POLICY_FETCHING) @@ -551,6 +553,12 @@ bool CloudPolicyValidatorBase::CheckDomainInPublicKeyVerificationData( << "Failed to deserialize new public key."; return false; } + if (public_key_data.new_public_key() != expected_public_key) { + LOG_POLICY(ERROR, POLICY_FETCHING) + << PolicyTypeLogPrefix(policy_type_, settings_entity_id_) + << "Key mismatch in new public key verification data."; + return false; + } if (public_key_data.domain() != ExtractDomainFromPolicy()) { LOG_POLICY(ERROR, POLICY_FETCHING) << PolicyTypeLogPrefix(policy_type_, settings_entity_id_) @@ -631,7 +639,7 @@ CloudPolicyValidatorBase::Status CloudPolicyValidatorBase::CheckCachedKey() { if (VerifySignature(new_cached_key_, verification_key_.value(), new_cached_key_signature_, em::PolicyFetchRequest::SHA256_RSA) && - CheckDomainInPublicKeyVerificationData(new_cached_key_)) { + CheckPublicKeyVerificationData(new_cached_key_, cached_key_)) { UMA_HISTOGRAM_ENUMERATION(kMetricKeySignatureVerification, MetricKeySignatureVerification::kSuccess); // Signature verification succeeded - return success to the caller. diff --git a/components/policy/core/common/cloud/cloud_policy_validator.h b/components/policy/core/common/cloud/cloud_policy_validator.h index fabba760bb620..653528a9cf80d 100644 --- a/components/policy/core/common/cloud/cloud_policy_validator.h +++ b/components/policy/core/common/cloud/cloud_policy_validator.h @@ -354,10 +354,12 @@ class POLICY_EXPORT CloudPolicyValidatorBase { // empty string if the policy does not contain a username field. std::string ExtractDomainFromPolicy(); - // Returns if the domain from the new_public_key_verification_data matches - // the domain extracted from the |policy_|. - bool CheckDomainInPublicKeyVerificationData( - const std::string& new_public_key_verification_data); + // Returns true if |new_public_key_verification_data| can be parsed, the + // public key it certifies equals |expected_public_key| and its domain + // matches the domain extracted from |policy_|. + bool CheckPublicKeyVerificationData( + const std::string& new_public_key_verification_data, + const std::string& expected_public_key); // Sets the owning domain used to verify new public keys, and ensures that // callers don't try to set conflicting values. diff --git a/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc b/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc index 8cc7185f27f00..6cad71856c886 100644 --- a/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc +++ b/components/policy/core/common/cloud/cloud_policy_validator_unittest.cc @@ -544,6 +544,36 @@ TEST_F(CloudPolicyValidatorTest, GoodNewSignatureEmptyDeprecatedSignature) { policy_.GetCopy()); } +TEST_F(CloudPolicyValidatorTest, ErrorVerificationDataKeyMismatch) { + // Build a response whose new_public_key_verification_data certifies the + // default new signing key. + policy_.Build(); + const std::string verification_data = + policy_.policy().new_public_key_verification_data(); + const std::string verification_data_signature = + policy_.policy().new_public_key_verification_data_signature(); + + // Build a second response that delivers an unrelated new_public_key and + // signs policy_data with it, then attach the verification data from the + // first response. + UserPolicyBuilder other; + other.SetDefaultInitialSigningKey(); + other.Build(); + ASSERT_NE(other.policy().new_public_key(), policy_.policy().new_public_key()); + other.policy().set_new_public_key_verification_data(verification_data); + other.policy().set_new_public_key_verification_data_signature( + verification_data_signature); + other.policy().clear_new_public_key_verification_signature_deprecated(); + + auto validator = std::make_unique( + other.GetCopy(), base::SingleThreadTaskRunner::GetCurrentDefault()); + validator->ValidateInitialKey(PolicyBuilder::kFakeDomain); + validator->RunValidation(); + EXPECT_EQ( + CloudPolicyValidatorBase::VALIDATION_BAD_KEY_VERIFICATION_SIGNATURE, + validator->status()); +} + TEST_F(CloudPolicyValidatorTest, ErrorDomainMismatchForKeyVerification) { policy_.Build(); policy_.policy().set_new_public_key_verification_data("invalid");