Fix a few TLS compliance issues#10594
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens TLS compliance-related behavior in the core handshake and exporter paths, primarily by enforcing correct timing/validity constraints for keying material export and session ticket usage.
Changes:
- Refuse
wolfSSL_export_keying_material()calls until the handshake has fully completed (per RFC 8446 / RFC 5705 intent). - Suppress use of expired stored session tickets (fall back to full handshake instead of attempting resumption).
- Map
BUFFER_Etodecode_errorinTranslateErrorToAlert().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ssl.c |
Adds a “handshake complete” gate before exporting keying material. |
src/internal.c |
Adds client-side session ticket expiry suppression and extends alert translation for buffer errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ssl->options.handShakeDone == 0 || | ||
| ssl->options.handShakeState != HANDSHAKE_DONE) { | ||
| WOLFSSL_MSG("Handshake not complete; refusing keying-material export"); | ||
| return WOLFSSL_FAILURE; | ||
| } |
|
…hout tls.c/tls13.c. Thanks to Ben Smyth for the report.
Thanks to Ben Smyth for the report.
Thanks to Ben Smyth for the report.
…set. This should fix hostap EAP-FAST failures.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 3 posted, 1 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] Expired-ticket branch drops SessionTicket extension entirely; client may no longer obtain a fresh ticket —
src/internal.c:31665-31687 - [Medium] No tests added for the three new compliance behaviors —
tests/api.c - [Low] Redundant dual condition in keying-material handshake-complete gate —
src/ssl.c:4076-4080
Skipped findings
- [Low]
Preprocessor directives indented to code column instead of repo convention
Review generated by Skoll
| * If bornOn is 0 or the secret callback is set, it is assumed that | ||
| * the session is being externally managed and this check is | ||
| * skipped. This is needed for hostap. */ | ||
| if (ssl->session->bornOn != 0 && |
There was a problem hiding this comment.
🟠 [Medium] Expired-ticket branch drops SessionTicket extension entirely; client may no longer obtain a fresh ticket
When the stored ticket is detected as expired the code sets ssl->options.resuming = 0 and skips TLSX_UseSessionTicket() entirely, so no SessionTicket extension is emitted in the ClientHello. Previously the (stale) ticket extension was sent, which still advertised ticket support and prompted the server to issue a NEW ticket during the resulting full handshake. The client-side empty/real SessionTicket extension is added in only two places: wolfSSL_UseSessionTicket()/wolfSSL_CTX_UseSessionTicket() (ssl_api_ext.c) and this resumption block. In a configuration where the extension is added only via this path (e.g. an app that loaded a ticket-bearing session with wolfSSL_set_session() without separately enabling the ticket extension on the new object/context), the expired branch now yields a ClientHello with no ticket extension at all, so the server will not issue a replacement ticket and the client permanently loses ticket-based resumption. Most apps enable tickets at the CTX level, where the empty extension persists and this does not bite, hence Medium/conditional. Separately, the comment's claim that suppressing the ticket 'saves a round-trip' is inaccurate: whether or not an expired ticket is sent, the server falls back to the same full handshake with the same number of round-trips.
Fix: Consider emitting an empty SessionTicket extension (mirroring the server-side blank-ticket behavior in tls.c TLSX_SessionTicket_Parse) in the expired branch so the client still requests a new ticket while not sending the stale one. Also correct the 'saves a round-trip' wording in the comment.
| * complete. Refuse the export until the handshake has completed so that | ||
| * a premature call cannot derive material from an uninitialised | ||
| * exporterSecret buffer. */ | ||
| if (ssl->options.handShakeDone == 0 || |
There was a problem hiding this comment.
🔵 [Low] Redundant dual condition in keying-material handshake-complete gate
The gate checks both handShakeDone == 0 and handShakeState != HANDSHAKE_DONE. These two flags are set together at every handshake-completion site (e.g. internal.c:8258-8259, 18136-18137, 18155-18156; tls13.c:11966, 12223) and handShakeState is never advanced past HANDSHAKE_DONE (the post-DONE state SERVER_FINISHED_ACKED lives in serverState, not handShakeState). The established idiom elsewhere (ssl.c:1025, internal.c:22140) uses handShakeDone == 0 alone. The double check is harmless and slightly more defensive, but inconsistent with the rest of the codebase. This is informational only — not a correctness problem.
Fix: Optionally simplify to the single handShakeDone == 0 check to match existing usage, or keep as-is if the extra belt-and-suspenders is intentional.
dgarske
left a comment
There was a problem hiding this comment.
See skoll review #10594 (review)
Description
Partially fixes zd#21873
Testing
Built in tests
Checklist