[sync::nigori] Read/write key derivation method from/to NigoriSpecifics

Add support for the key derivation method field in the NigoriSpecifics
proto in SyncEncryptionHandlerImpl. Persist the key derivation method
chosen for new passphrases in NigoriSpecifics. If updating from a remote
NigoriSpecifics, respect the key derivation method stored there. The
change is limited to cases when the passphrase type is
CUSTOM_PASSPHRASE.

A rationale for the change follows.

Prerequisite: if the custom passphrase key derivation method is set to
PBKDF2 (or UNSPECIFIED), the behavior of all decryption is exactly the
same as in previous versions.

Let "happy paths" be the encryption and decryption code paths which a
keystore-enabled client (referring to a client released after the
support for keystore migrations was introduced) will be able to hit
starting from a fresh account, if only other keystore-enabled clients
are syncing to that account.  Since legacy encryption methods are out of
scope for the change, we need to change the behavior only in these
"happy paths", and keep the behavior identical in others.

In ApplyNigoriUpdateImpl, we will read the key derivation method from
Nigori and save it in the class member.

In "happy paths", we will always know what key derivation method will be
used, and we will always know when we'll end up in a CUSTOM_PASSPHRASE
state. We will remember the key derivation method in the class as a
member when applicable. In particular, there are two such paths. Both
are short-circuits that are much easier to follow than the alternative,
legacy code paths:

1. |IsNigoriMigratedToKeystore() == true| case in
SetEncryptionPassphrase(), which calls SetCustomPassphrase() (only call
site). Here, it is us who are making the decision, so this is obvious.

2. |IsNigoriMigratedToKeystore() == true && ...| case in
SetDecryptionPassphrase(), which calls
DecryptPendingKeysWithExplicitPassphrase() (only call site). Here, we
will use the key derivation method from the member. Since we can't reach
this path in SetDecryptionPassphrase() without a valid Nigori node, we
assume that ApplyNigoriUpdateImpl() was called and has saved the key
derivation method from Nigori in the member.

Since we write the passphrase type to Nigori *only* in
AttemptToMigrateNigoriToKeystore, we will also write the custom
passphrase key derivation method *only* there. If the key derivation
method member is not set at that point (but the passphrase type we are
writing is CUSTOM_PASSPHRASE), that means we have reached the
CUSTOM_PASSPHRASE state through some other, non-"happy" path. In this
case, we will write PBKDF2 as the key derivation method. This will
ensure that decryption for these cases works the same way as in older
versions, no matter what.

The only thing that remains is converting an UNSPECIFIED key derivation
method in Nigori to PBKDF2. We will also do this in
ApplyNigoriUpdateImpl(), which is called when we receive a remote
Nigori. Because we expect to set the key derivation method explicitly
whenever we decide what it is, this will ensure that we use every chance
we can to set an explicit (rather than UNSPECIFIED) key derivation
method in Nigori.

Bug: 876408
Change-Id: Ia793f53f8777ed61af5edda9d8fa21090ad6da43
Reviewed-on: https://chromium-review.googlesource.com/1187148
Commit-Queue: David Davidović <davidovic@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587611}

components/sync/base/cryptographer.cc

@@ -24,12 +24,6 @@ const char kNigoriTag[] = "google_chrome_nigori";
 // assign the same name to a particular triplet.
 const char kNigoriKeyName[] = "nigori-key";
 
