AES-CBC #380

amirhosv · 2024-04-30T16:14:52Z

Description of changes:

Adding support for AES-CBC with no padding and with PKCS7 padding.

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

geedo0 · 2024-05-01T15:53:47Z

I did a first pass, but still need to go through how the buffering works and how we handle the padding/non-padding cases.

I normally don't like to police commit messages and PR descriptions, but this PR could use some more context and explanation around what's going on. This helps with the review process and also helps future developers figure out what happened in the code. This is especially important for large diffs like this. Things that would be helpful to explain:

Everything that's going on with the buffers.
Why PKCS5 aliases PKCS7.
Why we're adding this algorithm or a reference to a ticket.
What testing you're doing and why.
The refactoring you did around AES GCM and this new CBC class.

The README.md file needs to be updated to include the new algorithm.

Curious on your thoughts why we pulled in the NIST test vectors for this algorithm. ACCP doesn't consistently pull-in NIST vectors and tests for all algorithms. This PR accounts for half the rsp.gz files that we've imported. It's unclear to me what decision criteria is used for when we do vector testing in ACCP in addition to AWS-LC's extensive vector testing.

.github/workflows/macos_ci_jobs.yml

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

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

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

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

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

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

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

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

geedo0 · 2024-05-02T14:21:08Z

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

