Allow empty strings in crypto::Encryptor

This CHECK dates to https://codereview.chromium.org/8418034 and, rather
than being anything about cryptography, was some quirk about
base::WriteInto which no longer applies.

Encrypting the empty string with CTR is perfectly valid. You just get
the empty string back out. Decrypting the empty string with CBC, under
this file's padding construction, will always fail, but that shouldn't
be a CHECK failure, just a clean decryption failure like any other bad
ciphertext.

(EVP_CIPHER does have some weird edge cases around NULL pointers, but
none apply to CBC. To be safe, I've added tests for all these.)

Bug: none
Change-Id: Ia33e0cf2d81f7ab0f3dc7ded61496d6fa06404dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3689714
Commit-Queue: David Benjamin <davidben@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1011474}

crypto/encryptor.cc

@@ -64,24 +64,20 @@ bool Encryptor::Init(const SymmetricKey* key,
 }
 
 bool Encryptor::Encrypt(base::StringPiece plaintext, std::string* ciphertext) {
-  CHECK(!plaintext.empty() || mode_ == CBC);
   return CryptString(/*do_encrypt=*/true, plaintext, ciphertext);
 }
 
 bool Encryptor::Encrypt(base::span<const uint8_t> plaintext,
                         std::vector<uint8_t>* ciphertext) {
-  CHECK(!plaintext.empty() || mode_ == CBC);
   return CryptBytes(/*do_encrypt=*/true, plaintext, ciphertext);
 }
 
 bool Encryptor::Decrypt(base::StringPiece ciphertext, std::string* plaintext) {
-  CHECK(!ciphertext.empty());
   return CryptString(/*do_encrypt=*/false, ciphertext, plaintext);
 }
 
 bool Encryptor::Decrypt(base::span<const uint8_t> ciphertext,
                         std::vector<uint8_t>* plaintext) {
-  CHECK(!ciphertext.empty());
   return CryptBytes(/*do_encrypt=*/false, ciphertext, plaintext);
 }
 
@@ -102,18 +98,13 @@ bool Encryptor::SetCounter(base::span<const uint8_t> counter) {
 bool Encryptor::CryptString(bool do_encrypt,
                             base::StringPiece input,
                             std::string* output) {
-  size_t out_size = MaxOutput(do_encrypt, input.size());
-  CHECK_GT(out_size + 1, out_size);  // Overflow
-  std::string result;
-  uint8_t* out_ptr =
-      reinterpret_cast<uint8_t*>(base::WriteInto(&result, out_size + 1));
-
+  std::string result(MaxOutput(do_encrypt, input.size()), '\0');
   absl::optional<size_t> len =
       (mode_ == CTR)
           ? CryptCTR(do_encrypt, base::as_bytes(base::make_span(input)),
-                     base::make_span(out_ptr, out_size))
+                     base::as_writable_bytes(base::make_span(result)))
           : Crypt(do_encrypt, base::as_bytes(base::make_span(input)),
-                  base::make_span(out_ptr, out_size));
+                  base::as_writable_bytes(base::make_span(result)));
   if (!len)
     return false;
 

crypto/encryptor_unittest.cc

@@ -9,6 +9,7 @@
 #include <memory>
 #include <string>
 
+#include "base/containers/span.h"
 #include "base/strings/string_number_conversions.h"
 #include "crypto/symmetric_key.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -466,7 +467,7 @@ TEST(EncryptorTest, UnsupportedIV) {
   EXPECT_FALSE(encryptor.Init(sym_key.get(), crypto::Encryptor::CBC, iv));
 }
 
