[Sync:USS] Fix GetData() for backward compatible keystore Nigori

Old implementation of GetData() encrypted keystore_decryptor_token with
current default encryption key, which is wrong for backward compatible
keystore Nigori, since it uses non-keystore key as the default
encryption key. Now we always use most recent keystore key to encrypt
keystore_decryptor_token.

The old behavior shouldn't cause any issues, since it should be
impossible to reach GetData() in commit codepath, while Nigori is in
backward compatible keystore mode. But it's still better to fix, to
avoid data corruption in case it overlaps with other bugs and to not
violate invariants.

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

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -70,18 +70,22 @@ bool EncryptKeyBag(const CryptographerImpl& cryptographer,
   return cryptographer.Encrypt(proto.key_bag(), encrypted);
 }
 
-// TODO(crbug.com/922900): This should be revisited because the encryption key
-// should be determined by keystore keys, which does not always match the
-// default encryption key.
 bool EncryptKeystoreDecryptorToken(
     const CryptographerImpl& cryptographer,
-    sync_pb::EncryptedData* keystore_decryptor_token) {
+    sync_pb::EncryptedData* keystore_decryptor_token,
+    const std::vector<std::string>& keystore_keys) {
   DCHECK(keystore_decryptor_token);
 
   const sync_pb::NigoriKey default_key = cryptographer.ExportDefaultKey();
 
-  return cryptographer.EncryptString(default_key.SerializeAsString(),
-                                     keystore_decryptor_token);
+  // TODO(crbug.com/922900): consider maintaining cached version of this
+  // |keystore_cryptographer| to avoid its creation here and inside
+  // DecryptKestoreDecryptorToken().
+  std::unique_ptr<CryptographerImpl> keystore_cryptographer =
+      CreateCryptographerFromKeystoreKeys(keystore_keys);
+
+  return keystore_cryptographer->EncryptString(default_key.SerializeAsString(),
+                                               keystore_decryptor_token);
 }
 
 // Attempts to decrypt |keystore_decryptor_token| with |keystore_keys|. Returns
@@ -131,7 +135,8 @@ base::Optional<NigoriSpecifics> MakeDefaultKeystoreNigori(
 
   NigoriSpecifics specifics;
   if (!EncryptKeystoreDecryptorToken(
-          *cryptographer, specifics.mutable_keystore_decryptor_token())) {
+          *cryptographer, specifics.mutable_keystore_decryptor_token(),
+          keystore_keys)) {
     DLOG(ERROR) << "Failed to encrypt default key as keystore_decryptor_token.";
     return base::nullopt;
   }
@@ -1081,7 +1086,8 @@ std::unique_ptr<EntityData> NigoriSyncBridgeImpl::GetData() {
   }
   if (state_.passphrase_type == NigoriSpecifics::KEYSTORE_PASSPHRASE) {
     EncryptKeystoreDecryptorToken(*state_.cryptographer,
-                                  specifics.mutable_keystore_decryptor_token());
+                                  specifics.mutable_keystore_decryptor_token(),
+                                  state_.keystore_keys);
   }
   if (!state_.keystore_migration_time.is_null()) {
     specifics.set_keystore_migration_time(

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -108,6 +108,21 @@ MATCHER_P(EncryptedDataEq, expected, "") {
          given.blob() == expected.blob();
 }
 
+MATCHER_P3(EncryptedDataEqAfterDecryption,
+           expected,
+           password,
+           derivation_params,
+           "") {
+  const sync_pb::EncryptedData& given = arg;
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::FromSingleKeyForTesting(password, derivation_params);
+  std::string decrypted_given;
+  EXPECT_TRUE(cryptographer->DecryptToString(given, &decrypted_given));
+  std::string decrypted_expected;
+  EXPECT_TRUE(cryptographer->DecryptToString(expected, &decrypted_expected));
+  return decrypted_given == decrypted_expected;
+}
+
 MATCHER_P2(IsDummyNigoriMetadataBatchWithTokenAndSequenceNumber,
            expected_token,
            expected_sequence_number,
@@ -452,6 +467,42 @@ TEST_F(NigoriSyncBridgeImplTest,
   EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kGaiaKeyParams));
 }
 
+TEST_F(NigoriSyncBridgeImplTest, ShouldExposeBackwardCompatibleKeystoreNigori) {
+  const KeyParams kGaiaKeyParams = Pbkdf2KeyParams("gaia_key");
+  const std::string kRawKeystoreKey = "raw_keystore_key";
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams(kRawKeystoreKey);
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kGaiaKeyParams, kKeystoreKeyParams},
+      /*keystore_decryptor_params=*/kGaiaKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+  sync_pb::EncryptedData original_encryption_keybag =
+      entity_data.specifics.nigori().encryption_keybag();
+  sync_pb::EncryptedData original_keystore_decryptor_token =
+      entity_data.specifics.nigori().keystore_decryptor_token();
+
+  ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+
+  std::unique_ptr<EntityData> local_entity_data = bridge()->GetData();
+  ASSERT_TRUE(local_entity_data);
+  ASSERT_TRUE(local_entity_data->specifics.has_nigori());
+  // Note: EncryptedDataEqAfterDecryption() exercises more strict requirements
+  // than bridge must support, because there is nothing wrong with reordering
+  // of the keys in encryption_keybag, which will lead to failing this
+  // expectation.
+  EXPECT_THAT(local_entity_data->specifics.nigori().encryption_keybag(),
+              EncryptedDataEqAfterDecryption(original_encryption_keybag,
+                                             kGaiaKeyParams.password,
+                                             kGaiaKeyParams.derivation_params));
+  EXPECT_THAT(
+      local_entity_data->specifics.nigori().keystore_decryptor_token(),
+      EncryptedDataEqAfterDecryption(original_keystore_decryptor_token,
+                                     kKeystoreKeyParams.password,
+                                     kKeystoreKeyParams.derivation_params));
+}
+
 // Tests that we can successfully use old keys from encryption_keybag in
 // backward compatible mode.
 TEST_F(NigoriSyncBridgeImplTest,