Fix trusting Nigori key name from proto

The Nigori key name can be inferred from key material so there is no
need to consume the proto field with the name.

Bug: 967417
Change-Id: Ibbd98b6496a9d797f90afbc951e5689e1d7ce491
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1824887Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699764}

components/sync/nigori/nigori.cc

@@ -202,6 +202,9 @@ bool Nigori::Keys::InitByImport(const std::string& user_key_str,
   // |user_key| is not used anymore so we tolerate a failed import.
   user_key = SymmetricKey::Import(SymmetricKey::AES, user_key_str);
 
+  if (encryption_key_str.empty() || mac_key_str.empty())
+    return false;
+
   encryption_key = SymmetricKey::Import(SymmetricKey::AES, encryption_key_str);
   if (!encryption_key)
     return false;

components/sync/nigori/nigori_key_bag.cc

@@ -55,17 +55,11 @@ NigoriKeyBag NigoriKeyBag::CreateEmpty() {
 NigoriKeyBag NigoriKeyBag::CreateFromProto(const sync_pb::NigoriKeyBag& proto) {
   NigoriKeyBag output;
   for (const sync_pb::NigoriKey& key : proto.key()) {
-    std::unique_ptr<Nigori> nigori = Nigori::CreateByImport(
-        key.user_key(), key.encryption_key(), key.mac_key());
-    if (!nigori) {
+    if (output.AddKeyFromProto(key).empty()) {
       // TODO(crbug.com/922900): Consider propagating this error to callers such
       // that they can do smarter handling.
       DLOG(ERROR) << "Invalid NigoriKey protocol buffer message.";
-      continue;
     }
-    // TODO(crbug.com/967417): We shouldn't trust the name in the proto and
-    // instead should compute ComputeNigoriName(*nigori).
-    output.nigori_map_[key.name()] = std::move(nigori);
   }
   return output;
 }
@@ -118,6 +112,22 @@ std::string NigoriKeyBag::AddKey(std::unique_ptr<Nigori> nigori) {
   return key_name;
 }
 
+std::string NigoriKeyBag::AddKeyFromProto(const sync_pb::NigoriKey& key) {
+  std::unique_ptr<Nigori> nigori = Nigori::CreateByImport(
+      key.user_key(), key.encryption_key(), key.mac_key());
+  if (!nigori) {
+    return std::string();
+  }
+
+  const std::string key_name = ComputeNigoriName(*nigori);
+  if (key_name.empty()) {
+    return std::string();
+  }
+
+  nigori_map_[key_name] = std::move(nigori);
+  return key_name;
+}
+
 void NigoriKeyBag::AddAllUnknownKeysFrom(const NigoriKeyBag& other) {
   for (const auto& key_name_and_nigori : other.nigori_map_) {
     // Only use this key if we don't already know about it.
@@ -155,7 +165,6 @@ bool NigoriKeyBag::Decrypt(const sync_pb::EncryptedData& encrypted_input,
   if (it == nigori_map_.end()) {
     // The key used to encrypt the blob is not part of the set of installed
     // nigoris.
-    DLOG(ERROR) << "Cannot decrypt message";
     return false;
   }
 

components/sync/nigori/nigori_key_bag.h

@@ -48,6 +48,10 @@ class NigoriKeyBag {
   // string in case of failure.
   std::string AddKey(std::unique_ptr<Nigori> nigori);
 
+  // Similar to AddKey(), but reads the key material from a proto. The |name|
+  // field is ignored since it's redundant.
+  std::string AddKeyFromProto(const sync_pb::NigoriKey& key);
+
   // Merges all keys from another keybag, which means adding all keys that we
   // don't know about.
   void AddAllUnknownKeysFrom(const NigoriKeyBag& other);

components/sync/nigori/nigori_key_bag_unittest.cc

@@ -114,5 +114,24 @@ TEST(NigoriKeyBagTest, ShouldClone) {
   EXPECT_TRUE(cloned_key_bag.HasKey(key_name2));
 }
 
+TEST(NigoriKeyBagTest, ShouldIgnoreKeyNameProtoField) {
+  NigoriKeyBag original_key_bag = NigoriKeyBag::CreateEmpty();
+  const std::string real_key_name =
+      original_key_bag.AddKey(CreateTestNigori("password1"));
+  ASSERT_THAT(original_key_bag, SizeIs(1));
+
+  const std::string actual_key_name_in_proto =
+      NigoriKeyBag::CreateEmpty().AddKey(CreateTestNigori("password2"));
+
+  sync_pb::NigoriKeyBag proto = original_key_bag.ToProto();
+  proto.mutable_key(0)->set_name(actual_key_name_in_proto);
+
+  NigoriKeyBag restored_key_bag = NigoriKeyBag::CreateFromProto(proto);
+
+  ASSERT_THAT(restored_key_bag, SizeIs(1));
+  EXPECT_TRUE(restored_key_bag.HasKey(real_key_name));
+  EXPECT_FALSE(restored_key_bag.HasKey(actual_key_name_in_proto));
+}
+
 }  // namespace
 }  // namespace syncer