Skip to content

Conversation

@manastasova
Copy link
Contributor

@manastasova manastasova commented Nov 3, 2025

Add incremental length random input message tests for AES-XTS encryption/decryption

Issues:

N/A

Description of changes:

AWS-LC's current AES-XTS tests use fixed-size test vectors but do not systematically verify that encryption and decryption work correctly across different message lengths.

This PR adds a new test that generates random inputs of incremental lengths and verify that encrypt(decrypt(input)) == input for each length. This ensures that AES-XTS correctly handles messages of varying sizes, particularly:

  • Messages at and above the minimum length (16 bytes)
  • Messages that require ciphertext stealing (non-block-aligned sizes)
  • Messages of various sizes up to the practical maximum

The tests also add explicit length equality checks between plaintext and ciphertext to catch potential length-related bugs early.

Call-outs:

The test provides better confidence in correctness across different input sizes.

Testing:

./crypto/crypto_test --gtest_filter=XTSTest.EncryptDecryptRand.

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.

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


SCOPED_TRACE(plaintext.size());

int len;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'len' is not initialized [cppcoreguidelines-init-variables]

Suggested change
int len;
int len = 0;

SCOPED_TRACE(plaintext.size());

int len;
uint8_t *in_p, *out_p;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'in_p' is not initialized [cppcoreguidelines-init-variables]

Suggested change
uint8_t *in_p, *out_p;
uint8_t *in_p = nullptr, *out_p;

SCOPED_TRACE(plaintext.size());

int len;
uint8_t *in_p, *out_p;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'out_p' is not initialized [cppcoreguidelines-init-variables]

Suggested change
uint8_t *in_p, *out_p;
uint8_t *in_p, *out_p = nullptr;

@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2025

Codecov Report

❌ Patch coverage is 98.03922% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.44%. Comparing base (d826708) to head (5be4394).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crypto/fipsmodule/modes/xts_test.cc 98.03% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2795      +/-   ##
==========================================
+ Coverage   78.42%   78.44%   +0.02%     
==========================================
  Files         683      683              
  Lines      116283   116336      +53     
  Branches    16404    16410       +6     
==========================================
+ Hits        91194    91261      +67     
+ Misses      24213    24199      -14     
  Partials      876      876              

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

Copy link
Contributor

@nebeid nebeid left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you @manastasova.

iv.data()));
ASSERT_TRUE(
EVP_EncryptUpdate(ctx.get(), out_p, &len, in_p, plaintext.size()));
OPENSSL_memcpy(ciphertext.data(), out_p, plaintext.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line, and more so ciphertext, used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, not used. Thanks, Nevine. Updated.

#endif
}

TEST(XTSTest, EncryptDecyptRand) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TEST(XTSTest, EncryptDecyptRand) {
TEST(XTSTest, EncryptDecryptRand) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@manastasova manastasova changed the title Add Encrypt and Decrypt test on random input messages of incremental … AES-XTS Enc Dec test on rand incremental length inputs Nov 7, 2025
@manastasova manastasova marked this pull request as ready for review November 7, 2025 22:10
@manastasova manastasova requested a review from a team as a code owner November 7, 2025 22:10
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


// Test encryption.

OPENSSL_memcpy(in_p, plaintext.data(), plaintext.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: Use of memory after it is freed [clang-analyzer-cplusplus.NewDelete]

        OPENSSL_memcpy(in_p, plaintext.data(), plaintext.size());
        ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this warning appears. in_p is used as the input message for the encryption function later on.

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.

4 participants