Chrome · Sync
CVE-2026-79076
Logic Error in Sync
Overview
Medium
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Changed Functions
| Function | Change | Notes |
|---|---|---|
ifcomponents/trusted_vault/download_keys_response_handler.cc |
modified | |
DownloadKeysResponseHandlerTestcomponents/trusted_vault/download_keys_response_handler_unittest.cc |
modified | |
TEST_Pcomponents/trusted_vault/download_keys_response_handler_unittest.cc |
modified | |
TEST_Fcomponents/trusted_vault/download_keys_response_handler_unittest.cc |
modified |
Files Changed
components/trusted_vault/download_keys_response_handler.cccomponents/trusted_vault/download_keys_response_handler_unittest.cc
Patch
From a63b8a215549dc9dc196c2e33fb2c047d98ec21c Mon Sep 17 00:00:00 2001 From: Thomas Thrainer <[email protected]> Date: Wed, 22 Jul 2026 03:35:09 -0700 Subject: [PATCH] [TrustedVault] Clean up kE2eeRotationProofBypassFix feature flag Remove the fully rolled out `kE2eeRotationProofBypassFix` flag and inline changes to `DownloadKeysResponseHandler`. Fixed: 496395158 Test: components_unittests --gtest_filter=DownloadKeysResponseHandlerTest.* Change-Id: Ic6763ba271c297a97eba0627563b6ce46a6a6964 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8061799 Commit-Queue: Thomas Thrainer <[email protected]> Reviewed-by: Adem Derinel <[email protected]> Cr-Commit-Position: refs/heads/main@{#1666153} --- diff --git a/components/trusted_vault/download_keys_response_handler.cc b/components/trusted_vault/download_keys_response_handler.cc index a16191b4..936023e0 100644 --- a/components/trusted_vault/download_keys_response_handler.cc +++ b/components/trusted_vault/download_keys_response_handler.cc @@ -10,7 +10,6 @@ #include <utility> #include "base/check.h" -#include "components/trusted_vault/features.h" #include "components/trusted_vault/proto/vault.pb.h" #include "components/trusted_vault/proto_string_bytes_conversion.h" #include "components/trusted_vault/securebox.h" @@ -92,10 +91,8 @@ CHECK(!key_chain.empty()); CHECK(std::ranges::is_sorted(key_chain, std::less<>(), &ExtractedSharedKey::version)); - if (base::FeatureList::IsEnabled(kE2eeRotationProofBypassFix)) { - CHECK(key_chain.back().version > - last_known_trusted_vault_key_and_version.version); - } + CHECK(key_chain.back().version > + last_known_trusted_vault_key_and_version.version); int last_valid_key_version = last_known_trusted_vault_key_and_version.version; std::vector<uint8_t> last_valid_key = @@ -222,46 +219,34 @@ return ProcessedResponse( /*status=*/TrustedVaultDownloadKeysStatus::kMembershipEmpty); } - if (base::FeatureList::IsEnabled(kE2eeRotationProofBypassFix)) { - if (extracted_keys->back().version < - last_trusted_vault_key_and_version_.version) { - // |current_member| appears corrupt, as its keys have a lower version than - // the last currently trusted one. - return ProcessedResponse( - /*status=*/TrustedVaultDownloadKeysStatus::kMembershipCorrupted); - } + if (extracted_keys->back().version < + last_trusted_vault_key_and_version_.version) { + // |current_member| appears corrupt, as its keys have a lower version than + // the last currently trusted one. + return ProcessedResponse( + /*status=*/TrustedVaultDownloadKeysStatus::kMembershipCorrupted); + } - if (auto it = std::ranges::find(*extracted_keys, - last_trusted_vault_key_and_version_.version, - &ExtractedSharedKey::version); - it != extracted_keys->end() && - it->trusted_vault_key != last_trusted_vault_key_and_version_.key) { - // |current_member| appears corrupt, as its key with the version of the - // last trusted vault key doesn't match the key bytes of the last - // trusted vault key. - return ProcessedResponse( - /*status=*/TrustedVaultDownloadKeysStatus::kMembershipCorrupted); - } + if (auto it = std::ranges::find(*extracted_keys, + last_trusted_vault_key_and_version_.version, + &ExtractedSharedKey::version); + it != extracted_keys->end() && + it->trusted_vault_key != last_trusted_vault_key_and_version_.key) { + // |current_member| appears corrupt, as its key with the version of the + // last trusted vault key doesn't match the key bytes of the last + // trusted vault key. + return ProcessedResponse( + /*status=*/TrustedVaultDownloadKeysStatus::kMembershipCorrupted); + } - if (extracted_keys->back().version > - last_trusted_vault_key_and_version_.version && - !IsValidKeyChain(*extracted_keys, - last_trusted_vault_key_and_version_)) { - // New keys provided by |current_member| failed rotation proof - // verification. - return ProcessedResponse( - /*status=*/TrustedVaultDownloadKeysStatus:: - kKeyProofsVerificationFailed); - } - } else { - if (!IsValidKeyChain(*extracted_keys, - last_trusted_vault_key_and_version_)) { - // Data corresponding to |current_member| is corrupted or - // |last_trusted_vault_key_and_version_| is too old. - return ProcessedResponse( - /*status=*/TrustedVaultDownloadKeysStatus:: - kKeyProofsVerificationFailed); - } + if (extracted_keys->back().version > + last_trusted_vault_key_and_version_.version && + !IsValidKeyChain(*extracted_keys, last_trusted_vault_key_and_version_)) { + // New keys provided by |current_member| failed rotation proof + // verification. + return ProcessedResponse( + /*status=*/TrustedVaultDownloadKeysStatus:: + kKeyProofsVerificationFailed); } std::vector<std::vector<uint8_t>> trusted_vault_keys; @@ -271,19 +256,8 @@ TrustedVaultDownloadKeysStatus status = TrustedVaultDownloadKeysStatus::kSuccess; - const bool is_no_new_keys = - base::FeatureList::IsEnabled(kE2eeRotationProofBypassFix) - ? (extracted_keys->back().version == - last_trusted_vault_key_and_version_.version) - : (extracted_keys->back().version <= - last_trusted_vault_key_and_version_.version); - if (is_no_new_keys) { - // In theory, the check when the feature flag is disabled could be == - // instead of <=, since server version should not decrease, but it was - // tolerated to make the legacy implementation more robust. When the - // `kE2eeRotationProofBypassFix` flag is enabled, strictly lower version - // cases are already rejected in the conditions above, so checking for - // strict equality here is sufficient and more consistent. + if (extracted_keys->back().version == + last_trusted_vault_key_and_version_.version) { status = TrustedVaultDownloadKeysStatus::kNoNewKeys; } diff --git a/components/trusted_vault/download_keys_response_handler_unittest.cc b/components/trusted_vault/download_keys_response_handler_unittest.cc index e542164..2470cc9 100644 --- a/components/trusted_vault/download_keys_response_handler_unittest.cc +++ b/components/trusted_vault/download_keys_response_handler_unittest.cc @@ -7,8 +7,6 @@ #include <vector> #include "base/strings/string_number_conversions.h" -#include "base/test/scoped_feature_list.h" -#include "components/trusted_vault/features.h" #include "components/trusted_vault/proto/vault.pb.h" #include "components/trusted_vault/proto_string_bytes_conversion.h" #include "components/trusted_vault/securebox.h" @@ -82,19 +80,13 @@ return member.SerializeAsString(); } -class DownloadKeysResponseHandlerTest : public testing::TestWithParam<bool> { +class DownloadKeysResponseHandlerTest : public testing::Test { public: DownloadKeysResponseHandlerTest() : handler_(SecurityDomainId::kChromeSync, TrustedVaultKeyAndVersion(kKnownTrustedVaultKey, kKnownTrustedVaultKeyVersion), - MakeTestKeyPair()) { - if (GetParam()) { - scoped_feature_list_.InitAndEnableFeature(kE2eeRotationProofBypassFix); - } else { - scoped_feature_list_.InitAndDisableFeature(kE2eeRotationProofBypassFix); - } - } + MakeTestKeyPair()) {} ~DownloadKeysResponseHandlerTest() override = default; @@ -107,15 +99,12 @@ const std::vector<uint8_t> kTrustedVaultKey3 = {1, 2, 3, 7}; private: - base::test::ScopedFeatureList scoped_feature_list_; const DownloadKeysResponseHandler handler_; }; -INSTANTIATE_TEST_SUITE_P(All, DownloadKeysResponseHandlerTest, testing::Bool()); - // All HttpStatuses except kSuccess should end up in kOtherError, kNetworkError // or kMemberNotFound reporting. -TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleHttpErrors) { +TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleHttpErrors) { EXPECT_THAT( handler() .ProcessResponse( @@ -155,7 +144,7 @@ // Simplest legitimate case of key rotation, server side state corresponds to // kKnownTrustedVaultKey -> kTrustedVaultKey1 key chain. -TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleSingleKeyRotation) { +TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleSingleKeyRotation) { const DownloadKeysResponseHandler::ProcessedResponse processed_response = handler().ProcessResponse( /*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess, @@ -176,7 +165,7 @@
Loading diff…
Regression Test / PoC
shipped with the fix
diff --git a/components/trusted_vault/download_keys_response_handler_unittest.cc b/components/trusted_vault/download_keys_response_handler_unittest.cc
index e542164..2470cc9 100644
--- a/components/trusted_vault/download_keys_response_handler_unittest.cc
+++ b/components/trusted_vault/download_keys_response_handler_unittest.cc
@@ -7,8 +7,6 @@
#include <vector>
#include "base/strings/string_number_conversions.h"
-#include "base/test/scoped_feature_list.h"
-#include "components/trusted_vault/features.h"
#include "components/trusted_vault/proto/vault.pb.h"
#include "components/trusted_vault/proto_string_bytes_conversion.h"
#include "components/trusted_vault/securebox.h"
@@ -82,19 +80,13 @@
return member.SerializeAsString();
}
-class DownloadKeysResponseHandlerTest : public testing::TestWithParam<bool> {
+class DownloadKeysResponseHandlerTest : public testing::Test {
public:
DownloadKeysResponseHandlerTest()
: handler_(SecurityDomainId::kChromeSync,
TrustedVaultKeyAndVersion(kKnownTrustedVaultKey,
kKnownTrustedVaultKeyVersion),
- MakeTestKeyPair()) {
- if (GetParam()) {
- scoped_feature_list_.InitAndEnableFeature(kE2eeRotationProofBypassFix);
- } else {
- scoped_feature_list_.InitAndDisableFeature(kE2eeRotationProofBypassFix);
- }
- }
+ MakeTestKeyPair()) {}
~DownloadKeysResponseHandlerTest() override = default;
@@ -107,15 +99,12 @@
const std::vector<uint8_t> kTrustedVaultKey3 = {1, 2, 3, 7};
private:
- base::test::ScopedFeatureList scoped_feature_list_;
const DownloadKeysResponseHandler handler_;
};
-INSTANTIATE_TEST_SUITE_P(All, DownloadKeysResponseHandlerTest, testing::Bool());
-
// All HttpStatuses except kSuccess should end up in kOtherError, kNetworkError
// or kMemberNotFound reporting.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleHttpErrors) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleHttpErrors) {
EXPECT_THAT(
handler()
.ProcessResponse(
@@ -155,7 +144,7 @@
// Simplest legitimate case of key rotation, server side state corresponds to
// kKnownTrustedVaultKey -> kTrustedVaultKey1 key chain.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleSingleKeyRotation) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleSingleKeyRotation) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
/*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess,
@@ -176,7 +165,7 @@
// Multiple key rotations may happen while client is offline, server-side key
// chain is kKnownTrustedVaultKey -> kTrustedVaultKey1 -> kTrustedVaultKey2.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleMultipleKeyRotations) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleMultipleKeyRotations) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
/*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess,
@@ -203,7 +192,7 @@
// Full key chain is: kKnownTrustedVaultKey -> kTrustedVaultKey1 ->
// kTrustedVaultKey2.
// Server-side key chain is: kTrustedVaultKey1 -> kTrustedVaultKey2.
-TEST_P(DownloadKeysResponseHandlerTest,
+TEST_F(DownloadKeysResponseHandlerTest,
ShouldHandleAbsenseOfKnownKeyWhenKeyChainIsRecoverable) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
@@ -232,7 +221,7 @@
// Possible full key chain is: kKnownTrustedVaultKey -> kTrustedVaultKey1 ->
// kTrustedVaultKey2 -> kTrustedVaultKey3.
// Server side key chain is: kTrustedVaultKey2 -> kTrustedVaultKey3.
-TEST_P(DownloadKeysResponseHandlerTest,
+TEST_F(DownloadKeysResponseHandlerTest,
ShouldHandleAbsenseOfKnownKeyWhenKeyChainIsNotRecoverable) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
@@ -255,7 +244,7 @@
// The test populates undecryptable/corrupted |wrapped_key| field, handler
// should return kMembershipCorrupted to allow client to restore the member by
// re-registration.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleUndecryptableKey) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleUndecryptableKey) {
trusted_vault_pb::SecurityDomainMember member;
AddSecurityDomainMembership(
GetSecurityDomainPath(SecurityDomainId::kChromeSync),
@@ -280,7 +269,7 @@
// The test populates invalid |rotation_proof| field for the single key
// rotation. kTrustedVaultKey1 is expected to be signed with
// kKnownTrustedVaultKey, but instead it's signed with kTrustedVaultKey2.
-TEST_P(DownloadKeysResponseHandlerTest,
+TEST_F(DownloadKeysResponseHandlerTest,
ShouldHandleInvalidKeyProofOnSingleKeyRotation) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
@@ -301,7 +290,7 @@
// multiple key rotations have happened.
// kTrustedVaultKey1 is expected to be signed with kKnownTrustedVaultKey, but
// instead it's signed with kTrustedVaultKey2.
-TEST_P(DownloadKeysResponseHandlerTest,
+TEST_F(DownloadKeysResponseHandlerTest,
ShouldHandleInvalidKeyProofOnMultipleKeyRotations) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
@@ -321,7 +310,7 @@
}
// In this scenario client already has most recent trusted vault key.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleAbsenseOfNewKeys) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleAbsenseOfNewKeys) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
/*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess,
@@ -339,7 +328,7 @@
// Tests handling the situation, when response isn't a valid serialized
// SecurityDomainMemberProto proto.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleCorruptedResponseProto) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleCorruptedResponseProto) {
EXPECT_THAT(handler()
.ProcessResponse(
/*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess,
@@ -350,7 +339,7 @@
// Client expects that the sync security domain membership exists, but the
// response indicates it doesn't by having no memberships.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleAbsenseOfMemberships) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleAbsenseOfMemberships) {
EXPECT_THAT(handler()
.ProcessResponse(
/*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess,
@@ -361,7 +350,7 @@
}
// Same as above, but there is a different security domain membership.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleAbsenseOfSyncMembership) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleAbsenseOfSyncMembership) {
trusted_vault_pb::SecurityDomainMember member;
AddSecurityDomainMembership(
"other_domain", MakeTestKeyPair()->public_key(),
@@ -377,7 +366,7 @@
Eq(TrustedVaultDownloadKeysStatus::kMembershipNotFound));
}
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleEmptyMembership) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleEmptyMembership) {
trusted_vault_pb::SecurityDomainMember member;
AddSecurityDomainMembership(
GetSecurityDomainPath(SecurityDomainId::kChromeSync),
@@ -394,7 +383,7 @@
Eq(TrustedVaultDownloadKeysStatus::kMembershipEmpty));
}
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleAllSecurityDomains) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleAllSecurityDomains) {
for (const SecurityDomainId security_domain : kAllSecurityDomainIdValues) {
trusted_vault_pb::SecurityDomainMember member;
AddSecurityDomainMembership(
@@ -423,7 +412,7 @@
}
// Tests handling presence of other security domain memberships.
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleMultipleSecurityDomains) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleMultipleSecurityDomains) {
trusted_vault_pb::SecurityDomainMember member;
AddSecurityDomainMembership(
"other_domain", MakeTestKeyPair()->public_key(),
@@ -453,7 +442,7 @@
Eq(kKnownTrustedVaultKeyVersion + 1));
}
-TEST_P(DownloadKeysResponseHandlerTest,
+TEST_F(DownloadKeysResponseHandlerTest,
ShouldHandleSameVersionWithDifferentKey) {
// Server sends a different key at same version, with no rotation proof.
const std::vector<uint8_t> kDifferentKey = {9, 9, 9, 9};
@@ -466,22 +455,12 @@
/*trusted_vault_keys_versions=*/{kKnownTrustedVaultKeyVersion},
/*signing_keys=*/{{}}));
- if (GetParam()) {
- EXPECT_THAT(processed_response.status,
- Eq(TrustedVaultDownloadKeysStatus::kMembershipCorrupted));
- EXPECT_THAT(processed_response.downloaded_keys, IsEmpty());
- } else {
- // NOTE: The legacy implementation (when the feature flag is disabled) has
- // a bug/vulnerability where it accepts a different key at the same version
- // without verification, incorrectly returning `kNoNewKeys` with the
- // server-supplied wrong key.
- EXPECT_THAT(processed_response.status,
- Eq(TrustedVaultDownloadKeysStatus::kNoNewKeys));
- EXPECT_THAT(processed_response.downloaded_keys, ElementsAre(kDifferentKey));
- }
+ EXPECT_THAT(processed_response.status,
+ Eq(TrustedVaultDownloadKeysStatus::kMembershipCorrupted));
+ EXPECT_THAT(processed_response.downloaded_keys, IsEmpty());
}
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleVersionRegression) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleVersionRegression) {
// Server sends key with a lower version, no rotation proof.
const std::vector<uint8_t> kLowerVersionKey = {9, 9, 9, 9};
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
@@ -494,22 +473,12 @@
{kKnownTrustedVaultKeyVersion - 1},
/*signing_keys=*/{{}}));
- if (GetParam()) {
- EXPECT_THAT(processed_response.status,
- Eq(TrustedVaultDownloadKeysStatus::kMembershipCorrupted));
- EXPECT_THAT(processed_response.downloaded_keys, IsEmpty());
- } else {
- // NOTE: The legacy implementation (when the feature flag is disabled) has
- // a bug/vulnerability where it tolerates version regression, incorrectly
- // returning `kNoNewKeys` with the regressed lower-version key.
- EXPECT_THAT(processed_response.status,
- Eq(TrustedVaultDownloadKeysStatus::kNoNewKeys));
- EXPECT_THAT(processed_response.downloaded_keys,
- ElementsAre(kLowerVersionKey));
- }
+ EXPECT_THAT(processed_response.status,
+ Eq(TrustedVaultDownloadKeysStatus::kMembershipCorrupted));
+ EXPECT_THAT(processed_response.downloaded_keys, IsEmpty());
}
-TEST_P(DownloadKeysResponseHandlerTest, ShouldHandleRotationsToNewKeys) {
+TEST_F(DownloadKeysResponseHandlerTest, ShouldHandleRotationsToNewKeys) {
const DownloadKeysResponseHandler::ProcessedResponse processed_response =
handler().ProcessResponse(
/*http_status=*/TrustedVaultRequest::HttpStatus::kSuccess,
Loading diff…
Original Bug Report
reported by [email protected]
Potential E2EE bypass in Trusted Vault via skipped rotation proof validation
Project Fortify, an experimental security project, has identified the following potential security issue.
Overview: A potential logic flaw in Chrome’s Trusted Vault implementation may allow a compromised server to bypass rotation proof verification and replace a client’s legitimate vault keys. By providing a crafted key with a version less than or equal to the client’s local version, the server can trigger a silent overwrite of the local keys. This could defeat end-to-end encryption for synced data like passwords, allowing the server to decrypt them.
Affected files:
components/trusted_vault/download_keys_response_handler.cccomponents/trusted_vault/standalone_trusted_vault_backend.cc
Estimated timestamp from git blame: 2025-04-28
Description
There is a potential vulnerability in the Trusted Vault key handling code that could allow a compromised server (or a Man-in-the-Middle attacker capable of spoofing the server) to bypass rotation proof verification. This bypass allows the attacker to silently replace a user’s legitimate trusted vault keys with keys known to the attacker. If exploited, this breaks the end-to-end encryption (E2EE) guarantees of Chrome Sync, as future synced data will be encrypted using the attacker-controlled keys.
Note: The following analysis is based on code review by our setup. The steps represent a potential attack path, as a working proof-of-concept has not yet been executed or verified dynamically.
Technical Details
The vulnerability stems from a sequence of logic flaws across the key downloading and storage pipeline:
-
Validation Bypass in
IsValidKeyChain: Incomponents/trusted_vault/download_keys_response_handler.cc, theIsValidKeyChainfunction iterates through downloaded keys to verify their rotation proofs. However, it explicitly skips verification for any keys with a version less than or equal to the client’s current version:// components/trusted_vault/download_keys_response_handler.cc:91 for (const ExtractedSharedKey& next_key : key_chain) { if (next_key.version <= last_valid_key_version) { continue; // Verification is skipped } // ... VerifyRotationProof() is called here ... }If an attacker provides a key with
version <= last_valid_key_version, the function returnstruewithout verifying the key’s cryptographic linkage to the existing chain. -
Unvalidated Keys Returned on
kNoNewKeys: InProcessResponse, if the downloaded keys’ maximum version is less than or equal to the local version, the status is correctly set tokNoNewKeys. However, the function still returns the unvalidated, decrypted keys back to the caller:// components/trusted_vault/download_keys_response_handler.cc:228 if (extracted_keys->back().version <= last_trusted_vault_key_and_version_.version) { status = TrustedVaultDownloadKeysStatus::kNoNewKeys; } return ProcessedResponse(status, trusted_vault_keys, ...); -
Blind Overwrite in
StandaloneTrustedVaultBackend: WhenStandaloneTrustedVaultBackend::OnKeysRecoveredreceives thekNoNewKeysstatus, it still callsStoreKeyswith the provided (and unvalidated) keys, citing the need to purge old keys removed from the server:// components/trusted_vault/standalone_trusted_vault_backend.cc:770 case LocalRecoveryFactor::RecoveryStatus::kNoNewKeys: { StoreKeys(primary_account_->gaia, downloaded_vault_keys, last_vault_key_version); break; }Finally,
StoreKeysclears the user’s existing, legitimate vault keys and replaces them entirely with the attacker’s unvalidated keys.
Potential Attack Steps
To trigger this vulnerability, an attacker controlling the sync server would potentially perform the following steps:
- Wait for (or trigger via
MarkLocalKeysAsStale) the client to attempt a key recovery/download viaDownloadNewKeys. - In response to the client’s request, construct a malicious
SecurityDomainMemberprotobuf payload. - Include a single
SharedMemberKeyin the payload, setting itsepoch(version) to the client’s current versionV(or lower). - Set the
wrapped_keyto an attacker-generated key (K_evil), encrypted using the client’s publicSecureBoxkey (which the server possesses from device registration). - Omit a valid
rotation_proof. - The client decrypts
K_evil, skips validation because its version is<= V, and processes it as akNoNewKeysresponse. - The client wipes its legitimate key
K_legitand persistsK_evil. - The attacker can now passively decrypt any future E2EE sync data stored on the server.
Suggested Fix
There are two primary areas that should be patched to resolve this:
- Do not return unvalidated keys: In
DownloadKeysResponseHandler::ProcessResponse, if the response results inkNoNewKeys(or if keys were skipped during validation), the handler should not include those unverified keys in theProcessedResponse. It should return an empty list or only the keys that were strictly verified viaVerifyRotationProof. - Strict Equality/Validation Checking: Re-evaluate the logic in
IsValidKeyChain. If older keys are supplied by the server to overwrite local storage, the client must verify that these older keys actually match the keys it already holds locally, rather than blindly skipping validation and trusting the server’s payload.
Evaluated with Chrome root at commit: 0eb4855bda702feaaa8b899336664f97e3df88b8
Results so far have been promising, but there can be wrong deductions. If this proves to be a false positive, please close as WAI; data from false positives will be used to improve accuracy over time. Please feel free to reach out to me if you have concerns or feedback.
References
On This Page