-TEST(EncryptorTest, EmptyEncrypt) {
+TEST(EncryptorTest, EmptyEncryptCBC) {
   std::string key = "128=SixteenBytes";
   std::string iv = "Sweet Sixteen IV";
   std::string plaintext;
@@ -477,7 +478,7 @@ TEST(EncryptorTest, EmptyEncrypt) {
   ASSERT_TRUE(sym_key.get());
 
   crypto::Encryptor encryptor;
-  // The IV must be exactly as long a the cipher block size.
+  // The IV must be exactly as long as the cipher block size.
   EXPECT_EQ(16U, iv.size());
   EXPECT_TRUE(encryptor.Init(sym_key.get(), crypto::Encryptor::CBC, iv));
 
@@ -485,6 +486,64 @@ TEST(EncryptorTest, EmptyEncrypt) {
   EXPECT_TRUE(encryptor.Encrypt(plaintext, &ciphertext));
   EXPECT_EQ(expected_ciphertext_hex, base::HexEncode(ciphertext.data(),
                                                      ciphertext.size()));
+
+  std::string decrypted;
+  EXPECT_TRUE(encryptor.Decrypt(ciphertext, &decrypted));
+  EXPECT_EQ(decrypted, plaintext);
+
+  // Decrypting the empty string should fail. Our formulation of CBC expects a
+  // full block of padding for CBC.
+  EXPECT_FALSE(encryptor.Decrypt(std::string(), &decrypted));
+
+  // Repeat the test with the byte-based API.
+  EXPECT_TRUE(encryptor.Init(sym_key.get(), crypto::Encryptor::CBC, iv));
+  std::vector<uint8_t> ciphertext_bytes;
+  EXPECT_TRUE(
+      encryptor.Encrypt(base::span<const uint8_t>(), &ciphertext_bytes));
+  EXPECT_EQ(expected_ciphertext_hex, base::HexEncode(ciphertext_bytes));
+
+  std::vector<uint8_t> decrypted_bytes;
+  EXPECT_TRUE(encryptor.Decrypt(ciphertext_bytes, &decrypted_bytes));
+  EXPECT_EQ(decrypted_bytes.size(), 0u);
+
+  // Decrypting the empty string should fail. Our formulation of CBC expects a
+  // full block of padding for CBC.
+  EXPECT_FALSE(
+      encryptor.Decrypt(base::span<const uint8_t>(), &decrypted_bytes));
+}
+
+TEST(EncryptorTest, EmptyEncryptCTR) {
+  std::string key = "128=SixteenBytes";
+  std::string iv = "Sweet Sixteen IV";
+  std::string plaintext;
+  std::string expected_ciphertext;
+
+  std::unique_ptr<crypto::SymmetricKey> sym_key(
+      crypto::SymmetricKey::Import(crypto::SymmetricKey::AES, key));
+  ASSERT_TRUE(sym_key.get());
+
+  crypto::Encryptor encryptor;
+  EXPECT_TRUE(encryptor.Init(sym_key.get(), crypto::Encryptor::CTR, ""));
+  ASSERT_TRUE(encryptor.SetCounter(iv));
+
+  std::string ciphertext;
+  EXPECT_TRUE(encryptor.Encrypt(plaintext, &ciphertext));
+  EXPECT_EQ(expected_ciphertext, ciphertext);
+
+  std::string decrypted;
+  EXPECT_TRUE(encryptor.Decrypt(ciphertext, &decrypted));
+  EXPECT_EQ(decrypted, plaintext);
+
+  // Repeat the test with the byte-based API.
+  ASSERT_TRUE(encryptor.SetCounter(iv));
+  std::vector<uint8_t> ciphertext_bytes;
+  EXPECT_TRUE(
+      encryptor.Encrypt(base::span<const uint8_t>(), &ciphertext_bytes));
+  EXPECT_EQ(ciphertext_bytes.size(), 0u);
+
+  std::vector<uint8_t> decrypted_bytes;
+  EXPECT_TRUE(encryptor.Decrypt(base::span<const uint8_t>(), &decrypted_bytes));
+  EXPECT_EQ(decrypted_bytes.size(), 0u);
 }
 
 TEST(EncryptorTest, CipherTextNotMultipleOfBlockSize) {