Adopt base64-encoding prior to vault Nigori key derivation

Similarly to how it's done for keystore keys, the patch now adopts a
base64-encoding of the encryption keys, prior to the Nigori key
derivation (PBKDF2).

The main motivation is that the underlying crypto libraries don't
deal well with non-utf8 passphrases/passwords, or they diverge across
implementations (Java vs C++).

The rest of the code, prior to key derivation, continues to use binary
keys.

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

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

@@ -89,8 +89,6 @@ GURL GetTrustedVaultRetrievalURL(
 }
 
 KeyParams KeystoreKeyParams(const std::string& key) {
-  // Due to mis-encode of keystore keys to base64 we have to always encode such
-  // keys to provide backward compatibility.
   std::string encoded_key;
   base::Base64Encode(key, &encoded_key);
   return {syncer::KeyDerivationParams::CreateForPbkdf2(),
@@ -159,8 +157,11 @@ sync_pb::NigoriSpecifics BuildTrustedVaultNigoriSpecifics(
   std::unique_ptr<syncer::CryptographerImpl> cryptographer =
       syncer::CryptographerImpl::CreateEmpty();
   for (const std::string& trusted_vault_key : trusted_vault_keys) {
+    std::string encoded_key;
+    base::Base64Encode(trusted_vault_key, &encoded_key);
+
     const std::string key_name = cryptographer->EmplaceKey(
-        trusted_vault_key, syncer::KeyDerivationParams::CreateForPbkdf2());
+        encoded_key, syncer::KeyDerivationParams::CreateForPbkdf2());
     cryptographer->SelectDefaultEncryptionKey(key_name);
   }
 

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

@@ -43,8 +43,6 @@ const syncer::SyncFirstSetupCompleteSource kSetSourceFromTest =
     syncer::SyncFirstSetupCompleteSource::BASIC_FLOW;
 
 syncer::KeyParams KeystoreKeyParams(const std::string& key) {
-  // Due to mis-encode of keystore keys to base64 we have to always encode such
-  // keys to provide backward compatibility.
   std::string encoded_key;
   base::Base64Encode(key, &encoded_key);
   return {syncer::KeyDerivationParams::CreateForPbkdf2(),

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -93,6 +93,20 @@ KeyDerivationParams GetKeyDerivationParamsFromSpecifics(
   return KeyDerivationParams::CreateWithUnsupportedMethod();
 }
 
+// We need to apply base64 encoding before deriving Nigori keys because the
+// underlying crypto libraries (in particular the Java counterparts in JDK's
+// implementation for PBKDF2) assume the keys are utf8.
+std::vector<std::string> Base64EncodeKeys(
+    const std::vector<std::string>& keys) {
+  std::vector<std::string> encoded_keystore_keys;
+  for (const std::string& key : keys) {
+    std::string encoded_key;
+    base::Base64Encode(key, &encoded_key);
+    encoded_keystore_keys.push_back(std::move(encoded_key));
+  }
+  return encoded_keystore_keys;
+}
+
 bool SpecificsHasValidKeyDerivationParams(const NigoriSpecifics& specifics) {
   switch (GetKeyDerivationMethodFromSpecifics(specifics)) {
     case KeyDerivationMethod::UNSUPPORTED:
@@ -578,12 +592,11 @@ void NigoriSyncBridgeImpl::AddTrustedVaultDecryptionKeys(
     return;
   }
 
+  const std::vector<std::string> encoded_keys = Base64EncodeKeys(keys);
   NigoriKeyBag tmp_key_bag = NigoriKeyBag::CreateEmpty();
-  for (const std::string& key : keys) {
-    if (!key.empty()) {
-      tmp_key_bag.AddKey(Nigori::CreateByDerivation(
-          GetKeyDerivationParamsForPendingKeys(), key));
-    }
+  for (const std::string& encoded_key : encoded_keys) {
+    tmp_key_bag.AddKey(Nigori::CreateByDerivation(
+        GetKeyDerivationParamsForPendingKeys(), encoded_key));
   }
 
   base::Optional<ModelError> error = TryDecryptPendingKeysWith(tmp_key_bag);
@@ -652,17 +665,8 @@ bool NigoriSyncBridgeImpl::SetKeystoreKeys(
     return false;
   }
 
-  std::vector<std::string> encoded_keystore_keys(keys.size());
-  for (size_t i = 0; i < keys.size(); ++i) {
-    // We need to apply base64 encoding before using the keys to provide
-    // backward compatibility with non-USS implementation. It's actually needed
-    // only for the keys persisting, but was applied before passing keys to
-    // cryptographer, so we have to do the same.
-    base::Base64Encode(keys[i], &encoded_keystore_keys[i]);
-  }
-
   state_.keystore_keys_cryptographer =
-      KeystoreKeysCryptographer::FromKeystoreKeys(encoded_keystore_keys);
+      KeystoreKeysCryptographer::FromKeystoreKeys(Base64EncodeKeys(keys));
   if (!state_.keystore_keys_cryptographer) {
     state_.keystore_keys_cryptographer =
         KeystoreKeysCryptographer::CreateEmpty();

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -153,13 +153,18 @@ KeyParams Pbkdf2KeyParams(std::string key) {
 }
 
 KeyParams KeystoreKeyParams(const std::string& key) {
-  // Due to mis-encode of keystore keys to base64 we have to always encode such
-  // keys to provide backward compatibility.
+  // base64 encoding of the keys was adopted before deriving Nigori keys because
+  // the underlying crypto libraries (in particular the Java counterparts in
+  // JDK's implementation for PBKDF2) assume the keys are utf8.
   std::string encoded_key;
   base::Base64Encode(key, &encoded_key);
   return Pbkdf2KeyParams(std::move(encoded_key));
 }
 
+KeyParams TrustedVaultKeyParams(const std::string& key) {
+  return KeystoreKeyParams(key);
+}
+
 KeyParams ScryptKeyParams(const std::string& key) {
   return {KeyDerivationParams::CreateForScrypt("some_constant_salt"), key};
 }
@@ -1248,8 +1253,8 @@ TEST_F(NigoriSyncBridgeImplTest,
        ShouldRequireUserActionIfInitiallyUsingTrustedVault) {
   const std::string kTrustedVaultKey = "trusted_vault_key";
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
+      {TrustedVaultKeyParams(kTrustedVaultKey)});
 
   EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
 
@@ -1304,7 +1309,8 @@ TEST_F(NigoriSyncBridgeImplTest,
 
   EntityData new_entity_data;
   *new_entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
+      BuildTrustedVaultNigoriSpecifics(
+          {TrustedVaultKeyParams(kTrustedVaultKey)});
 
   EXPECT_CALL(*observer(), OnEncryptedTypesChanged(_, _)).Times(0);
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
@@ -1340,8 +1346,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   EXPECT_CALL(*observer(), OnPassphraseRequired(_, _, _)).Times(0);
 
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
+      {TrustedVaultKeyParams(kTrustedVaultKey)});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1356,8 +1362,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   EntityData new_entity_data;
   *new_entity_data.specifics.mutable_nigori() =
       BuildTrustedVaultNigoriSpecifics(
-          {Pbkdf2KeyParams(kTrustedVaultKey),
-           Pbkdf2KeyParams(kRotatedTrustedVaultKey)});
+          {TrustedVaultKeyParams(kTrustedVaultKey),
+           TrustedVaultKeyParams(kRotatedTrustedVaultKey)});
   EXPECT_CALL(*observer(), OnEncryptedTypesChanged(_, _)).Times(0);
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
   EXPECT_CALL(*observer(), OnPassphraseTypeChanged(_, _)).Times(0);
@@ -1384,8 +1390,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   const std::string kCustomPassphrase = "custom_passphrase";
 
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey)});
+  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
+      {TrustedVaultKeyParams(kTrustedVaultKey)});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1425,8 +1431,8 @@ TEST_F(NigoriSyncBridgeImplTest,
   const std::string kTrustedVaultKey1 = "trusted_vault_key_1";
   const std::string kTrustedVaultKey2 = "trusted_vault_key_2";
   EntityData entity_data;
-  *entity_data.specifics.mutable_nigori() =
-      BuildTrustedVaultNigoriSpecifics({Pbkdf2KeyParams(kTrustedVaultKey1)});
+  *entity_data.specifics.mutable_nigori() = BuildTrustedVaultNigoriSpecifics(
+      {TrustedVaultKeyParams(kTrustedVaultKey1)});
 
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
@@ -1444,9 +1450,9 @@ TEST_F(NigoriSyncBridgeImplTest,
   const CryptographerImpl& cryptographer =
       bridge()->GetCryptographerForTesting();
   ASSERT_THAT(cryptographer,
-              CanDecryptWith(Pbkdf2KeyParams(kTrustedVaultKey1)));
+              CanDecryptWith(TrustedVaultKeyParams(kTrustedVaultKey1)));
   EXPECT_THAT(cryptographer,
-              Not(CanDecryptWith(Pbkdf2KeyParams(kTrustedVaultKey2))));
+              Not(CanDecryptWith(TrustedVaultKeyParams(kTrustedVaultKey2))));
   EXPECT_THAT(cryptographer.KeyBagSizeForTesting(), Eq(size_t(1)));
 }