commit 00b6f5a7b5516877b3d0dd79fad7f39faeded699
author Maksim Ivanov <emaxx@chromium.org> Thu Sep 09 21:34:51 2021 +0200
committer Commit Bot <commit-bot@chromium.org> Tue Sep 14 13:45:16 2021 +0000
tree 53ac4b2126a2736a20dd990f3db3f8bd4c1f6738
parent 73c74f4b558b7d57fe559533529c5c721bece479

cryptohome: Refuse empty plain-/ciphertexts in AES

Change the AES encryption/decryption helpers to bail out when the
supplied plaintext/ciphertext is empty. It seems to most likely indicate
a bug, since such encryption/decryption is effectively a no-op.

Also emit a fatal failure in Debug builds, so that such mistakes are
more visible on test bots. Additionally do the same for other existing
DCHECKs in the AES helpers, i.e., logging + crashing in Debug builds and
logging + returning false in Release ones.

BUG=none
TEST=cros_workon_make --board=$BOARD --test cryptohome

Change-Id: I6e93b086b702c0cf2bdb71618bd571dd1bf33cfe
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3151841
Tested-by: Maksim Ivanov <emaxx@chromium.org>
Tested-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Betul Soysal <betuls@google.com>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Auto-Submit: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>

cryptohome/crypto/aes.cc

diff --git a/cryptohome/crypto/aes.cc b/cryptohome/crypto/aes.cc
index 78fea77..3d04288 100644
--- a/cryptohome/crypto/aes.cc
+++ b/cryptohome/crypto/aes.cc
@@ -13,6 +13,7 @@
 #include "cryptohome/crypto/secure_blob_util.h"
 
 #include <base/logging.h>
+#include <base/notreached.h>
 
 namespace cryptohome {
 
@@ -94,9 +95,25 @@
                    const brillo::SecureBlob& key,
                    const brillo::SecureBlob& iv,
                    brillo::SecureBlob* plaintext) {
-  DCHECK_EQ(key.size(), kAesGcm256KeySize);
-  DCHECK_EQ(iv.size(), kAesGcmIVSize);
-  DCHECK_EQ(tag.size(), kAesGcmTagSize);
+  if (ciphertext.empty()) {
+    NOTREACHED() << "Empty ciphertext passed to AesGcmDecrypt.";
+    return false;
+  }
+  if (tag.size() != kAesGcmTagSize) {
+    NOTREACHED() << "Wrong tag size passed to AesGcmDecrypt: " << tag.size()
+                 << ", expected " << kAesGcmTagSize << ".";
+    return false;
+  }
+  if (key.size() != kAesGcm256KeySize) {
+    NOTREACHED() << "Wrong key size passed to AesGcmDecrypt: " << key.size()
+                 << ", expected " << kAesGcm256KeySize << ".";
+    return false;
+  }
+  if (iv.size() != kAesGcmIVSize) {
+    NOTREACHED() << "Wrong iv size passed to AesGcmDecrypt: " << iv.size()
+                 << ", expected " << kAesGcmIVSize << ".";
+    return false;
+  }
 
   crypto::ScopedEVP_CIPHER_CTX ctx(EVP_CIPHER_CTX_new());
   if (ctx.get() == nullptr) {
@@ -123,7 +140,10 @@
   }
 
   if (ad.has_value()) {
-    DCHECK_NE(ad.value().size(), 0);
+    if (ad.value().empty()) {
+      NOTREACHED() << "Empty associated data passed to AesGcmDecrypt.";
+      return false;
+    }
     int out_size = 0;
     if (EVP_DecryptUpdate(ctx.get(), nullptr, &out_size, ad.value().data(),
                           ad.value().size()) != 1) {
@@ -171,7 +191,15 @@
                    brillo::SecureBlob* iv,
                    brillo::SecureBlob* tag,
                    brillo::SecureBlob* ciphertext) {
-  DCHECK_EQ(key.size(), kAesGcm256KeySize);
+  if (plaintext.empty()) {
+    NOTREACHED() << "Empty plaintext passed to AesGcmEncrypt.";
+    return false;
+  }
+  if (key.size() != kAesGcm256KeySize) {
+    NOTREACHED() << "Wrong key size passed to AesGcmEncrypt: " << key.size()
+                 << ", expected " << kAesGcm256KeySize << ".";
+    return false;
+  }
 
   iv->resize(kAesGcmIVSize);
   GetSecureRandom(iv->data(), kAesGcmIVSize);
@@ -201,7 +229,10 @@
   }
 
   if (ad.has_value()) {
-    DCHECK_NE(ad.value().size(), 0);
+    if (ad.value().empty()) {
+      NOTREACHED() << "Empty associated data passed to AesGcmEncrypt.";
+      return false;
+    }
     int out_size = 0;
     if (EVP_EncryptUpdate(ctx.get(), nullptr, &out_size, ad.value().data(),
                           ad.value().size()) != 1) {