Fix AES-CTS one-block, split-init IV, and TLS record HMAC handling - #476
Fix AES-CTS one-block, split-init IV, and TLS record HMAC handling#476gasbytes wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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->ivintowc_AesSetKeyso 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.
d5479bc to
79c2e3f
Compare
79c2e3f to
ddbf473
Compare
aidangarske
left a comment
There was a problem hiding this comment.
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
ddbf473 to
dfb7a22
Compare
aidangarske
left a comment
There was a problem hiding this comment.
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
dfb7a22 to
6fef0e0
Compare
- Use the cached IV for split-init AES-CTS in both init orders - consume tls_data_size and equalize hmac block count in final
6fef0e0 to
e62e724
Compare
|
Jenkins retest this please |
Added associated regression test for each change (test_aes128_cts_one_block, test_aes128_cts_split_init and test_hmac_tls_data_size).