Revert of [sync] Fix decryption failure caused by missing user_key (p…

…atchset chromium#2 id:40001 of https://codereview.chromium.org/2813453004/ )

Reason for revert:
Causing crashes on all platforms

Original issue's description:
> [sync] Fix decryption failure caused by missing user_key
>
> Recently, a change removed code that populated the user_key field of
> nigori nodes. Clients running code from before this change fail to
> import keys that don't have user_key populated. If a post-change
> client commits a nigori node, a pre-change client will fail to
> subsequently import it.
> This change restores code that populates user_key, but omits code
> that fails the import if the user_key is absent.
>
> R=pavely@chromium.org
> BUG=707967
>
> Review-Url: https://codereview.chromium.org/2813453004
> Cr-Commit-Position: refs/heads/master@{#463455}
> Committed: https://chromium.googlesource.com/chromium/src/+/399458cbfa1d8458842e9119e6ad063716c7e66e

TBR=pavely@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=707967

Review-Url: https://codereview.chromium.org/2809853003
Cr-Commit-Position: refs/heads/master@{#463655}

components/sync/base/cryptographer.cc

@@ -34,10 +34,10 @@ Cryptographer::Cryptographer(const Cryptographer& other)
       default_nigori_name_(other.default_nigori_name_) {
   for (NigoriMap::const_iterator it = other.nigoris_.begin();
        it != other.nigoris_.end(); ++it) {
-    std::string user_key, encryption_key, mac_key;
-    it->second->ExportKeys(&user_key, &encryption_key, &mac_key);
+    std::string encryption_key, mac_key;
+    it->second->ExportKeys(&encryption_key, &mac_key);
     linked_ptr<Nigori> nigori_copy(new Nigori());
-    nigori_copy->InitByImport(user_key, encryption_key, mac_key);
+    nigori_copy->InitByImport(encryption_key, mac_key);
     nigoris_.insert(std::make_pair(it->first, nigori_copy));
   }
 
@@ -150,8 +150,7 @@ bool Cryptographer::GetKeys(sync_pb::EncryptedData* encrypted) const {
     const Nigori& nigori = *it->second;
     sync_pb::NigoriKey* key = bag.add_key();
     key->set_name(it->first);
-    nigori.ExportKeys(key->mutable_user_key(), key->mutable_encryption_key(),
-                      key->mutable_mac_key());
+    nigori.ExportKeys(key->mutable_encryption_key(), key->mutable_mac_key());
   }
 
   // Encrypt the bag with the default Nigori.
@@ -306,8 +305,7 @@ void Cryptographer::InstallKeyBag(const sync_pb::NigoriKeyBag& bag) {
     // Only use this key if we don't already know about it.
     if (nigoris_.end() == nigoris_.find(key.name())) {
       std::unique_ptr<Nigori> new_nigori(new Nigori);
-      if (!new_nigori->InitByImport(key.user_key(), key.encryption_key(),
-                                    key.mac_key())) {
+      if (!new_nigori->InitByImport(key.encryption_key(), key.mac_key())) {
         NOTREACHED();
         continue;
       }
@@ -348,8 +346,7 @@ std::string Cryptographer::GetDefaultNigoriKeyData() const {
   if (iter == nigoris_.end())
     return std::string();
   sync_pb::NigoriKey key;
-  if (!iter->second->ExportKeys(key.mutable_user_key(),
-                                key.mutable_encryption_key(),
+  if (!iter->second->ExportKeys(key.mutable_encryption_key(),
                                 key.mutable_mac_key()))
     return std::string();
   return key.SerializeAsString();
@@ -364,8 +361,7 @@ bool Cryptographer::ImportNigoriKey(const std::string& serialized_nigori_key) {
     return false;
 
   std::unique_ptr<Nigori> nigori(new Nigori);
-  if (!nigori->InitByImport(key.user_key(), key.encryption_key(),
-                            key.mac_key())) {
+  if (!nigori->InitByImport(key.encryption_key(), key.mac_key())) {
     NOTREACHED();
     return false;
   }

components/sync/base/nigori.cc

@@ -79,12 +79,6 @@ bool Nigori::InitByDerivation(const std::string& hostname,
   if (!user_salt->GetRawKey(&raw_user_salt))
     return false;
 
-  // Kuser = PBKDF2(P, Suser, Nuser, 16)
-  user_key_ = SymmetricKey::DeriveKeyFromPassword(
-      SymmetricKey::AES, password, raw_user_salt, kUserIterations,
-      kDerivedKeySizeInBits);
-  DCHECK(user_key_);
-
   // Kenc = PBKDF2(P, Suser, Nenc, 16)
   encryption_key_ = SymmetricKey::DeriveKeyFromPassword(
       SymmetricKey::AES, password, raw_user_salt, kEncryptionIterations,
@@ -97,14 +91,11 @@ bool Nigori::InitByDerivation(const std::string& hostname,
       kDerivedKeySizeInBits);
   DCHECK(mac_key_);
 
-  return user_key_ && encryption_key_ && mac_key_;
+  return encryption_key_ && mac_key_;
 }
 
-bool Nigori::InitByImport(const std::string& user_key,
-                          const std::string& encryption_key,
+bool Nigori::InitByImport(const std::string& encryption_key,
                           const std::string& mac_key) {
-  user_key_ = SymmetricKey::Import(SymmetricKey::AES, user_key);
-
   encryption_key_ = SymmetricKey::Import(SymmetricKey::AES, encryption_key);
   DCHECK(encryption_key_);
 
@@ -232,14 +223,11 @@ bool Nigori::Decrypt(const std::string& encrypted, std::string* value) const {
   return true;
 }
 
-bool Nigori::ExportKeys(std::string* user_key,
-                        std::string* encryption_key,
+bool Nigori::ExportKeys(std::string* encryption_key,
                         std::string* mac_key) const {
   DCHECK(encryption_key);
   DCHECK(mac_key);
 
-  user_key_->GetRawKey(user_key);
-
   return encryption_key_->GetRawKey(encryption_key) &&
          mac_key_->GetRawKey(mac_key);
 }

components/sync/base/nigori.h

@@ -41,8 +41,7 @@ class Nigori {
 
   // Initialize the client by importing the given keys instead of deriving new
   // ones.
-  bool InitByImport(const std::string& user_key,
-                    const std::string& encryption_key,
+  bool InitByImport(const std::string& encryption_key,
                     const std::string& mac_key);
 
   // Derives a secure lookup name from |type| and |name|. If |hostname|,
@@ -60,9 +59,7 @@ class Nigori {
   bool Decrypt(const std::string& value, std::string* decrypted) const;
 
   // Exports the raw derived keys.
-  bool ExportKeys(std::string* user_key,
-                  std::string* encryption_key,
-                  std::string* mac_key) const;
+  bool ExportKeys(std::string* encryption_key, std::string* mac_key) const;
 
   static const char kSaltSalt[];  // The salt used to derive the user salt.
   static const size_t kSaltKeySizeInBits = 128;
@@ -71,16 +68,10 @@ class Nigori {
   static const size_t kHashSize = 32;
 
   static const size_t kSaltIterations = 1001;
-  static const size_t kUserIterations = 1002;
   static const size_t kEncryptionIterations = 1003;
   static const size_t kSigningIterations = 1004;
 
  private:
-  // user_key isn't used any more, but legacy clients will fail to import a
-  // nigori node without one. We preserve it for the sake of those clients, but
-  // it should be removed once enough clients have upgraded to code that doesn't
-  // enforce its presence.
-  std::unique_ptr<crypto::SymmetricKey> user_key_;
   std::unique_ptr<crypto::SymmetricKey> encryption_key_;
   std::unique_ptr<crypto::SymmetricKey> mac_key_;
 };

components/sync/base/nigori_unittest.cc

@@ -126,13 +126,12 @@ TEST(SyncNigoriTest, ExportImport) {
   Nigori nigori1;
   EXPECT_TRUE(nigori1.InitByDerivation("example.com", "username", "password"));
 
-  std::string user_key;
   std::string encryption_key;
   std::string mac_key;
-  EXPECT_TRUE(nigori1.ExportKeys(&user_key, &encryption_key, &mac_key));
+  EXPECT_TRUE(nigori1.ExportKeys(&encryption_key, &mac_key));
 
   Nigori nigori2;
-  EXPECT_TRUE(nigori2.InitByImport(user_key, encryption_key, mac_key));
+  EXPECT_TRUE(nigori2.InitByImport(encryption_key, mac_key));
 
   std::string original("test");
   std::string plaintext;
@@ -152,30 +151,5 @@ TEST(SyncNigoriTest, ExportImport) {
   EXPECT_EQ(permuted1, permuted2);
 }
 
-TEST(SyncNigoriTest, InitByDerivationSetsUserKey) {
-  Nigori nigori;
-  EXPECT_TRUE(nigori.InitByDerivation("example.com", "username", "password"));
-
-  std::string user_key = "";
-  std::string encryption_key;
-  std::string mac_key;
-  EXPECT_TRUE(nigori.ExportKeys(&user_key, &encryption_key, &mac_key));
-
-  EXPECT_NE(user_key, "");
-}
-
-TEST(SyncNigoriTest, InitByImportToleratesEmptyUserKey) {
-  Nigori nigori1;
-  EXPECT_TRUE(nigori1.InitByDerivation("example.com", "username", "password"));
-
-  std::string user_key;
-  std::string encryption_key;
-  std::string mac_key;
-  EXPECT_TRUE(nigori1.ExportKeys(&user_key, &encryption_key, &mac_key));
-
-  Nigori nigori2;
-  EXPECT_TRUE(nigori2.InitByImport("", encryption_key, mac_key));
-}
-
 }  // anonymous namespace
 }  // namespace syncer

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -216,8 +216,7 @@ class ModelTypeWorkerTest : public ::testing::Test {
       sync_pb::NigoriKey* key = bag.add_key();
 
       key->set_name(GetNigoriName(nigori));
-      nigori.ExportKeys(key->mutable_user_key(), key->mutable_encryption_key(),
-                        key->mutable_mac_key());
+      nigori.ExportKeys(key->mutable_encryption_key(), key->mutable_mac_key());
     }
 
     // Re-create the last nigori from that loop.