Skip to content

Conversation

ttungle96
Copy link

@ttungle96 ttungle96 commented Oct 14, 2025

Issues:

  1. Add XAES-256-GCM, which is extended AES-256-GCM with a derived key mode proposed by Filippo Valsorda in 2023, followed by a specification released in 2024.
  2. This implementation supports EVP_CIPHER API, allows configuration to use either FIPS-compliant CMAC available in AWS-LC, or an optimized CMAC dedicated to the specific use case of XAES-256-GCM from XAES-256-GCM #2652.
  3. Support varying nonce sizes: 20 ≤ b ≤ 24 based on the extension: https://eprint.iacr.org/2025/758.pdf#page=24

Description of Changes

Implementation for API EVP_CIPHER of XAES-256-GCM is appended to e_aes.c, and the tests are appended to cipher_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

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.

@ttungle96 ttungle96 requested a review from a team as a code owner October 14, 2025 23:44
Copy link
Contributor

@github-actions github-actions bot left a 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 999
#define NUM_NID 1000

static const uint8_t kObjectData[] = {
Copy link
Contributor

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[] = {
             ^

0x31,
};

static const ASN1_OBJECT kObjects[NUM_NID] = {
Copy link
Contributor

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-commenter
Copy link

codecov-commenter commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 98.64865% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.94%. Comparing base (0fed4db) to head (1d4b4da).

Files with missing lines Patch % Lines
crypto/test/test_util.cc 71.42% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           xaes-256-gcm    #2750    +/-   ##
==============================================
  Coverage         78.94%   78.94%            
==============================================
  Files               677      677            
  Lines            115525   115674   +149     
  Branches          16249    16257     +8     
==============================================
+ Hits              91198    91317   +119     
- Misses            23530    23556    +26     
- Partials            797      801     +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

CHECK_ERROR(EVP_DecryptFinal(ctx.get(), out_vec.data(), &out_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
}

TEST(CipherTest, XAES_256_GCM_EVP_CIPHER) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I can follow all the tests this function is performing. Maybe you could break it into smaller test functions?

CHECK_ERROR(EVP_DecryptUpdate(ctx.get(), out_vec.data(), &out_len, in_vec.data(), in_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
CHECK_ERROR(EVP_DecryptFinal(ctx.get(), out_vec.data(), &out_len), ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use CipherFileTest for test vectors?

ASSERT_TRUE(ctx);
ASSERT_TRUE(EVP_CipherInit_ex(ctx.get(), EVP_xaes_256_gcm(), NULL, NULL, NULL, 1));

// Invalid nonce size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please write down the valid range for nonce size.

const uint8_t *plaintext = (const uint8_t *)"Hello, XAES-256-GCM!";
size_t plaintext_len = strlen((const char *)plaintext);

ciphertext.resize(20);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this?

TEST(CipherTest, XAES_256_GCM_EVP_CIPHER) {
{
std::vector<uint8_t> ciphertext, tag, decrypted;
const uint8_t key[32] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not std::vector<uint8_t>?

struct xaes_256_gcm_ctx *xaes_ctx =
(struct xaes_256_gcm_ctx *)((uint8_t*)ctx->cipher_data + XAES_256_GCM_CTX_OFFSET);

uint8_t derived_key[(256 >> 3)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic numbers?

return 1;
}

static const uint8_t kZeroIn[AES_BLOCK_SIZE] = {0};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can move inside the function it's using it.


static int xaes_256_gcm_ctx_init(struct xaes_256_gcm_ctx *xaes_ctx,
const uint8_t *key, const unsigned key_len) {
if ((key_len << 3) != 256) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic numbers?

ptr += (uintptr_t)ptr & 8;

OPENSSL_memcpy(ptr, gctx, sizeof(EVP_AES_GCM_CTX));
OPENSSL_free(temp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?


static int xaes_256_gcm_init(EVP_CIPHER_CTX *ctx, const uint8_t *key,
const uint8_t *iv, int enc) {
if(key != NULL && !xaes_256_gcm_init_common(ctx, key,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bug -- if key is NULL than you'll fall through and call xaes_256_gcm_set_gcm_key, but I don't think you want to do that, do you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants