-
Notifications
You must be signed in to change notification settings - Fork 1k
Conformance to RFC 9846: New version of TLS 1.3 specification #11215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,7 +104,8 @@ | |
| * WOLFSSL_TLS13_NO_PEEK_HANDSHAKE_DONE: | ||
| * Disable peek returning WANT_READ for tickets default: off | ||
| * WOLFSSL_TLS13_IGNORE_AEAD_LIMITS: | ||
| * Ignore AEAD message limits from RFC 8446 default: off | ||
| * Ignore AEAD message limits from RFC 9846 5.5, which | ||
| * makes observing them a MUST default: off | ||
| * WOLFSSL_DTLS13_SEND_MOREACK_DEFAULT: | ||
| * Send more ACKs by default in DTLS 1.3 default: off | ||
| * | ||
|
|
@@ -23961,7 +23962,7 @@ static void LogAlert(int type) | |
| /* process alert, return level */ | ||
| #ifndef NO_SESSION_CACHE | ||
| /* RFC 5246 Section 7.2.2: a TLS 1.2 session whose connection is terminated by a | ||
| * fatal alert MUST be invalidated so it cannot be resumed. (TLS 1.3 RFC 8446 | ||
| * fatal alert MUST be invalidated so it cannot be resumed. (TLS 1.3 RFC 9846 | ||
| * Section 6.2 only requires closing the connection, but evicting here too is | ||
| * sound defense-in-depth.) Evict the cached session (which also drops any | ||
| * associated ticket). Acts on an established connection or an in-progress | ||
|
|
@@ -23975,7 +23976,7 @@ static void InvalidateSessionOnFatalAlert(WOLFSSL* ssl) | |
| return; | ||
| /* Don't evict on an unauthenticated record: a TLS 1.3 plaintext alert | ||
| * received under encryption (current record not decrypted) is rejected (or | ||
| * ignored) by DoAlert, and the teardown alert routes back here. RFC 8446 | ||
| * ignored) by DoAlert, and the teardown alert routes back here. RFC 9846 | ||
| * 6.2 doesn't require TLS 1.3 eviction; TLS 1.2 alerts are plaintext so are | ||
| * unaffected. */ | ||
| if (IsAtLeastTLSv1_3(ssl->version) && IsEncryptionOn(ssl, 0) && | ||
|
|
@@ -24039,10 +24040,16 @@ static int DoAlert(WOLFSSL* ssl, byte* input, word32* inOutIdx, int* type) | |
| { | ||
| ssl->alert_history.last_rx.code = code; | ||
| ssl->alert_history.last_rx.level = level; | ||
| if (level == alert_fatal) { | ||
| /* RFC 9846 Section 6.1: "user_canceled" only "generally" has | ||
| * AlertLevel=warning, and a receiver SHOULD keep reading until | ||
| * "close_notify" arrives. The level byte is meaningless in TLS 1.3, | ||
| * so do not let a peer that sends the alert at fatal level tear the | ||
| * connection down. */ | ||
| if (level == alert_fatal && | ||
| !(IsAtLeastTLSv1_3(ssl->version) && code == user_canceled)) { | ||
| ssl->options.isClosed = 1; /* Don't send close_notify */ | ||
| } | ||
| /* RFC 8446 Section 6.2: In TLS 1.3, all error alerts are implicitly | ||
| /* RFC 9846 Section 6.2: In TLS 1.3, all error alerts are implicitly | ||
| * fatal regardless of the AlertLevel byte. */ | ||
| if (IsAtLeastTLSv1_3(ssl->version) && | ||
| code != close_notify && code != user_canceled) { | ||
|
|
@@ -24094,12 +24101,16 @@ static int DoAlert(WOLFSSL* ssl, byte* input, word32* inOutIdx, int* type) | |
| } | ||
| #ifndef NO_SESSION_CACHE | ||
| /* Validated fatal alert: invalidate the session so it can't be resumed | ||
| * (RFC 5246 7.2.2; in TLS 1.3 all error alerts are fatal, RFC 8446 | ||
| * 6.2). */ | ||
| if (*type != close_notify && | ||
| (level == alert_fatal || | ||
| (IsAtLeastTLSv1_3(ssl->version) && *type != user_canceled))) | ||
| * (RFC 5246 7.2.2; in TLS 1.3 all error alerts are fatal, RFC 9846 | ||
| * 6.2). "close_notify" is not an error, and "user_canceled" is exempt | ||
| * in TLS 1.3 at any AlertLevel (RFC 9846 6.1). */ | ||
| if (IsAtLeastTLSv1_3(ssl->version)) { | ||
| if (*type != close_notify && *type != user_canceled) | ||
| InvalidateSessionOnFatalAlert(ssl); | ||
| } | ||
| else if (level == alert_fatal && *type != close_notify) { | ||
| InvalidateSessionOnFatalAlert(ssl); | ||
| } | ||
| #endif | ||
| } | ||
| return level; | ||
|
|
@@ -24949,7 +24960,10 @@ static int DoProcessAlertRecord(WOLFSSL* ssl) | |
| WOLFSSL_MSG("got ALERT!"); | ||
| ret = DoAlert(ssl, ssl->buffers.inputBuffer.buffer, | ||
| &ssl->buffers.inputBuffer.idx, &type); | ||
| if (ret == alert_fatal) | ||
| /* 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)) | ||
| return FATAL_ERROR; | ||
| else if (ret < 0) | ||
| return ret; | ||
|
|
@@ -24965,7 +24979,7 @@ static int DoProcessAlertRecord(WOLFSSL* ssl) | |
| if (type == decrypt_error) | ||
| return FATAL_ERROR; | ||
|
|
||
| /* RFC 8446 Section 6.2: In TLS 1.3, all error alerts MUST | ||
| /* RFC 9846 Section 6.2: In TLS 1.3, all error alerts MUST | ||
| * be treated as fatal regardless of the AlertLevel byte. | ||
| * Only close_notify (handled above) and user_canceled | ||
| * are exempt. */ | ||
|
|
@@ -28594,7 +28608,7 @@ int IsSCR(WOLFSSL* ssl) | |
| !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS) | ||
| /* | ||
| * Enforce limits specified in | ||
| * https://www.rfc-editor.org/rfc/rfc8446#section-5.5 | ||
| * https://www.rfc-editor.org/rfc/rfc9846#section-5.5 | ||
| */ | ||
| static int CheckTLS13AEADSendLimit(WOLFSSL* ssl) | ||
| { | ||
|
|
@@ -28664,6 +28678,20 @@ static int CheckTLS13AEADSendLimit(WOLFSSL* ssl) | |
| if (w64GTE(seq, limit)) { /* cppcheck-suppress uninitvar | ||
| * (false positive from cppcheck-2.13.0) | ||
| */ | ||
| #ifdef WOLFSSL_EARLY_DATA | ||
| /* RFC 9846 Section 5.5: a KeyUpdate cannot be performed for early | ||
| * data, so a sender MUST NOT exceed the limits while sending it. | ||
| * There is no way to rekey at this point - the handshake has not | ||
| * finished, so a KeyUpdate here would be out of order - and the write | ||
| * has to fail instead. */ | ||
| if (ssl->options.side == WOLFSSL_CLIENT_END && | ||
| ssl->earlyData != no_early_data && | ||
| ssl->earlyData != done_early_data) { | ||
| WOLFSSL_MSG("AEAD limit reached while sending early data"); | ||
| WOLFSSL_ERROR_VERBOSE(TOO_MUCH_EARLY_DATA); | ||
| return TOO_MUCH_EARLY_DATA; | ||
|
SparkiDev marked this conversation as resolved.
|
||
| } | ||
| #endif | ||
| return Tls13UpdateKeys(ssl); /* Need to generate new keys */ | ||
| } | ||
|
|
||
|
|
@@ -28926,6 +28954,17 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) | |
| #if defined(WOLFSSL_TLS13) && !defined(WOLFSSL_TLS13_IGNORE_AEAD_LIMITS) | ||
| if (IsAtLeastTLSv1_3(ssl->version)) { | ||
| ret = CheckTLS13AEADSendLimit(ssl); | ||
| #ifdef WOLFSSL_EARLY_DATA | ||
| /* Hitting the early data limit part way through a multi-record | ||
| * write must not throw away the records already emitted in this | ||
| * call: the caller is told to send the remainder over the | ||
| * 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 [Low] SendData returns a short write without partialWrite enabled · TLS protocol issues The new Fix: Record a sticky indicator (e.g. set |
||
| break; | ||
| } | ||
| #endif | ||
| if (ret != 0) { | ||
| ssl->error = ret; | ||
| return WOLFSSL_FATAL_ERROR; | ||
|
|
@@ -38336,7 +38375,14 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, | |
| int TranslateErrorToAlert(int err) | ||
| { | ||
| switch (err) { | ||
| /* RFC 9846 Section 4.3 requires a "decode_error" alert when an | ||
| * extension has data left over after its structure is parsed, and | ||
| * Section 6.2 defines the alert for any field out of range or | ||
| * message of incorrect length. The extension parsers report those | ||
| * 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [Medium] BUFFER_E - decode_error makes the DTLS stateless ClientHello path send a fatal alert and stick alert_history at… · Incorrect error handling
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 |
||
| return decode_error; | ||
| case WC_NO_ERR_TRACE(EXT_NOT_ALLOWED): | ||
| case WC_NO_ERR_TRACE(PEER_KEY_ERROR): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 [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-leveluser_canceledarriving 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 onssl->versionalone.