Fix read routine decryption, increase flush failure covg

crypto/pkcs7/bio/bio_deprecated_test.cc

@@ -244,15 +244,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) {
@@ -306,4 +314,4 @@ TEST_P(BIODeprecatedTest, Cipher) {
               Bytes(decrypted_pt_vec.data(), decrypted_pt_vec.size()));
     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;
   }