Skip to content

Fix AES-CTS one-block, split-init IV, and TLS record HMAC handling - #476

Open
gasbytes wants to merge 1 commit into
wolfSSL:masterfrom
gasbytes:aes-related-fix
Open

Fix AES-CTS one-block, split-init IV, and TLS record HMAC handling#476
gasbytes wants to merge 1 commit into
wolfSSL:masterfrom
gasbytes:aes-related-fix

Conversation

@gasbytes

@gasbytes gasbytes commented Aug 26, 2026

Copy link
Copy Markdown
Contributor
  • In wp_aes_cts_encrypt/wp_aes_cts_decrypt handle an input of exactly one block as plain CBC;
  • In wp_aes_stream_init pass ctx->iv to wc_AesSetKey, which is the cached iv from the wolfprovider context;
  • In wp_aes_stream_init restore oiv into iv when re-initializing without an iv, matching wp_aes_block_init;
  • In wp_hmac_set_ctx_params consume OSSL_MAC_PARAM_TLS_DATA_SIZE and advertise it in wp_hmac_settable_ctx_params;
  • In wp_hmac_final hash dummy blocks so the number of blocks hashed depends only on the padded record length;

Added associated regression test for each change (test_aes128_cts_one_block, test_aes128_cts_split_init and test_hmac_tls_data_size).

@gasbytes gasbytes self-assigned this Aug 26, 2026
Copilot AI lite review requested due to automatic review settings August 26, 2026 12:54
@gasbytes gasbytes added the ci:all PR OSP toggle: run all label Aug 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes two AES-CTS correctness issues in the wolfProvider AES stream implementation: (1) handling CTS for exactly one block, and (2) ensuring split-init sequences correctly preserve/use the IV when the key is set in a separate init call. It also adds targeted regression tests to prevent both issues from recurring.

Changes:

  • Treat AES-CTS input of exactly one block as plain CBC (encrypt/decrypt) to match OpenSSL behavior and avoid out-of-bounds behavior.
  • Update AES stream initialization to pass the cached ctx->iv into wc_AesSetKey so split init sequences keep the correct IV.
  • Add regression tests for one-block CTS and split-init IV handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/wp_aes_stream.c Adjusts key setup IV handling and adds special-case CTS logic for one-block inputs.
test/test_cipher.c Adds regression tests covering one-block CTS behavior and split-init IV behavior.
test/unit.c Registers the new unit tests in the test case table.
test/unit.h Declares prototypes for the new unit tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/wp_aes_stream.c
Comment thread test/test_cipher.c Outdated
Comment thread test/test_cipher.c Outdated
@gasbytes
gasbytes marked this pull request as ready for review August 26, 2026 13:37
@gasbytes
gasbytes requested a review from aidangarske August 26, 2026 13:52
@gasbytes gasbytes assigned aidangarske and unassigned gasbytes Aug 26, 2026
@gasbytes gasbytes changed the title Fix AES-CTS one-block and split-init IV handling Fix AES-CTS one-block, split-init IV, and TLS record HMAC handling Aug 28, 2026

@aidangarske aidangarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Skoll Code Review

Scan type: review

Overall recommendation: REQUEST_CHANGES
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

  • [High] Use the length-field width when counting final hash blockssrc/wp_hmac.c:313-321
  • [High] Preserve TLS state when duplicating HMAC contextssrc/wp_hmac.c:55-58
  • [Medium] Exercise the TLS block-count behavior in the regression testtest/test_hmac.c:814-863
  • [Medium] Cover the new CFB no-IV reinitialization branchsrc/wp_aes_stream.c:314-320

Review generated by Skoll

Comment thread src/wp_hmac.c Outdated
Comment thread src/wp_hmac.c
Comment thread test/test_hmac.c Outdated
Comment thread src/wp_aes_stream.c

@aidangarske aidangarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Skoll Code Review

Scan type: review-security

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)

Posted findings

  • [High] Invalid TLS padding takes a distinguishable HMAC timing pathsrc/wp_hmac.c:308-310
  • [Low] SHA-384 dummy-block test runs when SHA-384 is disabledtest/test_hmac.c:897

Review generated by Skoll

Comment thread src/wp_hmac.c Outdated
Comment thread test/test_hmac.c
- Use the cached IV for split-init AES-CTS in both init orders
- consume tls_data_size and equalize hmac block count in final
@aidangarske

Copy link
Copy Markdown
Member

Jenkins retest this please

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

Labels

ci:all PR OSP toggle: run all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants