AES-CBC with ISO10126 Padding #381

amirhosv · 2024-05-09T13:37:29Z

Description of changes:

AES-CBC with ISO10126 Padding

AWS-LC does not support ISO10126.
The padding and unpadding are handled in ACCP.
The padding is explained here: https://www.w3.org/TR/xmlenc-core1/#sec-Padding

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

+ AWS-LC does not support ISO10126. + The padding and unpadding are handled in ACCP. + The padding is explained here: https://www.w3.org/TR/xmlenc-core1/#sec-Padding

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

csrc/aes_cbc.cpp

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

csrc/aes_cbc.cpp

geedo0 · 2024-05-09T20:28:37Z

csrc/aes_cbc.cpp

+        }
+        // Record the new size of last_block.
+        // Since c <= 16, the following cast is fine.
+        last_block.get()[AES_CBC_BLOCK_SIZE_IN_BYTES] = (uint8_t)c;


I am not following how the last byte in the last_block is guaranteed to not be data. It seems like you're always holding onto the modulo of the last block (safe) or the last complete block in the case that we're aligned (possible data loss).

The comments in this section are expanded in the next commit.

Oh... Now I get it, are we using this extra byte at the end of this last-block array as a secret variable that stores the current length stored in the intermediate last-block scratch buffer? That state really needs to be tracked elsewhere, that's really unclear.

I hope the added comments help now.

The comments help, but I'd still prefer this tracked elsewhere like as a class member and/or separate variable if possible. This is really unconventional and liable to turn into nasty bugs in the future. I won't push further than this and I'll defer to the rest of the team for whether we want to push for a refactor.

It can be passed as a separate variable but it needs to be wrapped in something like an array. This value is read and written to. Having it as an integer or a primitive type that gets passed from Java to C++ is not going to work.

geedo0 · 2024-05-09T20:46:53Z

I've done a partial pass, here's the feedback so far.

alexw91 · 2024-05-09T22:27:43Z

csrc/aes_cbc.cpp

+    return padding == com_amazon_corretto_crypto_provider_AesCbcSpi_ISO10126_PADDING;
+}
+
+static bool is_enc(jint op_mode) { return op_mode == com_amazon_corretto_crypto_provider_AesCbcSpi_ENC_MODE; }


Nit: It's easier to read if function names use complete words. Maybe is_encrypt_mode() or is_encryption_mode()?

Sure. I'll fix it in the next commit.

alexw91 · 2024-05-09T22:46:45Z

tst/com/amazon/corretto/crypto/provider/test/AesCbcIso10126Test.java

+    // Since the padding is random, the last block of the cipher texts are not necessarily the same.
+    for (int i = 0; i < accpCipherText.length - 16; i++) {
+      assertEquals(sunCipherText[i], accpCipherText[i]);
+    }


Shouldn't the final byte be the same though? Can you add a check for that?

The padding is randomized and that's why the final blocks are not necessarily the same.

WShat Alex is saying is that since the last byte is the padding length, it should be the same when given the same plaintext.

The length of the padding, which is stored as the last byte of input, is the same in padded plain texts, but then it gets encrypted as part of the last block and it's not predictable what AES encryption produces for the last byte in the Cipher text.

This also makes causing bad padding exception hard. One could modify the cipher text or use a different key, but somehow the last byte of the unpadded plain text ends up being a number less than or equal to 16.

One thing that I can add is to use AES-CBC with NoPadding and decrypt both sunCipherText and accpCipherText that results in unpadded plain texts: in this case the last bytes will be the same.

alexw91 · 2024-05-09T23:01:05Z

csrc/aes_cbc.cpp

@@ -110,7 +126,7 @@ class AesCbcCipher {
        }

        // This method always returns 1 and succeeds.
