[Sync] Cryptographer::DecryptToString() should return boolean

This CL changes Cryptographer::DecryptToString() to return a boolean
instead of an empty string to signal failure/success.

Change-Id: I12f1f65a177a23648a83d522900fa5c99f60ff4c
Reviewed-on: https://chromium-review.googlesource.com/c/1350617
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611635}

components/sync/base/cryptographer.cc

@@ -98,8 +98,9 @@ bool Cryptographer::Encrypt(const ::google::protobuf::MessageLite& message,
 bool Cryptographer::EncryptString(const std::string& serialized,
                                   sync_pb::EncryptedData* encrypted) const {
   if (CanDecryptUsingDefaultKey(*encrypted)) {
-    const std::string& original_serialized = DecryptToString(*encrypted);
-    if (original_serialized == serialized) {
+    std::string original_serialized;
+    if (DecryptToString(*encrypted, &original_serialized) &&
+        original_serialized == serialized) {
       DVLOG(2) << "Re-encryption unnecessary, encrypted data already matches.";
       return true;
     }
@@ -122,28 +123,29 @@ bool Cryptographer::EncryptString(const std::string& serialized,
 bool Cryptographer::Decrypt(const sync_pb::EncryptedData& encrypted,
                             ::google::protobuf::MessageLite* message) const {
   DCHECK(message);
-  std::string plaintext = DecryptToString(encrypted);
+  std::string plaintext;
+  if (!DecryptToString(encrypted, &plaintext)) {
+    return false;
+  }
   return message->ParseFromString(plaintext);
 }
 
-std::string Cryptographer::DecryptToString(
-    const sync_pb::EncryptedData& encrypted) const {
-  // TODO(mamir): DecryptToString() should return a boolean to signal failure
-  // instead of an empty string.
+bool Cryptographer::DecryptToString(const sync_pb::EncryptedData& encrypted,
+                                    std::string* decrypted) const {
+  decrypted->clear();
   auto it = nigoris_.find(encrypted.key_name());
   if (nigoris_.end() == it) {
     // The key used to encrypt the blob is not part of the set of installed
     // nigoris.
     LOG(ERROR) << "Cannot decrypt message";
-    return std::string();
+    return false;
   }
 
-  std::string plaintext;
-  if (!it->second->Decrypt(encrypted.blob(), &plaintext)) {
-    return std::string();
+  if (!it->second->Decrypt(encrypted.blob(), decrypted)) {
+    return false;
   }
 
-  return plaintext;
+  return true;
 }
 
 bool Cryptographer::GetKeys(sync_pb::EncryptedData* encrypted) const {

components/sync/base/cryptographer.h

@@ -100,9 +100,10 @@ class Cryptographer {
   bool Decrypt(const sync_pb::EncryptedData& encrypted,
                ::google::protobuf::MessageLite* message) const;
 
-  // Decrypts |encrypted| and returns plaintext decrypted data. If decryption
-  // fails, returns empty string.
-  std::string DecryptToString(const sync_pb::EncryptedData& encrypted) const;
+  // Decrypts |encrypted| as a plaintext decrypted data in |decrypted|. If
+  // decryption fails, returns false otherwise returns true.
+  bool DecryptToString(const sync_pb::EncryptedData& encrypted,
+                       std::string* decrypted) const;
 
   // Encrypts the set of currently known keys into |encrypted|. Returns true if
   // successful.

components/sync/engine/engine_util.cc

@@ -90,7 +90,9 @@ bool AreSpecificsEqual(const Cryptographer* cryptographer,
       NOTREACHED() << "Attempting to compare undecryptable data.";
       return false;
     }
-    left_plaintext = cryptographer->DecryptToString(left.encrypted());
+    if (!cryptographer->DecryptToString(left.encrypted(), &left_plaintext)) {
+      return false;
+    }
   } else {
     left_plaintext = left.SerializeAsString();
   }
@@ -99,7 +101,9 @@ bool AreSpecificsEqual(const Cryptographer* cryptographer,
       NOTREACHED() << "Attempting to compare undecryptable data.";
       return false;
     }
-    right_plaintext = cryptographer->DecryptToString(right.encrypted());
+    if (!cryptographer->DecryptToString(right.encrypted(), &right_plaintext)) {
+      return false;
+    }
   } else {
     right_plaintext = right.SerializeAsString();
   }

components/sync/engine_impl/conflict_resolver.cc

@@ -113,8 +113,9 @@ void ConflictResolver::ProcessSimpleConflict(syncable::WriteTransaction* trans,
     bool server_encrypted_with_default_key = false;
     if (specifics.has_encrypted()) {
       DCHECK(cryptographer->CanDecryptUsingDefaultKey(specifics.encrypted()));
-      decrypted_specifics =
-          cryptographer->DecryptToString(specifics.encrypted());
+      // TODO(crbug.com/908391): what if the decryption below fails?
+      cryptographer->DecryptToString(specifics.encrypted(),
+                                     &decrypted_specifics);
     } else {
       decrypted_specifics = specifics.SerializeAsString();
     }
@@ -122,8 +123,9 @@ void ConflictResolver::ProcessSimpleConflict(syncable::WriteTransaction* trans,
       server_encrypted_with_default_key =
           cryptographer->CanDecryptUsingDefaultKey(
               server_specifics.encrypted());
-      decrypted_server_specifics =
-          cryptographer->DecryptToString(server_specifics.encrypted());
+      // TODO(crbug.com/908391): what if the decryption below fails?
+      cryptographer->DecryptToString(server_specifics.encrypted(),
+                                     &decrypted_server_specifics);
     } else {
       decrypted_server_specifics = server_specifics.SerializeAsString();
     }
@@ -139,8 +141,9 @@ void ConflictResolver::ProcessSimpleConflict(syncable::WriteTransaction* trans,
         decrypted_base_server_specifics =
             base_server_specifics.SerializeAsString();
       } else {
-        decrypted_base_server_specifics =
-            cryptographer->DecryptToString(base_server_specifics.encrypted());
+        // TODO(crbug.com/908391): what if the decryption below fails?
+        cryptographer->DecryptToString(base_server_specifics.encrypted(),
+                                       &decrypted_base_server_specifics);
       }
       if (decrypted_server_specifics == decrypted_base_server_specifics)
         base_server_specifics_match = true;

components/sync/engine_impl/model_type_worker.cc

@@ -684,13 +684,8 @@ bool ModelTypeWorker::DecryptSpecifics(const Cryptographer& cryptographer,
   DCHECK(in.has_encrypted());
   DCHECK(cryptographer.CanDecrypt(in.encrypted()));
 
-  std::string plaintext = cryptographer.DecryptToString(in.encrypted());
-  if (plaintext.empty()) {
-    DLOG(ERROR) << "Failed to decrypt a decryptable entity";
-    return false;
-  }
-  if (!out->ParseFromString(plaintext)) {
-    DLOG(ERROR) << "Failed to parse decrypted entity";
+  if (!cryptographer.Decrypt(in.encrypted(), out)) {
+    DLOG(ERROR) << "Failed to decrypt a decryptable specifics";
     return false;
   }
   return true;
@@ -704,7 +699,7 @@ bool ModelTypeWorker::DecryptPasswordSpecifics(
   DCHECK(in.has_password());
   DCHECK(in.password().has_encrypted());
   DCHECK(cryptographer.CanDecrypt(in.password().encrypted()));
-  // TODO(mamir): unify implementation with DecryptSpecifics() above.
+
   if (!cryptographer.Decrypt(
           in.password().encrypted(),
           out->mutable_password()->mutable_client_only_encrypted_data())) {

components/sync/engine_impl/sync_encryption_handler_impl.cc

@@ -1864,8 +1864,10 @@ bool SyncEncryptionHandlerImpl::DecryptPendingKeysWithKeystoreKey(
     // ensure we re-encrypt using the newest key.
     DVLOG(1) << "Attempting to decrypt pending keys using "
              << "keystore decryptor token.";
-    std::string serialized_nigori =
-        temp_cryptographer.DecryptToString(keystore_decryptor_token);
+    std::string serialized_nigori;
+    // TODO(crbug.com/908391): what if the decryption below fails?
+    temp_cryptographer.DecryptToString(keystore_decryptor_token,
+                                       &serialized_nigori);
 
     // This will decrypt the pending keys and add them if possible. The key
     // within |serialized_nigori| will be the default after.

components/sync/syncable/base_node.cc

@@ -85,9 +85,9 @@ bool BaseNode::DecryptIfNecessary() {
   }
 
   const sync_pb::EncryptedData& encrypted = specifics.encrypted();
-  std::string plaintext_data =
-      GetTransaction()->GetCryptographer()->DecryptToString(encrypted);
-  if (plaintext_data.length() == 0) {
+  std::string plaintext_data;
+  if (!GetTransaction()->GetCryptographer()->DecryptToString(encrypted,
+                                                             &plaintext_data)) {
     GetTransaction()->GetWrappedTrans()->OnUnrecoverableError(
         FROM_HERE, std::string("Failed to decrypt encrypted node of type ") +
                        ModelTypeToString(GetModelType()));

components/sync/syncable/delete_journal.cc

@@ -31,10 +31,10 @@ void DeleteJournal::GetBookmarkDeleteJournals(
     if (!specifics.has_encrypted()) {
       delete_journal_list->back().specifics = specifics;
     } else {
-      std::string plaintext_data =
-          trans->GetCryptographer()->DecryptToString(specifics.encrypted());
+      std::string plaintext_data;
       sync_pb::EntitySpecifics unencrypted_data;
-      if (plaintext_data.length() == 0 ||
+      if (!trans->GetCryptographer()->DecryptToString(specifics.encrypted(),
+                                                      &plaintext_data) ||
           !unencrypted_data.ParseFromString(plaintext_data)) {
         // Fail to decrypt, Add this delete journal to purge.
         undecryptable_journal.insert(delete_journal_list->back().id);