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

Open

gasbytes wants to merge 1 commit into

wolfSSL:master from

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:master from
gasbytes:aes-related-fix

Conversation

@gasbytes

@gasbytes gasbytes commented Aug 26, 2026 •

edited

Loading

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 label

Aug 26, 2026

Copilot started reviewing on behalf of gasbytes

August 26, 2026 12:55

View session

Copilot AI reviewed

Aug 26, 2026

View reviewed changes

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.

src/wp_aes_stream.c

test/test_cipher.c Outdated

@gasbytes

gasbytes force-pushed the aes-related-fix branch from d5479bc to 79c2e3f Compare

August 26, 2026 13:33

@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 force-pushed the aes-related-fix branch from 79c2e3f to ddbf473 Compare

August 28, 2026 11:13

@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 requested changes

Aug 28, 2026

View reviewed changes

@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 blocks — src/wp_hmac.c:313-321
[High] Preserve TLS state when duplicating HMAC contexts — src/wp_hmac.c:55-58
[Medium] Exercise the TLS block-count behavior in the regression test — test/test_hmac.c:814-863
[Medium] Cover the new CFB no-IV reinitialization branch — src/wp_aes_stream.c:314-320

Review generated by Skoll

src/wp_hmac.c Outdated

src/wp_hmac.c

test/test_hmac.c Outdated

src/wp_aes_stream.c

@gasbytes

gasbytes force-pushed the aes-related-fix branch from ddbf473 to dfb7a22 Compare

August 31, 2026 15:39

@gasbytes

gasbytes requested a review from aidangarske

August 31, 2026 15:39

aidangarske

aidangarske requested changes

Aug 31, 2026

View reviewed changes

@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 path — src/wp_hmac.c:308-310
[Low] SHA-384 dummy-block test runs when SHA-384 is disabled — test/test_hmac.c:897

Review generated by Skoll

src/wp_hmac.c Outdated

test/test_hmac.c

@gasbytes

gasbytes force-pushed the aes-related-fix branch from dfb7a22 to 6fef0e0 Compare

August 31, 2026 16:44

@gasbytes


 - Handle exactly one block in AES-CTS as plain CBC, matching OpenSSL

e62e724

- Use the cached IV for split-init AES-CTS in both init orders
- consume tls_data_size and equalize hmac block count in final

@gasbytes

gasbytes force-pushed the aes-related-fix branch from 6fef0e0 to e62e724 Compare

August 31, 2026 17:47

@gasbytes

gasbytes requested a review from aidangarske

August 31, 2026 18:40

@aidangarske

aidangarske commented Aug 31, 2026

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

padelsbach

padelsbach requested changes

Sep 11, 2026

View reviewed changes

src/wp_hmac.c

XMEMSET(dummy, 0, sizeof(dummy));

for (i = 0; ok && (i < macCtx->tlsDummyBlocks); i++) {

rc = wc_HmacUpdate(&macCtx->hmac, dummy, (word32)blockSz);

if (rc != 0) {

@padelsbach padelsbach Sep 11, 2026

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

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

This does a hmac operation after wc_HmacFinal which resets the object. We might get bogus or non-deterministic results. Can you check?

src/wp_hmac.c

WOLFPROV_ENTER(WP_LOG_COMP_MAC, "wp_hmac_update");

if (macCtx->tlsDataSize > 0) {

@padelsbach padelsbach Sep 11, 2026

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Does this work on chunk'ed data, or combined header+data? Seems like the block count is reestablished on each call

src/wp_hmac.c

static int wp_hmac_blocks(word32 len, int blockBits, word32 blockMask,

word32 padSz)

{

return (int)(len >> blockBits) +

@padelsbach padelsbach Sep 11, 2026

Copy link

Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Does the shift arithmetic work for HMAC-SHA3?

Labels

ci:all

4 participants

@gasbytes @aidangarske @padelsbach

Uh oh!

Conversation

@gasbytes gasbytes commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@aidangarske aidangarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@aidangarske aidangarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Uh oh!

Uh oh!

aidangarske commented Aug 31, 2026

Uh oh!

gasbytes commented Sep 1, 2026

Uh oh!

@padelsbach padelsbach Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

@padelsbach padelsbach Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

@padelsbach padelsbach Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@gasbytes gasbytes commented Aug 26, 2026 •

edited

Loading