Addressing first round of comments:

* Added comments to the code and moved some methods around to imporve
  readability.
* Added a test to show that BouncCastle recognizes PKCS7Padding but
  SunJCE does not.

.github/workflows/macos_ci_jobs.yml

@@ -22,12 +22,9 @@ jobs:
     - uses: actions/checkout@v3
       with:
         submodules: true
-    - name: Install lcov golang
+    - name: Install dependencies from brew
       run: |
-        env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov golang
-    - name: Install clang-format
-      run: |
-        env HOMEBREW_NO_AUTO_UPDATE=1 brew install clang-format
+        env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov golang clang-format
     - name: Setup pip
       uses: actions/setup-python@v4
       with:
@@ -70,12 +67,9 @@ jobs:
     - uses: actions/checkout@v3
       with:
         submodules: true
-    - name: Install lcov golang
-      run: |
-        env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov golang
-    - name: Install clang-format
+    - name: Install dependencies from brew
       run: |
-        env HOMEBREW_NO_AUTO_UPDATE=1 brew install clang-format
+        env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov golang clang-format
     - name: Setup pip
       uses: actions/setup-python@v4
       with:
@@ -107,12 +101,9 @@ jobs:
     - uses: actions/checkout@v3
       with:
         submodules: true
-    - name: Install lcov golang
-      run: |
-        env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov golang
-    - name: Install clang-format
+    - name: Install dependencies from brew
       run: |
-        env HOMEBREW_NO_AUTO_UPDATE=1 brew install clang-format
+        env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov golang clang-format
     - name: Setup pip
       uses: actions/setup-python@v4
       with:
@@ -142,7 +133,7 @@ jobs:
     - uses: actions/checkout@v3
       with:
         submodules: true
-    - name: Install lcov clang-format golang
+    - name: Install dependencies from brew
       run: |
         env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov clang-format golang
     - name: Setup pip
@@ -187,7 +178,7 @@ jobs:
     - uses: actions/checkout@v3
       with:
         submodules: true
-    - name: Install lcov clang-format golang
+    - name: Install dependencies from brew
       run: |
         env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov clang-format golang
     - name: Setup pip
@@ -221,7 +212,7 @@ jobs:
     - uses: actions/checkout@v3
       with:
         submodules: true
-    - name: Install lcov clang-format golang
+    - name: Install lcov dependencies from brew
       run: |
         env HOMEBREW_NO_AUTO_UPDATE=1 brew install lcov clang-format golang
     - name: Setup pip

README.md

@@ -30,10 +30,14 @@ Mac algorithms:
 
 Cipher algorithms:
 * AES/GCM/NoPadding
-* AES_128/GCM/NoPadding
-* AES_256/GCM/NoPadding
+* AES_\<n\>/GCM/NoPadding, where n can be 128, or 256
 * AES/KWP/NoPadding
 * AES/XTS/NoPadding
+* AES/CBC/NoPadding
+  * AES_\<n\>/CBC/NoPadding, where n can be 128, 192, or 256
+* AES/CBC/PKCS5Padding
+  * AES_\<n\>/CBC/PKCS5Padding, where n can be 128, 192, or 256
+  * PKCS7Padding is also accepted with AES/CBC and it is treated the same as PKCS5.
 * RSA/ECB/NoPadding
 * RSA/ECB/PKCS1Padding
 * RSA/ECB/OAEPPadding

csrc/aes_cbc.cpp

