Support transitions from trusted vault passphrase

This CL supports transitions from trusted vault passphrase to keystore
and custom passphrases. To avoid data corruptions by buggy clients the
bridge verifies that the remote keybag contains the last trusted vault
key, which was used as the default key. Since browser restart can
happen in between of receiving the remote update and decryption of the
new remote keybag, |last_trusted_vault_key| stored in NigoriLocalData.

Bug: 1020084
Change-Id: Ica654898a3499f599258bf9e65eb8d91580fcc2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087766
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747697}

components/sync/nigori/nigori_state.cc

@@ -152,6 +152,12 @@ NigoriState NigoriState::CreateFromLocalProto(
     state.pending_keystore_decryptor_token =
         proto.pending_keystore_decryptor_token();
   }
+
+  if (proto.has_last_default_trusted_vault_key_name()) {
+    state.last_default_trusted_vault_key_name =
+        proto.last_default_trusted_vault_key_name();
+  }
+
   return state;
 }
 
@@ -210,6 +216,10 @@ sync_pb::NigoriModel NigoriState::ToLocalProto() const {
     *proto.mutable_pending_keystore_decryptor_token() =
         *pending_keystore_decryptor_token;
   }
+  if (last_default_trusted_vault_key_name.has_value()) {
+    proto.set_last_default_trusted_vault_key_name(
+        *last_default_trusted_vault_key_name);
+  }
   return proto;
 }
 
@@ -236,6 +246,8 @@ sync_pb::NigoriSpecifics NigoriState::ToSpecificsProto() const {
     UpdateSpecificsFromKeyDerivationParams(
         *custom_passphrase_key_derivation_params, &specifics);
   }
+  // TODO(crbug.com/1020084): populate |keystore_decryptor_token| for trusted
+  // vault passphrase to allow rollbacks.
   if (passphrase_type == sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE) {
     // TODO(crbug.com/922900): it seems possible to have corrupted
     // |pending_keystore_decryptor_token| and an ability to recover it in case
@@ -277,6 +289,8 @@ NigoriState NigoriState::Clone() const {
   result.encrypt_everything = encrypt_everything;
   result.keystore_keys_cryptographer = keystore_keys_cryptographer->Clone();
   result.pending_keystore_decryptor_token = pending_keystore_decryptor_token;
+  result.last_default_trusted_vault_key_name =
+      last_default_trusted_vault_key_name;
   return result;
 }
 

components/sync/nigori/nigori_state.h

@@ -82,6 +82,10 @@ struct NigoriState {
   // can't be decrypted right after remote update arrival due to lack of
   // keystore keys. May be set only for keystore Nigori.
   base::Optional<sync_pb::EncryptedData> pending_keystore_decryptor_token;
+
+  // The name of the latest available trusted vault key that was used as the
+  // default encryption key.
+  base::Optional<std::string> last_default_trusted_vault_key_name;
 };
 
 }  // namespace syncer

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -198,13 +198,8 @@ bool IsValidPassphraseTransition(
     case NigoriSpecifics::CUSTOM_PASSPHRASE:
       return false;
     case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
-      // TODO(crbug.com/984940): The below should be allowed for
-      // CUSTOM_PASSPHRASE and KEYSTORE_PASSPHRASE but it requires carefully
-      // verifying that the client triggering the transition already had access
-      // to the trusted vault passphrase (e.g. the new keybag must be a
-      // superset of the old and the default key must have changed).
-      NOTIMPLEMENTED();
-      return false;
+      return new_passphrase_type == NigoriSpecifics::CUSTOM_PASSPHRASE ||
+             new_passphrase_type == NigoriSpecifics::KEYSTORE_PASSPHRASE;
   }
   NOTREACHED();
   return false;
@@ -653,6 +648,9 @@ void NigoriSyncBridgeImpl::AddTrustedVaultDecryptionKeys(
     return;
   }
 
+  state_.last_default_trusted_vault_key_name =
+      state_.cryptographer->GetDefaultEncryptionKeyName();
+
   storage_->StoreData(SerializeAsNigoriLocalData());
   broadcasting_observer_->OnCryptographerStateChanged(
       state_.cryptographer.get(), state_.pending_keys.has_value());
@@ -995,6 +993,17 @@ base::Optional<ModelError> NigoriSyncBridgeImpl::TryDecryptPendingKeysWith(
                       "Received keybag is missing the new default key.");
   }
 