+@ExtendWith(TestResultLogger.class)
+@Execution(ExecutionMode.CONCURRENT)
+@ResourceLock(value = TestUtil.RESOURCE_GLOBAL, mode = ResourceAccessMode.READ)
+public class AesCbcNistTest {


I'm unsure about whether or not we should be including NIST test vectors in ACCP. This is not something we do for all algorithms in ACCP. More testing is always great, but I think it's a bit redundant with AWS-LC's testing. Our focus should be more on tests that surface issues around interfacing with the native layer and less so around exhaustive validation of the cryptographic implementation.

For other algorithms, I noticed that we have known answer tests (KATs), so I looked for AES/CBC KATs and the NIST website showed up. I'm just trying to treat AES/CBC the same way that other algorithms have been treated.

I understand that we have them for other algorithms, but not for //all// algorithms. I'm trying to understand what our decision criteria is for including or not including KATs. It seems pretty random. Why are we exhaustively testing every single KAT style/variant for CBC but not for GCM? It just seems off to me for us to be KAT testing AES-CBC much more thoroughly than literally every other algorithm in ACCP.

I guess we never formally specified when to add KATs. My rule of thumb is to search online for KATs and if found, then add.

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

* 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.

amirhosv · 2024-05-03T13:44:46Z

I did a first pass, but still need to go through how the buffering works and how we handle the padding/non-padding cases.

I normally don't like to police commit messages and PR descriptions, but this PR could use some more context and explanation around what's going on. This helps with the review process and also helps future developers figure out what happened in the code. This is especially important for large diffs like this. Things that would be helpful to explain:

Everything that's going on with the buffers.

Why PKCS5 aliases PKCS7.

Why we're adding this algorithm or a reference to a ticket.

What testing you're doing and why.

The refactoring you did around AES GCM and this new CBC class.

The README.md file needs to be updated to include the new algorithm.

Curious on your thoughts why we pulled in the NIST test vectors for this algorithm. ACCP doesn't consistently pull-in NIST vectors and tests for all algorithms. This PR accounts for half the rsp.gz files that we've imported. It's unclear to me what decision criteria is used for when we do vector testing in ACCP in addition to AWS-LC's extensive vector testing.

I think I've answered all your comments.

geedo0 · 2024-05-06T17:22:15Z

I think I've answered all your comments.

Yes, I see it all in the code. To clarify though, I was really hoping to see some of these design decisions documented in the PR itself. That helps me a lot when trying to reason out why certain code looks the way it does.

WillChilds-Klein

still reviewing, about 1/2 way through.

csrc/aes_cbc.cpp

WillChilds-Klein · 2024-05-06T19:37:35Z

csrc/buffer.h

@@ -603,5 +603,30 @@ class JByteArrayCritical {
    jbyteArray jarray_;
 };



suggest some documentation here. what is this class? how does it differ from other buffer types?

sorry, i'd meant doc comments for the new ShimBuffer, but it's not super important.

No worries. I added comments to ShimByteBuffer as well.

WillChilds-Klein · 2024-05-06T19:37:41Z

csrc/buffer.cpp

+SimpleBuffer::SimpleBuffer(int size)
+    : buffer_(nullptr)
+{
+    buffer_ = (uint8_t*)malloc(size);


are we sure we want to use libc malloc here? why not use the same allocator as in other primitives?

I think we only have a SecureBuffer that allocates on stack. The scratch buffer size used when output clobbers input depends on the input/output buffers at runtime, so don't know the size before hand.
I don't think there is any issue in using malloc/free. The only concern could be that we do not zero memory after deallocation. The buffer is used to hold cipherText/plainText. In that sense it's as secure as how Java treats buffers by the garbage collector.
I'm open to suggestion to see if zeroing memory is needed before freeing up.

Two things:

The code is updated so that before freeing the memory it is zeroed.

I see that we have SecureAlloc that could potentially be used. The main issue with this class is that it throws std::bad_alloc. In our JNI code, we only catch java_ex, which means any other type of exception could potentially result in undefined behavior in JNI calls.

Replacing SecureAlloc with OPENSSL_cleanse makes sense. I wonder if we should extend that idea a little further, using OPENSSL_malloc and OPENSSL_free in SimpleBuffer. This would provide two benefits:

Automatically call OPENSSL_cleanse on free

Automatically use the same underlying allocator as AWS-LC in case they ever decide to switch to e.g. jemalloc or something. due to portability concerns, IMO this is unlikely.

not a blocker at all, just perhaps something to consider.

That's a great suggestion. I changed it so that it will use OPENSS_{malloc,free}.

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

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

WillChilds-Klein

LGTM, just two small open comment threads to consider here or in future PRs.

amirhosv force-pushed the aes-cbc branch 5 times, most recently from 3b64d33 to 755c978 Compare April 30, 2024 19:17

amirhosv marked this pull request as ready for review April 30, 2024 20:50

amirhosv requested a review from a team as a code owner April 30, 2024 20:50

AES-CBC

60cf9f7

amirhosv force-pushed the aes-cbc branch from 755c978 to 60cf9f7 Compare May 1, 2024 14:49

geedo0 reviewed May 1, 2024

View reviewed changes

geedo0 reviewed May 2, 2024

View reviewed changes

alexw91 reviewed May 2, 2024

View reviewed changes

tst/com/amazon/corretto/crypto/provider/test/AesCbcTest.java Outdated Show resolved Hide resolved

Addressing first round of comments:

056ea78

* 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.

Tests for in-place operations

eef8b64

geedo0 previously approved these changes May 6, 2024

View reviewed changes

WillChilds-Klein reviewed May 6, 2024

View reviewed changes

WillChilds-Klein reviewed May 7, 2024

View reviewed changes

Address PR comments

3bff6ab

amirhosv dismissed geedo0’s stale review via 3bff6ab May 7, 2024 16:33

Zero memory for SimpleBuffer using OPENSSL_cleanse

b3c3a7e

WillChilds-Klein previously approved these changes May 7, 2024

View reviewed changes

Use OPENSSL_malloc and free for SimpleBuffer

d855a69

amirhosv dismissed WillChilds-Klein’s stale review via d855a69 May 8, 2024 13:57

amirhosv requested review from geedo0 and WillChilds-Klein May 8, 2024 16:43

WillChilds-Klein approved these changes May 8, 2024

View reviewed changes

geedo0 approved these changes May 8, 2024

View reviewed changes

amirhosv merged commit ee2fa55 into corretto:main May 8, 2024
10 checks passed

amirhosv deleted the aes-cbc branch May 8, 2024 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AES-CBC #380

AES-CBC #380

amirhosv commented Apr 30, 2024

geedo0 commented May 1, 2024

geedo0 May 2, 2024

amirhosv May 3, 2024

geedo0 May 6, 2024

amirhosv May 7, 2024

amirhosv commented May 3, 2024

geedo0 commented May 6, 2024

WillChilds-Klein left a comment

WillChilds-Klein May 6, 2024

amirhosv May 7, 2024

WillChilds-Klein May 7, 2024

amirhosv May 8, 2024

WillChilds-Klein May 6, 2024

amirhosv May 7, 2024

amirhosv May 7, 2024

WillChilds-Klein May 7, 2024

amirhosv May 8, 2024 •

edited

Loading

WillChilds-Klein left a comment

		@@ -603,5 +603,30 @@ class JByteArrayCritical {
		jbyteArray jarray_;
		};

AES-CBC #380

AES-CBC #380

Conversation

amirhosv commented Apr 30, 2024

geedo0 commented May 1, 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

amirhosv commented May 3, 2024

geedo0 commented May 6, 2024

WillChilds-Klein left a 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

Choose a reason for hiding this comment

amirhosv May 8, 2024 • edited Loading

Choose a reason for hiding this comment

WillChilds-Klein left a comment

Choose a reason for hiding this comment

amirhosv May 8, 2024 •

edited

Loading