[Sync] Update Nigori::Encrypt() to indicate it never fails

Encryption success doesn't depend on input (except extreme corner cases
when input is way too long - but this would probably cause OOM crash).

Keys themselves are either derived in the sane way or imported and
verified, so corrupted keys shouldn't be possible.

Besides that, there are more generic reasons to fail, like OOM inside
BoringSSL. It seems reasonable to crash in such cases and perhaps even
safer to do so.

This CL updates Encrypt() method to use CHECK() inside and return
encrypted output directly (instead of using output parameter with
boolean result).

Bug: 1368018
Change-Id: I65dba67731ede1c0edd2682245d74768d6cfca4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4008281
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/main@{#1069066}

components/sync/driver/sync_service_crypto_unittest.cc

@@ -60,7 +60,7 @@ sync_pb::EncryptedData MakeEncryptedData(
   const std::string unencrypted = "test";
   sync_pb::EncryptedData encrypted;
   encrypted.set_key_name(nigori_name);
-  EXPECT_TRUE(nigori->Encrypt(unencrypted, encrypted.mutable_blob()));
+  encrypted.set_blob(nigori->Encrypt(unencrypted));
   return encrypted;
 }
 

components/sync/engine/nigori/nigori.cc

@@ -235,33 +235,30 @@ bool Nigori::Permute(Type type,
 }
 
 // Enc[Kenc,Kmac](value)
-bool Nigori::Encrypt(const std::string& value, std::string* encrypted) const {
+std::string Nigori::Encrypt(const std::string& value) const {
   std::string iv;
   crypto::RandBytes(base::WriteInto(&iv, kIvSize + 1), kIvSize);
 
   crypto::Encryptor encryptor;
-  if (!encryptor.Init(keys_.encryption_key.get(), crypto::Encryptor::CBC, iv))
-    return false;
+  CHECK(encryptor.Init(keys_.encryption_key.get(), crypto::Encryptor::CBC, iv));
 
   std::string ciphertext;
-  if (!encryptor.Encrypt(value, &ciphertext))
-    return false;
+  CHECK(encryptor.Encrypt(value, &ciphertext));
 
   HMAC hmac(HMAC::SHA256);
-  if (!hmac.Init(keys_.mac_key->key()))
-    return false;
+  CHECK(hmac.Init(keys_.mac_key->key()));
 
   std::vector<unsigned char> hash(kHashSize);
-  if (!hmac.Sign(ciphertext, &hash[0], hash.size()))
-    return false;
+  CHECK(hmac.Sign(ciphertext, &hash[0], hash.size()));
 
   std::string output;
   output.assign(iv);
   output.append(ciphertext);
   output.append(hash.begin(), hash.end());
 
-  Base64Encode(output, encrypted);
-  return true;
+  std::string base64_encoded_output;
+  Base64Encode(output, &base64_encoded_output);
+  return base64_encoded_output;
 }
 
 bool Nigori::Decrypt(const std::string& encrypted, std::string* value) const {

components/sync/engine/nigori/nigori.h

@@ -60,9 +60,8 @@ class Nigori {
   // Base64 encoded.
   bool Permute(Type type, const std::string& name, std::string* permuted) const;
 
-  // Encrypts |value|. Note that on success, |encrypted| will be Base64
-  // encoded.
-  bool Encrypt(const std::string& value, std::string* encrypted) const;
+  // Encrypts |value|. Note that the returned value is Base64 encoded.
+  std::string Encrypt(const std::string& value) const;
 
   // Decrypts |value| into |decrypted|. It is assumed that |value| is Base64
   // encoded.

components/sync/engine/nigori/nigori_unittest.cc

@@ -83,15 +83,8 @@ TEST(SyncNigoriTest, EncryptDifferentIv) {
       KeyDerivationParams::CreateForPbkdf2(), "password");
   ASSERT_THAT(nigori, NotNull());
 
-  std::string plaintext("value");
-
-  std::string encrypted1;
-  EXPECT_TRUE(nigori->Encrypt(plaintext, &encrypted1));
-
-  std::string encrypted2;
-  EXPECT_TRUE(nigori->Encrypt(plaintext, &encrypted2));
-
-  EXPECT_NE(encrypted1, encrypted2);
+  const std::string plaintext("value");
+  EXPECT_NE(nigori->Encrypt(plaintext), nigori->Encrypt(plaintext));
 }
 
 TEST(SyncNigoriTest, Decrypt) {
@@ -115,13 +108,10 @@ TEST(SyncNigoriTest, EncryptDecrypt) {
       KeyDerivationParams::CreateForPbkdf2(), "password");
   ASSERT_THAT(nigori, NotNull());
 
-  std::string plaintext("value");
-
-  std::string encrypted;
-  EXPECT_TRUE(nigori->Encrypt(plaintext, &encrypted));
+  const std::string plaintext("value");
 
   std::string decrypted;
-  EXPECT_TRUE(nigori->Decrypt(encrypted, &decrypted));
+  EXPECT_TRUE(nigori->Decrypt(nigori->Encrypt(plaintext), &decrypted));
 
   EXPECT_EQ(plaintext, decrypted);
 }
@@ -131,13 +121,10 @@ TEST(SyncNigoriTest, EncryptDecryptEmptyString) {
       KeyDerivationParams::CreateForPbkdf2(), "password");
   ASSERT_THAT(nigori, NotNull());
 
-  std::string plaintext;
-
-  std::string encrypted;
-  EXPECT_TRUE(nigori->Encrypt(plaintext, &encrypted));
+  const std::string plaintext;
 
   std::string decrypted;
-  EXPECT_TRUE(nigori->Decrypt(encrypted, &decrypted));
+  EXPECT_TRUE(nigori->Decrypt(nigori->Encrypt(plaintext), &decrypted));
 
   EXPECT_EQ(plaintext, decrypted);
 }
@@ -147,10 +134,9 @@ TEST(SyncNigoriTest, CorruptedIv) {
       KeyDerivationParams::CreateForPbkdf2(), "password");
   ASSERT_THAT(nigori, NotNull());
 
-  std::string plaintext("test");
+  const std::string plaintext("test");
 
-  std::string encrypted;
-  EXPECT_TRUE(nigori->Encrypt(plaintext, &encrypted));
+  std::string encrypted = nigori->Encrypt(plaintext);
 
   // Corrupt the IV by changing one of its byte.
   encrypted[0] = (encrypted[0] == 'a' ? 'b' : 'a');
@@ -166,10 +152,9 @@ TEST(SyncNigoriTest, CorruptedCiphertext) {
       KeyDerivationParams::CreateForPbkdf2(), "password");
   ASSERT_THAT(nigori, NotNull());
 
-  std::string plaintext("test");
+  const std::string plaintext("test");
 
-  std::string encrypted;
-  EXPECT_TRUE(nigori->Encrypt(plaintext, &encrypted));
+  std::string encrypted = nigori->Encrypt(plaintext);
 
   // Corrput the ciphertext by changing one of its bytes.
   encrypted[Nigori::kIvSize + 10] =
@@ -199,12 +184,10 @@ TEST(SyncNigoriTest, ExportImport) {
   std::string plaintext;
   std::string ciphertext;
 
-  EXPECT_TRUE(nigori1->Encrypt(original, &ciphertext));
-  EXPECT_TRUE(nigori2->Decrypt(ciphertext, &plaintext));
+  EXPECT_TRUE(nigori2->Decrypt(nigori1->Encrypt(original), &plaintext));
   EXPECT_EQ(original, plaintext);
 
-  EXPECT_TRUE(nigori2->Encrypt(original, &ciphertext));
-  EXPECT_TRUE(nigori1->Decrypt(ciphertext, &plaintext));
+  EXPECT_TRUE(nigori1->Decrypt(nigori2->Encrypt(original), &plaintext));
   EXPECT_EQ(original, plaintext);
 
   std::string permuted1, permuted2;

components/sync/nigori/nigori_key_bag.cc

@@ -147,15 +147,12 @@ bool NigoriKeyBag::EncryptWithKey(
   DCHECK(encrypted_output);
   DCHECK(HasKey(key_name));
 
-  encrypted_output->Clear();
-
-  if (!nigori_map_.find(key_name)->second->Encrypt(
-          input, encrypted_output->mutable_blob())) {
-    DLOG(ERROR) << "Failed to encrypt data.";
-    return false;
-  }
-
+  encrypted_output->set_blob(
+      nigori_map_.find(key_name)->second->Encrypt(input));
   encrypted_output->set_key_name(key_name);
+
+  // TODO(crbug.com/1368018): returned value is always true, update interface
+  // to return void or `encrypted_output`.
   return true;
 }
 

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -95,7 +95,7 @@ MATCHER_P(CanDecryptWith, key_params, "") {
   const std::string unencrypted = "test";
   sync_pb::EncryptedData encrypted;
   encrypted.set_key_name(nigori_name);
-  EXPECT_TRUE(nigori->Encrypt(unencrypted, encrypted.mutable_blob()));
+  encrypted.set_blob(nigori->Encrypt(unencrypted));
 
   if (!cryptographer.CanDecrypt(encrypted)) {
     return false;