Fix read routine decryption, increase flush failure covg

crypto/pkcs7/bio/bio_deprecated_test.cc

@@ -9,8 +9,8 @@
 #include <openssl/rand.h>
 #include <openssl/x509.h>
 
-#include "../internal.h"
 #include "../../test/test_util.h"
+#include "../internal.h"
 
 // NOTE: need to keep these in sync with cipher BIO source file
 #define ENC_MIN_CHUNK_SIZE 256
@@ -36,7 +36,8 @@ static const struct CipherParams Ciphers[] = {
 
 class BIODeprecatedTest : public testing::TestWithParam<CipherParams> {};
 
-INSTANTIATE_TEST_SUITE_P(PKCS7Test, BIODeprecatedTest, testing::ValuesIn(Ciphers),
+INSTANTIATE_TEST_SUITE_P(PKCS7Test, BIODeprecatedTest,
+                         testing::ValuesIn(Ciphers),
                          [](const testing::TestParamInfo<CipherParams> &params)
                              -> std::string { return params.param.name; });
 
@@ -244,15 +245,23 @@ TEST_P(BIODeprecatedTest, Cipher) {
     // Write to |bio_cipher| should still succeed in writing up to
     // ENC_BLOCK_SIZE bytes by buffering them
     wsize = BIO_write(bio_cipher.get(), buff, io_size);
+    // First write succeeds due to write buffering up to |ENC_BLOCK_SIZE| bytes
+    if (io_size >= ENC_BLOCK_SIZE) {
+      EXPECT_EQ(ENC_BLOCK_SIZE, wsize);
+    } else {
+      EXPECT_GT(ENC_BLOCK_SIZE, wsize);
+    }
     if (wsize > 0) {
       pt_vec.insert(pt_vec.end(), pos, pos + wsize);
       pos += wsize;
     }
-    EXPECT_GT(wsize, 0);
-    EXPECT_LE(wsize, ENC_BLOCK_SIZE);
+    // Buffer is full, so second write fails
+    EXPECT_FALSE(BIO_write(bio_cipher.get(), pt, sizeof(pt)));
+    // Writes still disabled, so flush fails
+    EXPECT_FALSE(BIO_flush(bio_cipher.get()));
     // Now that there's buffered data, |BIO_wpending| should match
     EXPECT_EQ((size_t)wsize, BIO_wpending(bio_cipher.get()));
-    // Renable writes
+    // Re-enable writes
     BIO_set_callback_ex(bio_mem.get(), nullptr);
     BIO_clear_retry_flags(bio_mem.get());
     if (wsize < io_size) {
@@ -307,3 +316,4 @@ TEST_P(BIODeprecatedTest, Cipher) {
     bio_mem.release();  // |bio_cipher| took ownership
   }
 }
+

crypto/pkcs7/bio/cipher.c

@@ -1,7 +1,6 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0 OR ISC
 
-#include <errno.h>
 #include <openssl/buffer.h>
 #include <openssl/crypto.h>
 #include <openssl/evp.h>
@@ -99,20 +98,27 @@ static int enc_read(BIO *b, char *out, int outl) {
 
   int bytes_processed = 0;
   int remaining = outl;
+  const int cipher_block_size = EVP_CIPHER_CTX_block_size(ctx->cipher);
   while (remaining > 0) {
     if (ctx->buf_len > 0) {
       int bytes_decrypted;
       int to_decrypt = remaining > ctx->buf_len ? ctx->buf_len : remaining;
-      char *out_pos = out + bytes_processed;
-      if (!EVP_DecryptUpdate(ctx->cipher, (uint8_t *)out_pos, &bytes_decrypted,
+      uint8_t *out_pos = ((uint8_t *)out) + bytes_processed;
+      // |EVP_DecryptUpdate| may write up to cipher_block_size-1 more bytes than
+      // requested, so return early if we cannot accommodate that with current
+      // |remaining| byte count.
+      if ((to_decrypt > (remaining - cipher_block_size + 1)) ||
+          !EVP_DecryptUpdate(ctx->cipher, out_pos, &bytes_decrypted,
                              &ctx->buf[ctx->buf_off], to_decrypt)) {
         BIO_copy_next_retry(b);
         return bytes_processed;
       };
-      ctx->buf_len -= bytes_decrypted;
-      ctx->buf_off += bytes_decrypted;
-      bytes_processed += bytes_decrypted;
-      remaining -= bytes_decrypted;
+      // Update buffer info and counters with number of bytes processed from our
+      // buffer.
+      ctx->buf_len -= to_decrypt;
+      ctx->buf_off += to_decrypt;
+      bytes_processed += to_decrypt;
+      remaining -= to_decrypt;
       continue;
     }
     assert(ctx->buf_len == 0);
@@ -171,7 +177,7 @@ static int enc_write(BIO *b, const char *in, int inl) {
     return 0;
   }
 
-  if ((ret = enc_flush(b, next, ctx, /*do_final*/ 0)) < 0) {
+  if ((ret = enc_flush(b, next, ctx, /*do_final*/ 0)) <= 0) {
     return ret;
   }
 
@@ -198,7 +204,7 @@ static int enc_write(BIO *b, const char *in, int inl) {
     ctx->buf_off += outcome;
     ctx->buf_len -= outcome;
   }
-  out:
+out:
   if (ret <= 0) {
     BIO_copy_next_retry(b);
   }

crypto/pkcs7/internal.h

@@ -207,7 +207,8 @@ OPENSSL_EXPORT const BIO_METHOD *BIO_f_cipher(void);
 // BIO_set_cipher is used internally for testing. It is not recommended for
 // external use.
 OPENSSL_EXPORT int BIO_set_cipher(BIO *b, const EVP_CIPHER *cipher,
-                     const unsigned char *key, const unsigned char *iv, int enc);
+                                  const unsigned char *key,
+                                  const unsigned char *iv, int enc);
 
 
 #if defined(__cplusplus)