-KeyParams::KeyParams()
-    : derivation_method(KeyDerivationMethod::UNKNOWN),
-      hostname(),
-      username(),
-      password() {}
-
 KeyParams::KeyParams(KeyDerivationMethod derivation_method,
                      const std::string& hostname,
                      const std::string& username,

components/sync/base/cryptographer.h

@@ -26,7 +26,6 @@ extern const char kNigoriTag[];
 
 // The parameters used to initialize a Nigori instance.
 struct KeyParams {
-  KeyParams();
   KeyParams(KeyDerivationMethod derivation_method,
             const std::string& hostname,
             const std::string& username,

components/sync/base/passphrase_enums.cc

@@ -68,7 +68,7 @@ KeyDerivationMethod ProtoKeyDerivationMethodToEnum(
 
   // We do not know about this value. It is likely a method added in a newer
   // version of Chrome.
-  return KeyDerivationMethod::UNKNOWN;
+  return KeyDerivationMethod::UNSUPPORTED;
 }
 
 sync_pb::NigoriSpecifics::KeyDerivationMethod EnumKeyDerivationMethodToProto(
@@ -76,7 +76,7 @@ sync_pb::NigoriSpecifics::KeyDerivationMethod EnumKeyDerivationMethodToProto(
   switch (method) {
     case KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003:
       return sync_pb::NigoriSpecifics::PBKDF2_HMAC_SHA1_1003;
-    case KeyDerivationMethod::UNKNOWN:
+    case KeyDerivationMethod::UNSUPPORTED:
       // This value does not have a counterpart in the protocol proto enum,
       // because it is just a client side abstraction.
       break;

components/sync/base/passphrase_enums.h

@@ -31,17 +31,17 @@ sync_pb::NigoriSpecifics::PassphraseType EnumPassphraseTypeToProto(
 // user's custom passphrase.
 enum class KeyDerivationMethod {
   PBKDF2_HMAC_SHA1_1003 = 0,  // PBKDF2-HMAC-SHA1 with 1003 iterations.
-  UNKNOWN = 1,                // Unknown method, likely from a future version.
+  UNSUPPORTED = 1,  // Unsupported method, likely from a future version.
 };
 
 // This function accepts an integer and not KeyDerivationMethod from the proto
 // in order to be able to handle new, unknown values.
 KeyDerivationMethod ProtoKeyDerivationMethodToEnum(
     ::google::protobuf::int32 method);
-// Note that KeyDerivationMethod::UNKNOWN is an invalid input for this function,
-// since it corresponds to a method that we are not aware of and so cannot
-// meaningfully convert. The caller is responsible for ensuring that
-// KeyDerivationMethod::UNKNOWN is never passed to this function.
+// Note that KeyDerivationMethod::UNSUPPORTED is an invalid input for this
+// function, since it corresponds to a method that we are not aware of and so
+// cannot meaningfully convert. The caller is responsible for ensuring that
+// KeyDerivationMethod::UNSUPPORTED is never passed to this function.
 sync_pb::NigoriSpecifics::KeyDerivationMethod EnumKeyDerivationMethodToProto(
     KeyDerivationMethod method);
 

components/sync/driver/sync_service_crypto.cc

@@ -112,13 +112,20 @@ bool CheckPassphraseAgainstPendingKeys(
     const std::string& passphrase) {
   DCHECK(pending_keys.has_blob());
   DCHECK(!passphrase.empty());
+  if (key_derivation_method == KeyDerivationMethod::UNSUPPORTED) {
+    DLOG(ERROR) << "Cannot derive keys using an unsupported key derivation "
+                   "method. Rejecting passphrase.";
+    return false;
+  }
+
   Nigori nigori;
-  nigori.InitByDerivation(key_derivation_method, "localhost", "dummy",
-                          passphrase);
+  bool derivation_result = nigori.InitByDerivation(
+      key_derivation_method, "localhost", "dummy", passphrase);
+  DCHECK(derivation_result);
   std::string plaintext;
-  bool result = nigori.Decrypt(pending_keys.blob(), &plaintext);
-  DVLOG_IF(1, !result) << "Passphrase failed to decrypt pending keys.";
-  return result;
+  bool decrypt_result = nigori.Decrypt(pending_keys.blob(), &plaintext);
+  DVLOG_IF(1, !decrypt_result) << "Passphrase failed to decrypt pending keys.";
+  return decrypt_result;
 }
 
 }  // namespace
@@ -130,7 +137,8 @@ SyncServiceCrypto::SyncServiceCrypto(
     : notify_observers_(notify_observers),
       reconfigure_(reconfigure),
       sync_prefs_(sync_prefs),
-      passphrase_key_derivation_method_(KeyDerivationMethod::UNKNOWN),
+      passphrase_key_derivation_method_(
+          KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003),
       weak_factory_(this) {
   DCHECK(notify_observers_);
   DCHECK(reconfigure_);
@@ -210,6 +218,13 @@ bool SyncServiceCrypto::SetDecryptionPassphrase(const std::string& passphrase) {
   // This should only be called when we have cached pending keys.
   DCHECK(cached_pending_keys_.has_blob());
 
+  // For types other than CUSTOM_PASSPHRASE, we should be using the old PBKDF2
+  // key derivation method.
+  if (cached_passphrase_type_ != PassphraseType::CUSTOM_PASSPHRASE) {
+    DCHECK_EQ(passphrase_key_derivation_method_,
+              KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003);
+  }
+
   // Check the passphrase that was provided against our local cache of the
   // cryptographer's pending keys (which we cached during a previous
   // OnPassphraseRequired event). If this was unsuccessful, the UI layer can

components/sync/driver/sync_service_crypto.h

@@ -144,9 +144,8 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
   // The key derivation method for the passphrase. We save it when we receive a
   // passphrase required event, as it is a necessary piece of information to be
   // able to properly perform a decryption attempt, and we want to be able to
-  // synchronously do that from the UI thread. This is needed only for custom
-  // passphrase users, as for all other passphrase types we will only ever
-  // receive PBKDF2 as the key derivation method.
+  // synchronously do that from the UI thread. For passphrase types other than
+  // CUSTOM_PASSPHRASE, this will always be PBKDF2.
   KeyDerivationMethod passphrase_key_derivation_method_;
 
   // If an explicit passphrase is in use, the time at which the passphrase was

components/sync/engine/sync_string_conversions.cc

@@ -61,7 +61,7 @@ const char* BootstrapTokenTypeToString(BootstrapTokenType type) {
 const char* KeyDerivationMethodToString(KeyDerivationMethod method) {
   switch (method) {
     ENUM_CASE(KeyDerivationMethod::PBKDF2_HMAC_SHA1_1003);
-    ENUM_CASE(KeyDerivationMethod::UNKNOWN);
+    ENUM_CASE(KeyDerivationMethod::UNSUPPORTED);
   }
 
   NOTREACHED();

components/sync/engine_impl/sync_encryption_handler_impl.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "base/sequence_checker.h"
 #include "base/time/time.h"
 #include "components/sync/base/cryptographer.h"
@@ -176,6 +177,17 @@ class SyncEncryptionHandlerImpl : public SyncEncryptionHandler,
   bool UpdateEncryptedTypesFromNigori(const sync_pb::NigoriSpecifics& nigori,
                                       syncable::BaseTransaction* const trans);
 
+  // If the Nigori node doesn't contain an explicit custom passphrase key
+  // derivation method, it means it was committed with a previous version
+  // which was unaware of this field and implicitly used PBKDF2. This method
+  // checks for this condition and explicitly writes PBKDF2 as the key
+  // derivation method.
+  void ReplaceImplicitKeyDerivationMethodInNigori(WriteTransaction* trans);
+
+  // Same as ReplaceImplicitKeyDerivationMethodInNigori, just
+  // wrapped in a write transaction.
+  void ReplaceImplicitKeyDerivationMethodInNigoriWithTransaction();
+
   // TODO(zea): make these public and have them replace SetEncryptionPassphrase
   // and SetDecryptionPassphrase.
   // Helper methods for handling passphrases once keystore migration has taken
@@ -185,8 +197,7 @@ class SyncEncryptionHandlerImpl : public SyncEncryptionHandler,
   // is not already set.
   // Triggers OnPassphraseAccepted on success, OnPassphraseRequired if a custom
   // passphrase already existed.
-  void SetCustomPassphrase(KeyDerivationMethod key_derivation_method,
-                           const std::string& passphrase,
+  void SetCustomPassphrase(const std::string& passphrase,
                            WriteTransaction* trans,
                            WriteNode* nigori_node);
   // Decrypt the encryption keybag using a user provided passphrase.
@@ -194,7 +205,6 @@ class SyncEncryptionHandlerImpl : public SyncEncryptionHandler,
   // passphrase or a custom passphrase.
   // Triggers OnPassphraseAccepted on success, OnPassphraseRequired on failure.
   void DecryptPendingKeysWithExplicitPassphrase(
-      KeyDerivationMethod key_derivation_method,
       const std::string& passphrase,
       WriteTransaction* trans,
       WriteNode* nigori_node);
@@ -209,10 +219,8 @@ class SyncEncryptionHandlerImpl : public SyncEncryptionHandler,
   // |is_explicit|: used to differentiate between a custom passphrase (true) and
   //                a GAIA passphrase that is implicitly used for encryption
   //                (false).
-  // |trans| and |nigori_node|: used to access data in the cryptographer.
   void FinishSetPassphrase(bool success,
                            const std::string& bootstrap_token,
-                           KeyDerivationMethod key_derivation_method,
                            WriteTransaction* trans,
                            WriteNode* nigori_node);
 
@@ -317,6 +325,11 @@ class SyncEncryptionHandlerImpl : public SyncEncryptionHandler,
   // before support for this field was added.
   base::Time custom_passphrase_time_;
 
+  // The key derivation method we are using for the custom passphrase. This can
+  // end up not being set e.g. in cases when we reach a CUSTOM_PASSPHRASE state
+  // through a legacy code path.
+  base::Optional<KeyDerivationMethod> custom_passphrase_key_derivation_method_;
+
   base::WeakPtrFactory<SyncEncryptionHandlerImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncEncryptionHandlerImpl);