Conformance to RFC 9846: New version of TLS 1.3 specification - #11215
Conformance to RFC 9846: New version of TLS 1.3 specification#11215SparkiDev wants to merge 1 commit into
Conversation
|
f0a3108 to
67bc08c
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11215
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
67bc08c to
a75c3a2
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11215
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
RFC 9846, 5.5 CheckTLS13AEADSendLimit(): at the AEAD limit while sending early data, return TOO_MUCH_EARLY_DATA instead of calling Tls13UpdateKeys(). A KeyUpdate there would go out pre-handshake. RFC 9846, 6.1 Three alert sites in DoAlert() / DoProcessAlertRecord(): TLS 1.3 user_canceled is now exempt from teardown and session invalidation at any AlertLevel, not just warning. TLS 1.2 unchanged. RFC 9846, 4.7.3 New Tls13KeyUpdateLimitReached() helper shared by send and receive paths. At the 2^48-1 cap, DoTls13KeyUpdate() drops a peer's update_requested and continues, rather than failing the connection. App-initiated wolfSSL_update_keys() still returns BAD_STATE_E. RFC 9846, 4.3 TranslateErrorToAlert() maps BUFFER_E to decode_error as well as BUFFER_ERROR — one case label covering 27 malformed-extension sites that previously aborted with no alert sent. Tests added.
a75c3a2 to
3ac56cc
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11215
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| * as either BUFFER_ERROR or the wolfCrypt BUFFER_E; both must map | ||
| * here, or the handshake aborts silently with no alert sent. */ | ||
| case WC_NO_ERR_TRACE(BUFFER_ERROR): | ||
| case WC_NO_ERR_TRACE(BUFFER_E): |
There was a problem hiding this comment.
🟠 [Medium] BUFFER_E - decode_error makes the DTLS stateless ClientHello path send a fatal alert and stick alert_history at… · Incorrect error handling
CheckDtlsCookie() (src/dtls.c:291,294) returns BUFFER_E for a malformed DTLS 1.3 cookie extension, which reaches DoClientHello() at src/internal.c:40993. That error is now translated to decode_error, so a fatal alert is sent to an address-unverified peer and DtlsIgnoreError(BUFFER_E) then zeroes ret and the object keeps running with alert_history.last_tx.level == alert_fatal, which permanently suppresses later alerts via the guards at src/internal.c:20676, src/internal.c:34575 and src/tls13.c:14996.
Related known finding #7568 (similar but distinct): Both concern fatal-alert behavior on DoClientHello error paths, but #7568 maps VERSION_ERROR from version negotiation to a missing/wrong protocol_version alert, while this maps malformed-cookie BUFFER_E to decode_error before DTLS address validation and leaves alert history poisoned. The faulting operations, causes, and required fixes differ.
Fix: Suppress the alert on the pre-cookie stateless path in DoClientHello(), or clear alert_history.last_tx in DtlsResetState().
| /* RFC 9846 Section 6.1: keep reading past a TLS 1.3 "user_canceled" until | ||
| * "close_notify" arrives, whatever AlertLevel the peer used. */ | ||
| if (ret == alert_fatal && | ||
| !(IsAtLeastTLSv1_3(ssl->version) && type == user_canceled)) |
There was a problem hiding this comment.
🔵 [Low] TLS 1.3 user_canceled exemption is applied before version negotiation, silently ignoring a pre-1.3 peer's fatal alert · Logic errors
IsAtLeastTLSv1_3(ssl->version) reflects the maximum supported version, not the negotiated one, until the ServerHello is processed. A fatal-level user_canceled arriving before negotiation is therefore exempted from teardown even when the peer is TLS 1.2, contradicting the PR's stated "TLS 1.2 unchanged" and leaving the connection in WANT_READ instead of failing. The same premature check exists at src/internal.c:24049.
Related known finding #6723 (similar but distinct): Both rely on IsAtLeastTLSv1_3(ssl->version) in early record processing, but #6723 applies TLS 1.3 CCS handling to DTLS 1.3 records, whereas this exempts a fatal user_canceled alert before TLS version negotiation. They affect different record types, functions, protocol roles, and require separate patches.
Fix: Gate the exemption on a negotiated TLS 1.3 connection (e.g. ssl->options.tls1_3) rather than on ssl->version alone.
| * completed handshake, and it can only work out the remainder | ||
| * from the returned count. Report the short write; the next call | ||
| * re-runs this check with nothing sent yet and fails properly. */ | ||
| if ((ret == WC_NO_ERR_TRACE(TOO_MUCH_EARLY_DATA)) && (sent > 0)) { |
There was a problem hiding this comment.
🔵 [Low] SendData returns a short write without partialWrite enabled · TLS protocol issues
The new break exits the send loop with ssl->error == 0 and returns sent < sz. Every other short return in SendData is gated on ssl->options.partialWrite (line 29193), so a caller relying on the default all-or-error contract silently drops the untransmitted tail of the early data.
Fix: Record a sticky indicator (e.g. set ssl->error = TOO_MUCH_EARLY_DATA before breaking) so the caller can distinguish this short write from a complete one.
| ExpectIntEQ(wolfSSL_connect(ssl_c), -1); | ||
| ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); | ||
|
|
||
| ExpectIntEQ(test_memio_inject_message(&test_ctx, 1, |
There was a problem hiding this comment.
🔵 [Low] New user_canceled tests only cover the plaintext pre-handshake alert · Missing edge-case coverage on a function the PR also changed
Both new alert tests inject an unencrypted user_canceled record while the client has only sent ClientHello. The RFC 9846 6.1 case the PR targets — an authenticated fatal user_canceled after the handshake, where the peer keeps reading until close_notify — never reaches DoProcessAlertRecord with keys.decryptedCur set, and no test confirms the connection still carries data afterwards.
Fix: Add a case that completes the handshake, has the peer send a fatal user_canceled over the encrypted channel, then asserts subsequent read/write still succeed.
Description
RFC 9846, 5.5
CheckTLS13AEADSendLimit(): at the AEAD limit while sending early data, return TOO_MUCH_EARLY_DATA instead of calling Tls13UpdateKeys(). A KeyUpdate there would go out pre-handshake.
RFC 9846, 6.1
Three alert sites in DoAlert() / DoProcessAlertRecord(): TLS 1.3 user_canceled is now exempt from teardown and session invalidation at any AlertLevel, not just warning. TLS 1.2 unchanged.
RFC 9846, 4.7.3
New Tls13KeyUpdateLimitReached() helper shared by send and receive paths. At the 2^48-1 cap, DoTls13KeyUpdate() drops a peer's update_requested and continues, rather than failing the connection. App-initiated wolfSSL_update_keys() still returns BAD_STATE_E.
RFC 9846, 4.3
TranslateErrorToAlert() maps BUFFER_E to decode_error as well as BUFFER_ERROR — one case label covering 27 malformed-extension sites that previously aborted with no alert sent.
Tests added.
Testing
Added tests.