-
Notifications
You must be signed in to change notification settings - Fork 155
Add EVP_CIPHER API for XAES-256-GCM with Key Commitment #2826
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
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
| #define NUM_NID 1000 | ||
| #define NUM_NID 1001 | ||
|
|
||
| static const uint8_t kObjectData[] = { |
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.
warning: unknown type name 'uint8_t' [clang-diagnostic-error]
static const uint8_t kObjectData[] = {
^| 0x32, | ||
| }; | ||
|
|
||
| static const ASN1_OBJECT kObjects[NUM_NID] = { |
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.
warning: unknown type name 'ASN1_OBJECT' [clang-diagnostic-error]
static const ASN1_OBJECT kObjects[NUM_NID] = {
^
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## xaes-256-gcm #2826 +/- ##
================================================
+ Coverage 78.30% 78.32% +0.01%
================================================
Files 683 683
Lines 117484 117572 +88
Branches 16489 16499 +10
================================================
+ Hits 92000 92086 +86
- Misses 24602 24604 +2
Partials 882 882 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
043f78e to
a7ccfc5
Compare
a594eb1 to
01a9681
Compare
nebeid
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.
I will review cipher_test.cc
| #if defined(OPENSSL_32_BIT) | ||
| assert((uintptr_t)ptr % 4 == 0); | ||
| ptr += (uintptr_t)ptr & 4; | ||
| #endif | ||
| assert((uintptr_t)ptr % 8 == 0); | ||
| ptr += (uintptr_t)ptr & 8; |
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 doubt we need these alignment requirements in addition to those of the gcm ctx. Can we try removing them here and on line 1858? Otherwise, do you know what would drive our need 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.
It will lead to many CI tests failed. I tried that. You can see the failed tests below. However, when I created and run with a docker, all tests still passed.
|
|
||
| XAES_256_GCM_KC_CTX *xaes_ctx = xaes_256_gcm_kc_from_cipher_ctx(ctx); | ||
|
|
||
| switch(type) { |
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 it's more prudent to check that ptr is not NULL.
I see that there are no checks in aes_gcm_ctrl, but it's good to verify here and we should probably change there as well wherever it's used. It's sometimes intentionally NULL as in the INIT case in
aws-lc/crypto/fipsmodule/cipher/cipher.c
Line 185 in 88aec89
| if (!EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_INIT, 0, NULL)) { |
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. I'll add checking ptr for two new added ctrl commands.
| AAD = 633273702e6f72672f584145532d3235362d47434d | ||
| Tag = 7fd083bf3fdb41abd740a21f71eb769d | ||
|
|
||
| # Note: KC are our own test values |
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 it possible to use the same set of tests as the ones without KC and the test harness just ignores it when it's plain XAES?
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.
Does that mean I should remove all changes that were made to cipher_test.cc to support key commitment verification?
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.
No, sorry, what I meant is if you were to unify the 2 sets of test vectors in this file (i.e. delete the ones above of XAES-256-GCM because they're superseded by the KC one), would they then work with both XAES and XAES-KC? It seems to me that XAES may not be tested in CipherFileTest because it needs its own "Cipher" string. So you can ignore the suggestion.
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. It needs to have "Cipher = XAES-256-GCM-KC" to specify the cipher.
| case NID_xaes_256_gcm: | ||
| assert(ctx->cipher->ctx_size == sizeof(XAES_256_GCM_CTX)); | ||
| ptr = ctx->cipher_data; | ||
| #if defined(OPENSSL_32_BIT) |
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.
In my previous comment, I meant keeping this alignment in aes_gcm_from_cipher_ctx and try and remove them from the ones below, but maybe it doesn't make sense because it's the same pointer to ctx->cipher_data and it would be different if you're not doing the same operations to get it.
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. Since aes_gcm_from_cipher_ctx() is called in other places like aes_gcm_cipher(), aes_gcm_ctrl(), and aes_gcm_init_key(), it needs to be consistent everywhere.
include/openssl/cipher.h
Outdated
| // We're introducing a new error code for invalid key commitment | ||
| // We're introducing a new error code for invalid key commitment and invalid buffer | ||
| #define CIPHER_R_KEY_COMMITMENT_INVALID 145 | ||
| #define CIPHER_R_BUFFER_INVALID 146 |
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.
You can use
Line 1447 in 88aec89
| #define GUARD_PTR(ptr) __AWS_LC_ENSURE((ptr) != NULL, OPENSSL_PUT_ERROR(CRYPTO, ERR_R_PASSED_NULL_PARAMETER); \ |
and no need to introduce a new error.
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 replaced that directive in my update and deleted CIPHER_R_BUFFER_INVALID.
| AAD = 633273702e6f72672f584145532d3235362d47434d | ||
| Tag = 7fd083bf3fdb41abd740a21f71eb769d | ||
|
|
||
| # Note: KC are our own test values |
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.
No, sorry, what I meant is if you were to unify the 2 sets of test vectors in this file (i.e. delete the ones above of XAES-256-GCM because they're superseded by the KC one), would they then work with both XAES and XAES-KC? It seems to me that XAES may not be tested in CipherFileTest because it needs its own "Cipher" string. So you can ignore the suggestion.
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.
Can the negative tests that were added for XAES (e.g. shorter nonces) be modified to also test XAES-KC?
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 made changes in cipher_test.cc to test negative cases with invalid key/nonce, and shorter nonce for both evp-xaes-256-gcm and evp-xaes-256-gcm-kc.
Description of Changes
kcat the end with default value to support testing cipher with key commitment without disrupting legacy code.Testing
Reuse the test vectors in #2750 with our own added key commitment values.
Evaluation
Performance evaluation with speedtool on c6i.2xlarge (8 vCPUs and 16 GB RAM):

./crypto_test --gtest_filter='CipherTest.*'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.