-
Notifications
You must be signed in to change notification settings - Fork 152
Implementation of XAES-256-GCM with EVP_AEAD #2809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## xaes-256-gcm #2809 +/- ##
================================================
+ Coverage 78.27% 78.31% +0.03%
================================================
Files 683 683
Lines 117356 117484 +128
Branches 16482 16488 +6
================================================
+ Hits 91865 92002 +137
+ Misses 24609 24601 -8
+ Partials 882 881 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
crypto/fipsmodule/cipher/e_aes.c
Outdated
| // ------------------------------------------------------------------------------ | ||
| // ---------------- EVP_AEAD XAES-256-GCM Without Key Commitment ---------------- | ||
| // ------------------------------------------------------------------------------ | ||
| /* Since ctx->state capacity is 568 bytes, it does not have enough space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's ctx->state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I write in design doc: "state is a void pointer in the EVP_AEAD_CTX object pointing to an opaque memory that can be used to store implementation-specific data".
See this:
https://github.com/ttungle96/aws-lc/blob/xaes-256-gcm/include/openssl/aead.h#L223
https://github.com/ttungle96/aws-lc/blob/xaes-256-gcm/include/openssl/aead.h#L213
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but other people reading the code might not have access to your design doc :)
anyway, why don't you increase the size of ctx->state instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I'm not sure whether it is allowed to do so. Increasing the size of ctx->state will simplify the code. Should I follow that direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, @nebeid what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I just increased the size of ctx->state and cleaned allocate/free memory code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better not to have a pointer, thanks, dkostic. On the other hand, do we need to have the gcm struct at all? What's different in using it compared to #2652 where it was created on the stack in the seal/open calls because nothing in it needs to be persisted between calls.
We're not considering yet reusing the gcm context, are you preparing for that usecase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed struct aead_aes_gcm_ctx gcm_ctx from AEAD_XAES_256_GCM_CTX.
crypto/fipsmodule/cipher/e_aes.c
Outdated
| AEAD_XAES_256_GCM_CTX *xaes_ctx = (AEAD_XAES_256_GCM_CTX*)&ctx->state; | ||
|
|
||
| // Allocate memory for xaes_ctx->gcm_ctx | ||
| xaes_ctx->gcm_ctx = OPENSSL_malloc(sizeof(struct aead_aes_gcm_ctx)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should check that OPENSSL_malloc succeeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add checking. The issue is it's hard to create a test case that could make it failed to increase test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's ok, you can leave that particular code path untested.
| AES_encrypt(M1, derived_key, &xaes_ctx->xaes_key); | ||
| AES_encrypt(M2, derived_key + AES_BLOCK_SIZE, &xaes_ctx->xaes_key); | ||
| AES_encrypt(M, derived_key, xaes_key); | ||
| M[1] ^= 0x03; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be good to clarify the outcome of this XOR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added a comment!
crypto/fipsmodule/cipher/e_aes.c
Outdated
| // ------------------------------------------------------------------------------ | ||
| // ---------------- EVP_AEAD XAES-256-GCM Without Key Commitment ---------------- | ||
| // ------------------------------------------------------------------------------ | ||
| /* Since ctx->state capacity is 568 bytes, it does not have enough space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better not to have a pointer, thanks, dkostic. On the other hand, do we need to have the gcm struct at all? What's different in using it compared to #2652 where it was created on the stack in the seal/open calls because nothing in it needs to be persisted between calls.
We're not considering yet reusing the gcm context, are you preparing for that usecase?
tool/speed.cc
Outdated
| #if AWSLC_API_VERSION > 16 | ||
| !SpeedKEM(selected) || | ||
| #endif | ||
| #if AWSLC_API_VERSION > 31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XAES APIs are introduced in
Line 117 in 368d499
| #define AWSLC_API_VERSION 35 |
so maybe we need to introduce a new
#if AWSLC_API_VERSION > 34 for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I updated it.
crypto/cipher_extra/aead_test.cc
Outdated
| // Test encryption and decryption with a plaintext | ||
| const uint8_t *plaintext = (const uint8_t *)"Hello, XAES-256-GCM!"; | ||
| std::vector<uint8_t> ciphertext_and_tag; | ||
| DecodeHex(&ciphertext_and_tag, "01e5f78bc99de880bd2eeff2870d361f0eab5b2fc55268f34b14045878fe3668db980319"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this being used in the test? I don't see it in the arguments.
Where are the results from? The EVP implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. They're redundant. Since for shorter nonce, we current only test to ensure encryption/decryption. I'll update. Thank you!
sgmenda-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some non-blocking remarks
crypto/fipsmodule/cipher/e_aes.c
Outdated
| // Reference for nonce size < 24 bytes: | ||
| // https://eprint.iacr.org/2025/758.pdf#page=24 | ||
| /* When nonce size b < 24 bytes, it uses bytes [b-12:b] | ||
| * of input nonce as iv for the underlying AES encryption. | ||
| * nonce_len is b in the referece, where 20 <= b <= 24 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is repeated twice, here an in aead_xaes_256_gcm_open_gather. it would be clearer to make a helper function / macro that takes (nonce, nonce_len) and returns nonce + nonce_len - AES_GCM_NONCE_LENGTH and put the comment there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! I'll change.
include/openssl/aead.h
Outdated
| // EVP_aead_xaes_256_gcm is AES-256 in Galois Counter Mode with CMAC-based KDF | ||
| OPENSSL_EXPORT const EVP_AEAD *EVP_aead_xaes_256_gcm(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a TLS-specific AEAD, so disambiguate. Also link to spec.
| // EVP_aead_xaes_256_gcm is AES-256 in Galois Counter Mode with CMAC-based KDF | |
| OPENSSL_EXPORT const EVP_AEAD *EVP_aead_xaes_256_gcm(void); | |
| // Other AEAD algorithms. | |
| // EVP_aead_xaes_256_gcm is AES-256 in Galois Counter Mode with CMAC-based KDF, as specified in https://c2sp.org/XAES-256-GCM | |
| OPENSSL_EXPORT const EVP_AEAD *EVP_aead_xaes_256_gcm(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move this next to EVP_aead_aes_256_gcm().
tool/speed.cc
Outdated
| #endif | ||
| #if AWSLC_API_VERSION > 31 | ||
| !SpeedDigestSign(selected) || | ||
| !SpeedDigestSign(selected) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| !SpeedDigestSign(selected) || | |
| !SpeedDigestSign(selected) || |
70cd651
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
Issues:
Description of Changes
Implementation for API EVP_AEAD of XAES-256-GCM is appended to e_aes.c, and the tests are appended to aead_test.cc.
Testing
We use the test vectors provided here:
https://github.com/C2SP/C2SP/blob/main/XAES-256-GCM.md
https://github.com/C2SP/C2SP/blob/main/XAES-256-GCM/openssl/openssl.c
https://github.com/C2SP/C2SP/blob/main/XAES-256-GCM/go/XAES-256-GCM_test.go
./crypto_test --gtest_filter='All/PerAEADTest.*'./crypto_test --gtest_filter='CipherTest.*'Modifications compared with #2652
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.