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

1 similar comment
@gasbytes

gasbytes commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

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