-        if (EVP_CIPHER_CTX_set_padding(ctx_, padding) != 1) {
+        if (EVP_CIPHER_CTX_set_padding(ctx_, padding_to_be_used(padding)) != 1) {


Would something like this be simpler? Otherwise the reader needs to read into 2 nested functions to figure out what is going on.

if (padding == com_amazon_corretto_crypto_provider_AesCbcSpi_ISO10126_PADDING) { /* AWS-LC doesn't support ISO10126 padding so ensure AWS-LC does not perform any padding so we can do it ourselves. */ padding = com_amazon_corretto_crypto_provider_AesCbcSpi_NO_PADDING; } if (EVP_CIPHER_CTX_set_padding(ctx_, padding) != 1) { ...

WillChilds-Klein · 2024-05-09T19:48:51Z

csrc/aes_cbc.cpp

+        // The last_block is used to store this information.
+        JBinaryBlob last_block(jenv_, nullptr, j_last_block);
+        // The last byte of last_block stores the number of bytes in it from the previous call.
+        int last_block_len = last_block.get()[AES_CBC_BLOCK_SIZE_IN_BYTES];


isn't this one past the end of the array? should it be AES_CBC_BLOCK_SIZE_IN_BYTES-1?

I've added more comments that describes why this is not an issue.

geedo0 · 2024-05-10T13:20:11Z

csrc/aes_cbc.cpp

+        }
+        // Record the new size of last_block.
+        // Since c <= 16, the following cast is fine.
+        last_block.get()[AES_CBC_BLOCK_SIZE_IN_BYTES] = (uint8_t)c;


Oh... Now I get it, are we using this extra byte at the end of this last-block array as a secret variable that stores the current length stored in the intermediate last-block scratch buffer? That state really needs to be tracked elsewhere, that's really unclear.

geedo0 · 2024-05-10T13:34:30Z

tst/com/amazon/corretto/crypto/provider/test/AesCbcIso10126Test.java

+    // Since the padding is random, the last block of the cipher texts are not necessarily the same.
+    for (int i = 0; i < accpCipherText.length - 16; i++) {
+      assertEquals(sunCipherText[i], accpCipherText[i]);
+    }


WShat Alex is saying is that since the last byte is the padding length, it should be the same when given the same plaintext.

tst/com/amazon/corretto/crypto/provider/test/AesCbcIso10126Test.java

+ Add more comments to the code. + Add more tests. + Update README.md and CHANGELOG.md

+ Sometimes MacOS JDK11 runs out of memory for CBC ByteBuffer tests

alexw91 · 2024-05-10T20:05:48Z

tst/com/amazon/corretto/crypto/provider/test/AesCbcTest.java

@@ -497,7 +485,7 @@ public void testMultiStepByteBuffer(
  private static Stream<Arguments> byteBufferTestParams() {
    final List<Arguments> result = new ArrayList<>();
    for (final int keySize : new int[] {128}) {
-      for (int i = 0; i != 1024; i++) {


Why was this reduced to 512?

In the Git commit I mentioned this. Some of the MacOS CI tasks sometimes run out of memory with this test.

Does this mean that this PR increases memory usage of AES-CBC? I'm wondering if this is a sign of a potential memory leak somewhere or not? I don't see any potential memory leaks myself, but I think it's worth it for everyone to take an extra 5-10 minutes to re-review with this in mind, and for us to be extra careful once this change is released and keep an eye on memory usage metrics.

This is from the CBC tests and it's not part of this change. The ISO10126 implementation doesn't allocate anything on the heap and it relies on CBC implementation. If there is any leaks, it must be coming from the CBC implementation. I can see one potential leak from in the CBC implementation in the following two places: InitUpdate, and InitUpdateFinal
new NativeEvpCipherCtx registers the allocated EVP_CIPHER_CTX with the Janitor. In case the registration fails, we could have a leak. However, this style of allocate-then-register has been the norm in other parts of our code: https://github.com/corretto/amazon-corretto-crypto-provider/blob/main/src/com/amazon/corretto/crypto/provider/AesGcmSpi.java#L1004

alexw91 · 2024-05-10T20:11:10Z

csrc/aes_cbc.cpp

+    // For ISO10126, we use CBC with no padding.
+    static int padding_to_be_used(int padding)
+    {
+        return is_iso10126_padding(padding) ? com_amazon_corretto_crypto_provider_AesCbcSpi_NO_PADDING : padding;
+    }


This function isn't used any longer, it can be deleted

Forgot that. I'll remove it in the next commit.

alexw91 · 2024-05-10T20:58:29Z

csrc/aes_cbc.cpp

+        if (!is_iso10126_padding || is_enc) {
+            return update(input, input_len, output, unprocessed_input);
+        }
+        // We are decrypting and padding is ISO10126. In this case, we must not decrypt the last block until do_final.


I don't have a concrete request, but I'll say that I can't keep 11 different length variables in my head simultaneously. Is there any way to simplify this? Or provide a clearer explanation of the relationship between all these variables up front? Maybe an Ascii art diagram? Or a diagram image attached as a PR comment? I find myself wishing for something similar to s2n's stuffer diagram in s2n's documentation.

input_len

last_block_len

all

rem

save_bytes

process_bytes

process_bytes_last_block

process_bytes_input

save_bytes_input

save_bytes_last_block

from_index

I'll look into this.

I simplified the logic and now the first five variables are used.

geedo0 · 2024-05-13T14:22:31Z

csrc/aes_cbc.cpp

-        unsigned int from_index = last_block_len - save_bytes_last_block;
-        for (unsigned int i = 0; i < save_bytes_last_block; i++) {
-            last_block.get()[i] = last_block.get()[from_index + i];
+        if (all <= AES_CBC_BLOCK_SIZE_IN_BYTES) {


This latest iteration is a lot easier to follow. I'm still not a fan of having to hold onto a full-block on each update. This is not a problem for the logic in any other block algorithm. This seems to be caused by not having the right flags/context in between invocations, but I understand that this is a pretty annoying refactor to address.

One thing you can do to simplify is to avoid special casing the all=16b case by just letting it flow through with a 0-byte update. That way we have one fewer edge case to track.

After an offline convo, Amir walked me through why we need to manage this last full block on the decryption path.

geedo0

Approved, but would really like to see us refactor how the scratch-buffer size is tracked. The way it's handled now is really unconventional and puts us at high-risk of odd bugs.

AES-CBC with ISO10126 Padding

d36ad55

+ AWS-LC does not support ISO10126. + The padding and unpadding are handled in ACCP. + The padding is explained here: https://www.w3.org/TR/xmlenc-core1/#sec-Padding

amirhosv marked this pull request as ready for review May 9, 2024 14:26

amirhosv requested a review from a team as a code owner May 9, 2024 14:26

Add more tests for multi-step operations and more comments.

9cdee59

geedo0 reviewed May 9, 2024

View reviewed changes

alexw91 reviewed May 9, 2024

View reviewed changes

WillChilds-Klein reviewed May 10, 2024

View reviewed changes

geedo0 reviewed May 10, 2024

View reviewed changes

amirhosv added 2 commits May 10, 2024 15:09

Addressing PR comments and update docs

c996c1b

+ Add more comments to the code. + Add more tests. + Update README.md and CHANGELOG.md

Use Enum for Paddings and reduce the number of tests for CBC ByteBuffer

d890290

+ Sometimes MacOS JDK11 runs out of memory for CBC ByteBuffer tests

alexw91 reviewed May 10, 2024

View reviewed changes

amirhosv added 2 commits May 10, 2024 21:43

Remove unused code and add a test for processing data one-byte-at-a-time

3e6e7a9

Simplify the logic of update when decrypting with ISO10126 padding

9b1935b

geedo0 reviewed May 13, 2024

View reviewed changes

geedo0 approved these changes May 13, 2024

View reviewed changes

alexw91 approved these changes May 13, 2024

View reviewed changes

amirhosv merged commit 09151a3 into corretto:main May 13, 2024
10 checks passed

amirhosv deleted the aes-cbc-iso10126 branch May 13, 2024 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AES-CBC with ISO10126 Padding #381

AES-CBC with ISO10126 Padding #381

amirhosv commented May 9, 2024

geedo0 May 9, 2024

amirhosv May 10, 2024

geedo0 May 10, 2024

amirhosv May 10, 2024

geedo0 May 10, 2024

amirhosv May 10, 2024

geedo0 commented May 9, 2024

alexw91 May 9, 2024

amirhosv May 10, 2024

alexw91 May 9, 2024

amirhosv May 10, 2024

geedo0 May 10, 2024

amirhosv May 10, 2024 •

edited

Loading

alexw91 May 9, 2024 •

edited

Loading

amirhosv May 10, 2024

WillChilds-Klein May 9, 2024

amirhosv May 10, 2024

geedo0 May 10, 2024

geedo0 May 10, 2024

alexw91 May 10, 2024

amirhosv May 10, 2024

alexw91 May 13, 2024

amirhosv May 13, 2024 •

edited

Loading

alexw91 May 10, 2024

amirhosv May 10, 2024

alexw91 May 10, 2024

amirhosv May 10, 2024

amirhosv May 13, 2024

geedo0 May 13, 2024

geedo0 May 13, 2024

geedo0 left a comment

AES-CBC with ISO10126 Padding #381

AES-CBC with ISO10126 Padding #381

Conversation

amirhosv commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geedo0 commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirhosv May 10, 2024 • edited Loading

Choose a reason for hiding this comment

alexw91 May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirhosv May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geedo0 left a comment

Choose a reason for hiding this comment

amirhosv May 10, 2024 •

edited

Loading

alexw91 May 9, 2024 •

edited

Loading

amirhosv May 13, 2024 •

edited

Loading