Repository navigation
wp_aes_block: strip the explicit IV only for TLS 1.1+ and DTLS - #500
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation consistently applies the version-specific record layout and includes coverage for both affected paths.
Review effort: Balanced
Findings: None
What changed in this PR
Corrects AES-CBC record post-processing across TLS/DTLS versions.
Changes:
- Preserves TLS 1.0 payloads by not stripping an explicit IV.
- Handles explicit IVs for TLS 1.1+ and DTLS.
- Adds TLS 1.0 and DTLS 1.2 interoperability tests.
| File | Description |
|---|---|
src/wp_aes_block.c |
Applies version-specific explicit-IV handling. |
test/test_tls_cbc.c |
Adds version-aware record tests. |
test/unit.c |
Registers the new tests. |
test/unit.h |
Declares the new tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #500
Scan targets checked: wolfprovider-src, wolfprovider-bugs
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: test/unit.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
yosuke-wolfssl
assigned
wolfSSL-Bot and
yosuke-wolfssl and unassigned
yosuke-wolfssl and
wolfSSL-Bot
- wp_aes_block_tls_dec_record() takes the explicit IV length from tls_version: AES_BLOCK_SIZE for TLS 1.1, TLS 1.2 and DTLS, none for TLS 1.0. Any other version fails. - The minimum record length check and both the ETM and MtE paths use that IV length in place of AES_BLOCK_SIZE. - test_tls_cbc.c helpers take the TLS version and lay out the record with or without an explicit IV. The MAC check is skipped when there is no MAC. - test_tls_cbc_ver() runs a table of records through OpenSSL alone or across OpenSSL and wolfProvider; test_tls12_cbc and test_tls12_cbc_ossl use it. - Add test_tls10_cbc and test_dtls12_cbc: AES128-SHA records with MtE and with ETM at TLS1_VERSION and DTLS1_2_VERSION, across OpenSSL and wolfProvider. - qt5network5 expects 521 passed QSslSocket tests for FIPS and 522 for non-FIPS, where tst_QSslSocket::pskServer(WithoutProxy) now passes. The check matches the Totals: line of the test log. Issue: F-14604
yosuke-wolfssl
force-pushed
the
fix/f-14604
branch
from
October 7, 2026 05:24
51d5707 to
a198e39
Compare
yosuke-wolfssl
added
ci:all
Contributor
Author
|
Jenkins retest this please |
1 similar comment
Contributor
Author
|
Jenkins retest this please |
ColtonWilley
approved these changes
Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
wp_aes_block_tls_dec_record()always removed a 16-byte explicit IV from a decrypted AES-CBC TLS record, whatever the TLS version. TLS 1.0 records have no explicit IV; their first block is payload, and OpenSSL's record layer does not skip it. The length handed back was 16 bytes short, so any TLS 1.0 + AES-CBC connection where wolfProvider decrypts fails during the handshake:bad_record_mac.Reaching this takes TLS 1.0 enabled (
MinProtocol=TLSv1,@SECLEVEL=0) with wolfProvider supplying AES-CBC.Fix (
src/wp_aes_block.c)The explicit IV length now comes from
tls_version, as in OpenSSL'sossl_cipher_tlsunpadblock():tls_versionTLS1_1_VERSION,TLS1_2_VERSION,DTLS1_VERSION,DTLS1_2_VERSION,DTLS1_BAD_VERAES_BLOCK_SIZETLS1_VERSIONThe minimum record length check and both the ETM and MtE paths use that length. Closes f-14604.
Tests (
test/test_tls_cbc.c)test_tls10_cbc: AES128-SHA records, MtE and ETM, decrypted across OpenSSL and wolfProvider in both directions. Without the fix it fails withgot 22, expected 38.test_dtls12_cbc: the same records atDTLS1_2_VERSION. It guards the new version list: dropping theDTLS1_2_VERSIONcase makes it fail.CI (
.github/workflows/qt5network5.yml)The QSslSocket check expects an exact pass count. With this fix,
tst_QSslSocket::pskServer(WithoutProxy)passes on non-FIPS: it runs TLS 1.0 withPSK-AES256-CBC-SHA, and the handshake used to fail. That is the only test that changed result. Non-FIPS now expects 522 passed (was 521); FIPS stays at 521. The check matches theTotals:line.Verification
s_serverwith wolfProvider against stocks_clienton TLS 1.0 /AES128-SHA: the handshake failed before the fix (ETM and-no_etm) and exchanges data after it.