+  if (state_.last_default_trusted_vault_key_name.has_value() &&
+      !new_key_bag.HasKey(*state_.last_default_trusted_vault_key_name)) {
+    // Protocol violation.
+    return ModelError(FROM_HERE,
+                      "Received keybag is missing the last trusted vault key.");
+  }
+
+  // Reset |last_default_trusted_vault_key_name| as |state_| might go out of
+  // TRUSTED_VAULT passphrase type. The callers are responsible to set it again
+  // if needed.
+  state_.last_default_trusted_vault_key_name = base::nullopt;
   state_.cryptographer->EmplaceKeysFrom(new_key_bag);
   state_.cryptographer->SelectDefaultEncryptionKey(new_default_key_name);
   state_.pending_keys.reset();
@@ -1054,6 +1063,7 @@ void NigoriSyncBridgeImpl::ApplyDisableSyncChanges() {
   state_.custom_passphrase_time = base::Time();
   state_.keystore_migration_time = base::Time();
   state_.custom_passphrase_key_derivation_params = base::nullopt;
+  state_.last_default_trusted_vault_key_name = base::nullopt;
   broadcasting_observer_->OnCryptographerStateChanged(
       state_.cryptographer.get(),
       /*has_pending_keys=*/false);

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -1141,9 +1141,7 @@ TEST(NigoriSyncBridgeImplPersistenceTest, ShouldRestoreKeystoreNigori) {
   auto storage1 = std::make_unique<testing::NiceMock<MockNigoriStorage>>();
   sync_pb::NigoriLocalData nigori_local_data;
   ON_CALL(*storage1, StoreData(_))
-      .WillByDefault([&](const sync_pb::NigoriLocalData& data) {
-        nigori_local_data = data;
-      });
+      .WillByDefault(testing::SaveArg<0>(&nigori_local_data));
 
   // Provide some metadata to verify that we store it.
   auto processor1 =
@@ -1428,6 +1426,260 @@ TEST_F(NigoriSyncBridgeImplTest,
   EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
 }
 
+// Tests processing of remote incremental update that transits from trusted
+// vault to keystore passphrase.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldProcessRemoteTransitionFromTrustedVaultToKeystore) {
+  const KeyParams kTrustedVaultKeyParams =
+      TrustedVaultKeyParams(kTrustedVaultKey);
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKeyParams});
+
+  ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->Init());
+  ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
+  bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
+  ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
+  ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
+              Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
+  ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
+
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams(kRawKeystoreKey);
+  EntityData new_entity_data;
+  *new_entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kTrustedVaultKeyParams, kKeystoreKeyParams},
+      /*keystore_decryptor_params=*/kKeystoreKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+
+  EXPECT_CALL(*observer(), OnEncryptedTypesChanged(_, _)).Times(0);
+  EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
+  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
+                               NotNull(), /*has_pending_keys=*/false));
+  EXPECT_CALL(
+      *observer(),
+      OnPassphraseTypeChanged(PassphraseType::kKeystorePassphrase, NullTime()));
+
+  EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
+              Eq(base::nullopt));
+  EXPECT_THAT(bridge()->GetPassphraseTypeForTesting(),
+              Eq(sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE));
+  EXPECT_THAT(bridge()->GetEncryptedTypesForTesting(),
+              Eq(AlwaysEncryptedUserTypes()));
+  EXPECT_FALSE(bridge()->HasPendingKeysForTesting());
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kTrustedVaultKeyParams));
+  EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams));
+  EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kKeystoreKeyParams));
+}
+
+// Tests processing of remote incremental update that transits from trusted
+// vault to custom passphrase.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldProcessRemoteTransitionFromTrustedVaultToCustomPassphrase) {
+  const KeyParams kTrustedVaultKeyParams =
+      TrustedVaultKeyParams(kTrustedVaultKey);
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() =
+      BuildTrustedVaultNigoriSpecifics({kTrustedVaultKeyParams});
+
+  ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->Init());
+  ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
+  bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
+  ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
+  ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
+              Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
+  ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
+
+  const KeyParams kCustomPassphraseKeyParams =
+      Pbkdf2KeyParams("custom_passphrase");
+  EntityData new_entity_data;
+  *new_entity_data.specifics.mutable_nigori() =
+      BuildCustomPassphraseNigoriSpecifics(
+          kCustomPassphraseKeyParams,
+          /*old_key_params=*/kTrustedVaultKeyParams);
+
+  EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
+                               /*encrypted_types=*/EncryptableUserTypes(),
+                               /*encrypt_everything=*/true));
+  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
+                               NotNull(), /*has_pending_keys=*/true));
+  EXPECT_CALL(*observer(),
+              OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
+                                      Not(NullTime())));
+  EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
+              Eq(base::nullopt));
+  EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
+
+  EXPECT_CALL(*observer(), OnPassphraseAccepted());
+  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
+                               NotNull(), /*has_pending_keys=*/false));
+  EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(Ne(std::string()),
+                                                   PASSPHRASE_BOOTSTRAP_TOKEN));
+  bridge()->SetDecryptionPassphrase(kCustomPassphraseKeyParams.password);
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kTrustedVaultKeyParams));
+  EXPECT_THAT(cryptographer, CanDecryptWith(kCustomPassphraseKeyParams));
+  EXPECT_THAT(cryptographer,
+              HasDefaultKeyDerivedFrom(kCustomPassphraseKeyParams));
+}
+
+// Tests processing of remote incremental update that transits from trusted
+// vault to keystore passphrase, which doesn't contain trusted vault key. The
+// bridge should report model error.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldFailOnInvalidRemoteTransitionFromTrustedVaultToKeystore) {
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
+      {TrustedVaultKeyParams(kTrustedVaultKey)});
+
+  ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->Init());
+  ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
+  bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
+  ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
+  ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
+              Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
+  ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
+
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams(kRawKeystoreKey);
+  // Don't populate kTrustedVaultKey into |new_entity_data|.
+  EntityData new_entity_data;
+  *new_entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kKeystoreKeyParams},
+      /*keystore_decryptor_params=*/kKeystoreKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+
+  EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
+              Ne(base::nullopt));
+}
+
+// Tests processing of remote incremental update that transits from trusted
+// vault to custom passphrase, which doesn't contain trusted vault key. The
+// bridge should report model error.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldFailOnInvalidRemoteTransitionFromTrustedVaultToCustomPassphrase) {
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
+      {TrustedVaultKeyParams(kTrustedVaultKey)});
+
+  ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->Init());
+  ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
+  bridge()->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
+  ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
+  ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
+              Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
+  ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
+
+  const KeyParams kCustomPassphraseKeyParams =
+      Pbkdf2KeyParams("custom_passphrase");
+  // Don't populate kTrustedVaultKey into |new_entity_data|.
+  EntityData new_entity_data;
+  *new_entity_data.specifics.mutable_nigori() =
+      BuildCustomPassphraseNigoriSpecifics(kCustomPassphraseKeyParams);
+
+  // The bridge doesn't know whether update is valid until decryption, expect
+  // processing as a normal update.
+  EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
+                               /*encrypted_types=*/EncryptableUserTypes(),
+                               /*encrypt_everything=*/true));
+  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
+                               NotNull(), /*has_pending_keys=*/true));
+  EXPECT_CALL(*observer(),
+              OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
+                                      Not(NullTime())));
+  EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
+              Eq(base::nullopt));
+  EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
+
+  // Once decryption passphrase is provided, bridge should ReportError().
+  EXPECT_CALL(*processor(), ReportError(_));
+  bridge()->SetDecryptionPassphrase(kCustomPassphraseKeyParams.password);
+}
+
+// Tests processing of remote incremental update that transits from trusted
+// vault to custom passphrase, which doesn't contain trusted vault key. Mimics
+// browser restart in between of receiving the remote update and providing
+// custom passphrase. The bridge should report model error.
+TEST(NigoriSyncBridgeImplPersistenceTest,
+     ShouldFailOnInvalidRemoteTransitionFromTrustedVaultAfterRestart) {
+  // Emulate storing on disc.
+  auto storage1 = std::make_unique<testing::NiceMock<MockNigoriStorage>>();
+  sync_pb::NigoriLocalData nigori_local_data;
+  ON_CALL(*storage1, StoreData(_))
+      .WillByDefault(testing::SaveArg<0>(&nigori_local_data));
+
+  const FakeEncryptor kEncryptor;
+  auto bridge1 = std::make_unique<NigoriSyncBridgeImpl>(
+      std::make_unique<testing::NiceMock<MockNigoriLocalChangeProcessor>>(),
+      std::move(storage1), &kEncryptor,
+      base::BindRepeating(&Nigori::GenerateScryptSalt),
+      /*packed_explicit_passphrase_key=*/std::string(),
+      /*packed_keystore_keys=*/std::string());
+
+  // Perform initial sync with trusted vault passphrase.
+  const std::vector<uint8_t> kTrustedVaultKey = {2, 3, 4, 5, 6};
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
+      {TrustedVaultKeyParams(kTrustedVaultKey)});
+
+  const std::vector<uint8_t> kRawKeystoreKey = {0, 1, 2, 3, 4};
+  ASSERT_TRUE(bridge1->SetKeystoreKeys({kRawKeystoreKey}));
+  ASSERT_THAT(bridge1->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+  ASSERT_TRUE(bridge1->Init());
+  ASSERT_TRUE(bridge1->HasPendingKeysForTesting());
+  bridge1->AddTrustedVaultDecryptionKeys({kTrustedVaultKey});
+  ASSERT_FALSE(bridge1->HasPendingKeysForTesting());
+  ASSERT_THAT(bridge1->GetPassphraseTypeForTesting(),
+              Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
+  ASSERT_THAT(bridge1->GetData(), Not(HasCustomPassphraseNigori()));
+
+  // Mimic invalid remote update with custom passphrase.
+  const KeyParams kCustomPassphraseKeyParams =
+      Pbkdf2KeyParams("custom_passphrase");
+  // Don't populate kTrustedVaultKeyParams into |new_entity_data|.
+  EntityData new_entity_data;
+  *new_entity_data.specifics.mutable_nigori() =
+      BuildCustomPassphraseNigoriSpecifics(kCustomPassphraseKeyParams);
+
+  // The bridge doesn't know whether update is valid until decryption, expect
+  // processing as a normal update.
+  ASSERT_THAT(bridge1->ApplySyncChanges(std::move(new_entity_data)),
+              Eq(base::nullopt));
+
+  // Create secondary storage which will return |nigori_local_data| on
+  // RestoreData() call.
+  auto storage2 = std::make_unique<testing::NiceMock<MockNigoriStorage>>();
+  ON_CALL(*storage2, RestoreData()).WillByDefault(Return(nigori_local_data));
+
+  // Create secondary processor.
+  auto processor2 =
+      std::make_unique<testing::NiceMock<MockNigoriLocalChangeProcessor>>();
+  // Once decryption passphrase is provided, bridge should ReportError().
+  EXPECT_CALL(*processor2, ReportError(_));
+
+  auto bridge2 = std::make_unique<NigoriSyncBridgeImpl>(
+      std::move(processor2), std::move(storage2), &kEncryptor,
+      base::BindRepeating(&Nigori::GenerateScryptSalt),
+      /*packed_explicit_passphrase_key=*/std::string(),
+      /*packed_keystore_keys=*/std::string());
+
+  bridge2->SetDecryptionPassphrase(kCustomPassphraseKeyParams.password);
+}
+
 TEST_F(NigoriSyncBridgeImplTest,
        ShouldNotAddDecryptionKeysToTrustedVaultCryptographer) {
   const std::vector<uint8_t> kTrustedVaultKey1{kTrustedVaultKey};

components/sync/protocol/nigori_local_data.proto

@@ -90,6 +90,11 @@ message NigoriModel {
   // Encryptor keystore decryptor token. Used for decryption of keystore Nigori
   // in case keystore keys arrived after NigoriSpecifics.
   optional EncryptedData pending_keystore_decryptor_token = 11;
+
+  // Contains the name of the latest available trusted vault key that was used
+  // as the default encryption key. Resets when the client go out of pending
+  // decryption state and transits to other passphrase types.
+  optional string last_default_trusted_vault_key_name = 12;
 }
 
 // Sync proto to store Nigori data in storage. Proto should be encrypted with

components/sync/protocol/proto_visitors.h

@@ -606,6 +606,7 @@ VISIT_PROTO_FIELDS(const sync_pb::NigoriModel& proto) {
   VISIT_REP(encrypted_types_specifics_field_number);
   VISIT_REP(keystore_key);
   VISIT(pending_keystore_decryptor_token);
+  VISIT(last_default_trusted_vault_key_name);
 }
 
 VISIT_PROTO_FIELDS(const sync_pb::NigoriLocalData& proto) {