Medium chrome Logic Error 📄 Reporter bug report 🔧 Commit mapped

Overview

Medium
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactImproper input validation in Sync
DescriptionImproper input validation in Sync
ComponentSync
Bug ClassLogic Error
Tracker496395158
Fix commita63b8a215549 (chromium/src) +56/-121
CISA KEVNot listed
CreditedGoogle
Disclosed2026-08-25

Changed Functions

FunctionChangeNotes
if
components/trusted_vault/download_keys_response_handler.cc
modified
DownloadKeysResponseHandlerTest
components/trusted_vault/download_keys_response_handler_unittest.cc
modified
TEST_P
components/trusted_vault/download_keys_response_handler_unittest.cc
modified
TEST_F
components/trusted_vault/download_keys_response_handler_unittest.cc
modified

Files Changed

  • components/trusted_vault/download_keys_response_handler.cc
  • components/trusted_vault/download_keys_response_handler_unittest.cc
From a63b8a215549dc9dc196c2e33fb2c047d98ec21c Mon Sep 17 00:00:00 2001
From: Thomas Thrainer <thomasth@google.com>
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 <thomasth@google.com>
Reviewed-by: Adem Derinel <derinel@google.com>
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 vm...@google.com

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.cc
  • components/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:

  1. Validation Bypass in IsValidKeyChain: In components/trusted_vault/download_keys_response_handler.cc, the IsValidKeyChain function 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 returns true without verifying the key’s cryptographic linkage to the existing chain.

  2. Unvalidated Keys Returned on kNoNewKeys: In ProcessResponse, if the downloaded keys’ maximum version is less than or equal to the local version, the status is correctly set to kNoNewKeys. 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, ...);
    
  3. Blind Overwrite in StandaloneTrustedVaultBackend: When StandaloneTrustedVaultBackend::OnKeysRecovered receives the kNoNewKeys status, it still calls StoreKeys with 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, StoreKeys clears 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:

  1. Wait for (or trigger via MarkLocalKeysAsStale) the client to attempt a key recovery/download via DownloadNewKeys.
  2. In response to the client’s request, construct a malicious SecurityDomainMember protobuf payload.
  3. Include a single SharedMemberKey in the payload, setting its epoch (version) to the client’s current version V (or lower).
  4. Set the wrapped_key to an attacker-generated key (K_evil), encrypted using the client’s public SecureBox key (which the server possesses from device registration).
  5. Omit a valid rotation_proof.
  6. The client decrypts K_evil, skips validation because its version is <= V, and processes it as a kNoNewKeys response.
  7. The client wipes its legitimate key K_legit and persists K_evil.
  8. 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:

  1. Do not return unvalidated keys: In DownloadKeysResponseHandler::ProcessResponse, if the response results in kNoNewKeys (or if keys were skipped during validation), the handler should not include those unverified keys in the ProcessedResponse. It should return an empty list or only the keys that were strictly verified via VerifyRotationProof.
  2. 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.

View on issue tracker