Fix crash in SetKeystoreKeys()

There is a scenario causing crash in old implementation:
1. Client receives NigoriSpecifics with non-decryptable
|keystore_decryptor_token| in backward-compatible keystore mode.
2. User provides implicit passphrase.
3. Client decrypts |pending_keys| with implicit passphrase, but
continues to ask for new keystore keys.
4. Client receives keystore keys and crashes when trying to dereference
|pending_keys|.

Current fix is to not do anything if there is no |pending_keys|.

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

chrome/browser/sync/test/integration/single_client_nigori_sync_test.cc

@@ -434,6 +434,49 @@ IN_PROC_BROWSER_TEST_P(SingleClientNigoriSyncTestWithUssTests,
   EXPECT_TRUE(GetSyncService(/*index=*/0)->GetExperimentalAuthenticationKey());
 }
 
+// Tests that client can decrypt |pending_keys| with implicit passphrase in
+// backward-compatible keystore mode, when |keystore_decryptor_token| is
+// non-decryptable (corrupted). Additionally verifies that there is no
+// regression causing crbug.com/1042203.
+IN_PROC_BROWSER_TEST_P(
+    SingleClientNigoriSyncTestWithUssTests,
+    ShouldDecryptWithImplicitPassphraseInBackwardCompatibleKeystoreMode) {
+  const std::vector<std::vector<uint8_t>>& keystore_keys =
+      GetFakeServer()->GetKeystoreKeys();
+  ASSERT_THAT(keystore_keys, SizeIs(1));
+
+  // Emulates mismatch between keystore key returned by the server and keystore
+  // key used in NigoriSpecifics.
+  std::vector<uint8_t> corrupted_keystore_key = keystore_keys[0];
+  corrupted_keystore_key.push_back(42u);
+  const KeyParams kKeystoreKeyParams =
+      KeystoreKeyParams(corrupted_keystore_key);
+  const KeyParams kDefaultKeyParams = {
+      syncer::KeyDerivationParams::CreateForPbkdf2(), "password"};
+  SetNigoriInFakeServer(
+      GetFakeServer(),
+      BuildKeystoreNigoriSpecifics(
+          /*keybag_keys_params=*/{kDefaultKeyParams, kKeystoreKeyParams},
+          /*keystore_decryptor_params*/ {kDefaultKeyParams},
+          /*keystore_key_params=*/kKeystoreKeyParams));
+
+  const autofill::PasswordForm password_form =
+      passwords_helper::CreateTestPasswordForm(0);
+  passwords_helper::InjectEncryptedServerPassword(
+      password_form, kDefaultKeyParams.password,
+      kDefaultKeyParams.derivation_params, GetFakeServer());
+  SetupSyncNoWaitingForCompletion();
+
+  EXPECT_TRUE(
+      PassphraseRequiredStateChecker(GetSyncService(0), /*desired_state=*/true)
+          .Wait());
+  EXPECT_TRUE(GetSyncService(0)->GetUserSettings()->SetDecryptionPassphrase(
+      "password"));
+  EXPECT_TRUE(WaitForPasswordForms({password_form}));
+  // TODO(crbug.com/1042251): verify that client fixes NigoriSpecifics once
+  // such behavior is supported.
+}
+
 INSTANTIATE_TEST_SUITE_P(USS,
                          SingleClientNigoriSyncTestWithUssTests,
                          ::testing::Values(false, true));

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -700,13 +700,15 @@ std::string NigoriSyncBridgeImpl::GetLastKeystoreKey() const {
 
 bool NigoriSyncBridgeImpl::NeedKeystoreKey() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // We explicitly ask the server for keystore keys iff it's first-time sync,
-  // i.e. if we have no keystore keys yet. In case of key rotation, it's a
-  // server responsibility to send updated keystore keys. |keystore_keys_| is
-  // expected to be non-empty before MergeSyncData() call, regardless of
-  // passphrase type.
+  // Explicitly asks the server for keystore keys if it's first-time sync, i.e.
+  // if there is no keystore keys yet or remote keybag wasn't decryptable due
+  // to absence of some keystore key. In case of key rotation, it's a server
+  // responsibility to send updated keystore keys. |keystore_keys_| is expected
+  // to be non-empty before MergeSyncData() call, regardless of passphrase
+  // type.
   return state_.keystore_keys_cryptographer->IsEmpty() ||
-         state_.pending_keystore_decryptor_token.has_value();
+         (state_.pending_keystore_decryptor_token.has_value() &&
+          state_.pending_keys.has_value());
 }
 
 bool NigoriSyncBridgeImpl::SetKeystoreKeys(
@@ -724,7 +726,12 @@ bool NigoriSyncBridgeImpl::SetKeystoreKeys(
     return false;
   }
 
-  if (state_.pending_keystore_decryptor_token.has_value()) {
+  // TODO(crbug.com/1042251): having |pending_keystore_decryptor_token| without
+  // |pending_keys| means that new |keystore_decryptor_token| could be build
+  // in order to replace the potentially corrupted remote
+  // |keystore_decryptor_token|.
+  if (state_.pending_keystore_decryptor_token.has_value() &&
+      state_.pending_keys.has_value()) {
     // Newly arrived keystore keys could resolve pending encryption state in
     // keystore mode.
     DCHECK_EQ(state_.passphrase_type, NigoriSpecifics::KEYSTORE_PASSPHRASE);

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -728,6 +728,11 @@ TEST_F(NigoriSyncBridgeImplTest,
   EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams));
   EXPECT_THAT(cryptographer, CanDecryptWith(kPassphraseKeyParams));
   EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kPassphraseKeyParams));
+
+  // Regression part of the test, SetKeystoreKeys() call in this scenario used
+  // to cause the crash (see crbug.com/1042203).
+  EXPECT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  EXPECT_FALSE(bridge()->NeedKeystoreKey());
 }
 
 // Tests that unsuccessful attempt of |pending_keys| decryption ends up in