@@ -56,19 +56,31 @@ class AesCbcCipher {
         , ctx_(reinterpret_cast<EVP_CIPHER_CTX*>(ctx_ptr))
         , own_ctx_(!save_ctx)
     {
+        if (ctx_ != nullptr) {
+            // if there is a context, we don't need to do anything.
+            return;
+        }
+
+        // There is no context, so we need to create one.
+        ctx_ = EVP_CIPHER_CTX_new();
         if (ctx_ == nullptr) {
-            // There is no context, so we need to create one.
-            ctx_ = EVP_CIPHER_CTX_new();
-            if (ctx_ == nullptr) {
-                throw_openssl(EX_RUNTIME_CRYPTO, "EVP_CIPHER_CTX_new failed.");
-            }
+            throw_openssl(EX_RUNTIME_CRYPTO, "EVP_CIPHER_CTX_new failed.");
+        }
 
-            if (!own_ctx_) {
-                // We need to return the context.
-                jlong tmpPtr = reinterpret_cast<jlong>(ctx_);
-                jenv_->SetLongArrayRegion(ctx_container, 0, 1, &tmpPtr);
-            }
+        if (own_ctx_) {
+            // Since we should own the context, there is no need to return the context to the caller.
+            return;
         }
+
+        // We need to return the context.
+        if (ctx_container == nullptr) {
+            // This should not happen. We ensure this at the call sites.
+            EVP_CIPHER_CTX_free(ctx_);
+            throw java_ex(EX_ERROR, "THIS SHOULD NOT BE REACHABLE. No container is provided to return the context.");
+        }
+
+        jlong tmpPtr = reinterpret_cast<jlong>(ctx_);
+        jenv_->SetLongArrayRegion(ctx_container, 0, 1, &tmpPtr);
     }
 
     ~AesCbcCipher()
@@ -260,7 +272,6 @@ extern "C" JNIEXPORT jint JNICALL Java_com_amazon_corretto_crypto_provider_AesCb
 
 extern "C" JNIEXPORT jint JNICALL Java_com_amazon_corretto_crypto_provider_AesCbcSpi_nUpdateFinal(JNIEnv* env,
     jclass,
-    jlongArray ctxContainer,
     jlong ctxPtr,
     jboolean saveCtx,
     jobject inputDirect,
@@ -273,7 +284,7 @@ extern "C" JNIEXPORT jint JNICALL Java_com_amazon_corretto_crypto_provider_AesCb
     jint outputOffset)
 {
     try {
-        AesCbcCipher aes_cbc_cipher(env, ctxContainer, ctxPtr, saveCtx);
+        AesCbcCipher aes_cbc_cipher(env, nullptr, ctxPtr, saveCtx);
 
         int result = 0;
 

src/com/amazon/corretto/crypto/provider/AesCbcSpi.java

@@ -42,7 +42,9 @@ class AesCbcSpi extends CipherSpi {
     AES_CBC_PKCS7_PADDING_NAMES.add("AES_128/CBC/PKCS7Padding".toLowerCase());
     AES_CBC_PKCS7_PADDING_NAMES.add("AES_192/CBC/PKCS7Padding".toLowerCase());
     AES_CBC_PKCS7_PADDING_NAMES.add("AES_256/CBC/PKCS7Padding".toLowerCase());
-    // PKCS5Padding with AES/CBC should be treated as PKCS7Padding
+    // PKCS5Padding with AES/CBC must be treated as PKCS7Padding. PKCS7Padding name is not
+    // recognized by SunJCE, but BouncyCastle supports PKCS7Padding as a valid name for the same
+    // padding.
     AES_CBC_PKCS7_PADDING_NAMES.add("AES/CBC/PKCS5Padding".toLowerCase());
     AES_CBC_PKCS7_PADDING_NAMES.add("AES_128/CBC/PKCS5Padding".toLowerCase());
     AES_CBC_PKCS7_PADDING_NAMES.add("AES_192/CBC/PKCS5Padding".toLowerCase());
@@ -114,6 +116,10 @@ protected int engineGetBlockSize() {
   protected int engineGetOutputSize(final int inputLen) {
     // There is no need to check if the Cipher is initialized since
     // javax.crypto.Cipher::getOutputSize checks that.
+
+    // This method cannot assume if the next operation is going to be engineUpdate or engineDoFinal.
+    // We provide separate methods to find the output length for engineUpdates and engineDoFinals to
+    // avoid over allocation and alignment checking of input.
     final long all = inputLen + unprocessedInput;
 
     final long rem = all % BLOCK_SIZE_IN_BYTES;
@@ -209,7 +215,7 @@ private static byte[] checkAesCbcIv(final AlgorithmParameterSpec params)
       throws InvalidAlgorithmParameterException {
     if (!(params instanceof IvParameterSpec)) {
       throw new InvalidAlgorithmParameterException(
-          "I don't know how to handle a " + params.getClass());
+          "Unknown AlgorithmParameterSpec: " + params.getClass());
     }
 
     final IvParameterSpec ivParameterSpec = (IvParameterSpec) params;
@@ -224,11 +230,12 @@ private static byte[] checkAesCbcIv(final AlgorithmParameterSpec params)
   @Override
   protected byte[] engineUpdate(final byte[] input, final int inputOffset, final int inputLen) {
     Utils.checkArrayLimits(input, inputOffset, inputLen);
+    // Since we allocate the output's memory, we only check if the cipher is in the correct state.
     finalOrUpdateStateCheck();
-    final byte[] result = new byte[getOutputSizeUpdate(inputLen)];
-    update(null, input, inputOffset, inputLen, null, result, 0);
     // For update, getOutputSizeUpdate returns the exact required size, therefore there is no need
     // for trimming the result;
+    final byte[] result = new byte[getOutputSizeUpdate(inputLen)];
+    update(null, input, inputOffset, inputLen, null, result, 0);
     return result;
   }
 
@@ -381,56 +388,36 @@ private int update(
     }
   }
 
-  private static native int nInitUpdate(
-      int opMode,
-      int padding,
-      byte[] key,
-      int keyLen,
-      byte[] iv,
-      long[] ctxContainer,
-      long ctxPtr,
-      ByteBuffer inputDirect,
-      byte[] inputArray,
-      int inputOffset,
-      int inputLen,
-      ByteBuffer outputDirect,
-      byte[] outputArray,
-      int outputOffset);
-
-  private static native int nUpdate(
-      long ctxPtr,
-      ByteBuffer inputDirect,
-      byte[] inputArray,
-      int inputOffset,
-      int inputLen,
-      int unprocessedInput,
-      ByteBuffer outputDirect,
-      byte[] outputArray,
-      int outputOffset);
-
   @Override
-  protected byte[] engineDoFinal(byte[] input, int inputOffset, int inputLen)
+  protected byte[] engineDoFinal(final byte[] input, final int inputOffset, final int inputLen)
       throws IllegalBlockSizeException, BadPaddingException {
-    Utils.checkArrayLimits(emptyIfNull(input), inputOffset, inputLen);
+    final byte[] inputNotNull = emptyIfNull(input);
+    Utils.checkArrayLimits(inputNotNull, inputOffset, inputLen);
+    // Since we allocate the output's memory, we only check if the cipher is in the correct state.
     finalOrUpdateStateCheck();
     final byte[] result = new byte[getOutputSizeFinal(inputLen)];
-    final int resultLen = doFinal(null, emptyIfNull(input), inputOffset, inputLen, null, result, 0);
+    final int resultLen = doFinal(null, inputNotNull, inputOffset, inputLen, null, result, 0);
     return resultLen == result.length ? result : Arrays.copyOf(result, resultLen);
   }
 
   @Override
   protected int engineDoFinal(
-      byte[] input, int inputOffset, int inputLen, byte[] output, int outputOffset)
+      final byte[] input,
+      final int inputOffset,
+      final int inputLen,
+      final byte[] output,
+      final int outputOffset)
       throws ShortBufferException, IllegalBlockSizeException, BadPaddingException {
-    Utils.checkArrayLimits(emptyIfNull(input), inputOffset, inputLen);
+    final byte[] inputNotNull = emptyIfNull(input);
+    Utils.checkArrayLimits(inputNotNull, inputOffset, inputLen);
     Utils.checkArrayLimits(output, outputOffset, output.length - outputOffset);
     finalChecks(inputLen, output.length - outputOffset);
 
-    return doFinal(null, emptyIfNull(input), inputOffset, inputLen, null, output, outputOffset);
+    return doFinal(null, inputNotNull, inputOffset, inputLen, null, output, outputOffset);
   }
 
   @Override
-  protected int engineDoFinal(ByteBuffer input, ByteBuffer output)
+  protected int engineDoFinal(final ByteBuffer input, final ByteBuffer output)
       throws ShortBufferException, IllegalBlockSizeException, BadPaddingException {
     finalChecks(input.remaining(), output.remaining());
 
@@ -463,6 +450,8 @@ private void finalChecks(final int inputLen, final int outputLen)
     }
   }
 
+  // This method is used when calling engineDoFinal to ensure that output is large enough and the
+  // input is aligned with block size if needed.
   private int getOutputSizeFinal(final int inputLen) throws IllegalBlockSizeException {
     final long all = ((long) inputLen) + ((long) unprocessedInput);
     final long rem = all % BLOCK_SIZE_IN_BYTES;
@@ -551,7 +540,6 @@ private int doFinal(
               nativeCtx.use(
                   ctxPtr ->
                       nUpdateFinal(
-                          null,
                           ctxPtr,
                           true,
                           inputDirect,
@@ -565,6 +553,8 @@ private int doFinal(
         }
       } else {
         // Don't need to save the context
+        final long ctxPtr = nativeCtx == null ? 0 : nativeCtx.take();
+        nativeCtx = null;
         if (cipherState == CipherState.INITIALIZED) {
           result =
               nInitUpdateFinal(
@@ -574,7 +564,7 @@ private int doFinal(
                   key.length,
                   iv,
                   null,
-                  nativeCtx == null ? 0 : nativeCtx.take(),
+                  ctxPtr,
                   false,
                   inputDirect,
                   inputArray,
@@ -587,8 +577,7 @@ private int doFinal(
           // Cipher is in UPDATE state and don't need to save the context
           result =
               nUpdateFinal(
-                  null,
-                  nativeCtx.take(),
+                  ctxPtr,
                   false,
                   inputDirect,
                   inputArray,
@@ -599,7 +588,6 @@ private int doFinal(
                   outputArray,
                   outputOffset);
         }
-        nativeCtx = null;
       }
 
       cipherState = CipherState.INITIALIZED;
@@ -609,11 +597,7 @@ private int doFinal(
 
     } catch (final Exception e) {
       cipherState = CipherState.NEEDS_INITIALIZATION;
-      if (saveContext) {
-        saveNativeContextIfNeeded(ctxContainer[0]);
-      } else {
-        nativeCtx = null;
-      }
+      saveNativeContextIfNeeded(ctxContainer[0]);
       throw e;
     }
   }
@@ -624,6 +608,12 @@ private void saveNativeContextIfNeeded(final long ctxPtr) {
     }
   }
 
+  // We have four JNI calls. Their names start with the letter n, followed by the operations that
+  // they perform on the underlying EVP_CIPHER_CTX. For example, nInitUpdate calls init and update
+  // on the context.
+
+  // This method is used for one-shot operations, when engineDoFinal is invoked immediately after
+  // engineInit.
   private static native int nInitUpdateFinal(
       int opMode,
       int padding,
@@ -641,8 +631,37 @@ private static native int nInitUpdateFinal(
       byte[] outputArray,
       int outputOffset);
 
-  private static native int nUpdateFinal(
+  // This method is used the first time engineUpdate is used in a multi-step operation.
+  private static native int nInitUpdate(
+      int opMode,
+      int padding,
+      byte[] key,
+      int keyLen,
+      byte[] iv,
       long[] ctxContainer,
+      long ctxPtr,
+      ByteBuffer inputDirect,
+      byte[] inputArray,
+      int inputOffset,
+      int inputLen,
+      ByteBuffer outputDirect,
+      byte[] outputArray,
+      int outputOffset);
+
+  // This method is used the n^th time engineUpdate is used in a multi-step operation, where n >= 2.
+  private static native int nUpdate(
+      long ctxPtr,
+      ByteBuffer inputDirect,
+      byte[] inputArray,
+      int inputOffset,
+      int inputLen,
+      int unprocessedInput,
+      ByteBuffer outputDirect,
+      byte[] outputArray,
+      int outputOffset);
+
+  // This method is used  when engineDoFinal is used to finalize a multi-step operation.
+  private static native int nUpdateFinal(
       long ctxPtr,
       boolean saveCtx,
       ByteBuffer inputDirect,

src/com/amazon/corretto/crypto/provider/NativeEvpCipherCtx.java

@@ -2,6 +2,10 @@
 // SPDX-License-Identifier: Apache-2.0
 package com.amazon.corretto.crypto.provider;
 
+/**
+ * This class is used in AesGcmSpi and AesCbcSpi to ensure EVP_CIPHER_CTX that is associated to a
+ * Cipher object is properly cleaned.
+ */
 final class NativeEvpCipherCtx extends NativeResource {
   NativeEvpCipherCtx(final long ptr) {
     super(ptr, Utils::releaseEvpCipherCtx);