Fix session ticket key rotation when hint exceeds key lifetime#10822
Fix session ticket key rotation when hint exceeds key lifetime#10822mattia-moffa wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a session-ticket encryption key rotation edge case where ticketHint can exceed the default ticket key lifetime, previously causing TicketEncCbCtx_ChooseKey() to return BAD_STATE_E and break subsequent handshakes when reusing a server WOLFSSL_CTX.
Changes:
- Update default session ticket key selection/rotation logic to avoid returning
BAD_STATE_Ewhen both keys are decrypt-valid but neither coversticketHint. - Add a new API regression test that reuses server/client contexts across multiple handshakes with an oversized ticket hint.
- Register the new test in the ssl extension test list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/internal.c |
Adjusts default ticket key selection behavior when neither key covers the configured hint window. |
tests/api/test_ssl_ext.c |
Adds a regression test covering repeated handshakes with a reused server context and oversized ticket hint. |
tests/api/test_ssl_ext.h |
Exposes/registers the new regression test in the ssl_ext group. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The default ticket encryption callback rotates two keys and can only honor a hint below WOLFSSL_TICKET_KEY_LIFETIME/2; a larger hint previously left no key available for encryption, failing every handshake after the first on a reused server CTX. Now these values are rejected.
mattia-moffa
force-pushed
the
20260629-ticket-hint-window-check
branch
from
3125631 to
9514b21
Compare
|
Jenkins retest this please |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: REQUEST_CHANGES
Findings: 5 total — 4 posted, 1 skipped
2 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] [review+review-security] Hint rejection also fires with a custom TicketEncCb -- macro gate reflects macro presence, not the default callback —
src/ssl_api_ext.c:1073-1080 - [Low] [review] Compile-time and runtime hint checks disagree by one for odd WOLFSSL_TICKET_KEY_LIFETIME —
wolfssl/internal.h:2050-2052
Findings not tied to a diff line
New hint cap regresses OpenSSL-compat SSL_CTX_set_timeout for common timeouts (= 30 min)
File: src/ssl_api_ext.c:1073-1080 (manifests via src/ssl_sess.c:958-968)
Function: wolfSSL_CTX_set_TicketHint / wolfSSL_CTX_set_timeout
Severity: High
Category: Regression
wolfSSL_CTX_set_TicketHint is also called internally by the OpenSSL-compat wolfSSL_CTX_set_timeout (SSL_CTX_set_timeout). Under OPENSSL_EXTRA && HAVE_SESSION_TICKET && !NO_WOLFSSL_SERVER, wolfSSL_CTX_set_timeout(ctx, to) forwards the session timeout straight into wolfSSL_CTX_set_TicketHint(ctx, (int)to) (ssl_sess.c:968). The new check rejects any hint >= WOLFSSL_TICKET_KEY_LIFETIME / 2, i.e. any value >= 1800 seconds (30 min) with the default 3600s key lifetime. Session timeouts of 30+ minutes are the norm (OpenSSL's own default is 7200s; values like 3600/7200/86400 are routine), so SSL_CTX_set_timeout(ctx, 7200) -- which previously succeeded (only >604800 was rejected) -- now returns BAD_FUNC_ARG. This is a backward-incompatible break of a widely-used public compat API. Worse, ctx->timeout is already assigned at ssl_sess.c:958 BEFORE the ticket-hint call fails, so the session-cache timeout is silently applied while the function reports failure, leaving inconsistent state for callers that check the return code. Severity views differ (review: High/BLOCK; review-security: Medium) -- the stricter High is kept because this breaks a public API for common inputs; the session-cache timeout is conceptually independent of ticket-key rotation. Not covered by CI: test_wolfSSL_CTX_set_timeout only uses values below 1800.
Recommendation: Decouple the session-cache timeout from the ticket-hint constraint. In wolfSSL_CTX_set_timeout, clamp the value passed to wolfSSL_CTX_set_TicketHint (e.g. min(to, WOLFSSL_TICKET_KEY_LIFETIME/2 - 1)) or ignore a ticket-hint rejection when setting the session timeout, so a large session timeout still succeeds and ctx state stays consistent. Keep the hard rejection only for the direct public setter if desired. Add a test_wolfSSL_CTX_set_timeout case with a value >= WOLFSSL_TICKET_KEY_LIFETIME/2 (e.g. 3600) to lock in the intended behavior.
Referenced code: src/ssl_api_ext.c:1073-1080 (manifests via src/ssl_sess.c:958-968) (10 lines)
No test covers the wolfSSL_CTX_set_timeout - set_TicketHint regression path
File: tests/api.c:27884-27913
Function: test_wolfSSL_CTX_set_timeout
Severity: Medium
Category: test
The new tests in test_ssl_ext.c exercise the direct setter, but the highest-impact behavior change is via the internal caller wolfSSL_CTX_set_timeout. The existing test_wolfSSL_CTX_set_timeout only uses small values (20, 30, 100), all below the new 1800s cap, so it will neither catch the regression nor lock in whatever the intended behavior becomes. Since the change silently alters what SSL_CTX_set_timeout returns for common values, this path needs explicit coverage.
Recommendation: Add a case under OPENSSL_EXTRA + HAVE_SESSION_TICKET + server that calls wolfSSL_CTX_set_timeout with a value >= WOLFSSL_TICKET_KEY_LIFETIME/2 (e.g. 3600) and asserts the intended outcome (success with clamped hint per the fix above), and verify ctx state is consistent.
Skipped findings
- [Info]
Stricter compile-time #error may break previously-valid custom lifetime/hint configurations
Review generated by Skoll
|
Jenkins retest this please: "Error cloning remote repo 'origin'". Timeout |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] [review+review-security] Oversized ticket hint returns FATAL and tears down the whole handshake instead of degrading to a ticket-less connection —
src/internal.c:41749 - [Low] [review] New failure uses WOLFSSL_TICKET_RET_FATAL while other encrypt-side returns in this function use REJECT —
src/internal.c:41751 - [Info] [review] Trivial custom ticket-encryption callback duplicated across two test files —
tests/api/test_ssl_ext.c:203
Findings not tied to a diff line
Compile-time ticket lifetime guard does not cover the new runtime 2*hint constraint
File: wolfssl/internal.h:2050
Function: DefTicketEncCb
Severity: Low
Category: Convention
internal.h statically enforces WOLFSSL_TICKET_KEY_LIFETIME > SESSION_TICKET_HINT_DEFAULT, but the new runtime rule requires 2 * ticketHint < WOLFSSL_TICKET_KEY_LIFETIME. With the defaults (lifetime 3600, default hint 300) there is no problem (600 < 3600). However, a user who overrides WOLFSSL_TICKET_KEY_LIFETIME to a value in the range (SESSION_TICKET_HINT_DEFAULT, 2*SESSION_TICKET_HINT_DEFAULT] (e.g. 400-600) passes the compile-time #error check yet now has every default-hint ticket handshake fail at runtime, with no build-time warning. This is a narrow, already-degenerate configuration, but the failure surfaces only at runtime.
Recommendation: Consider tightening the static assert to #if WOLFSSL_TICKET_KEY_LIFETIME <= 2 * SESSION_TICKET_HINT_DEFAULT (or documenting the 2*hint requirement near WOLFSSL_TICKET_KEY_LIFETIME) so users who shrink the key lifetime are warned at build time rather than seeing runtime handshake failures.
Referenced code: wolfssl/internal.h:2050-2052 (3 lines)
Review generated by Skoll
|
|
||
| /* The two-key rotation can only honor a hint below half the key lifetime. | ||
| * Refuse to issue a ticket whose advertised lifetime it cannot cover. */ | ||
| if (enc && 2 * ctx->ticketHint >= WOLFSSL_TICKET_KEY_LIFETIME) { |
There was a problem hiding this comment.
🟠 [Medium] Oversized ticket hint returns FATAL and tears down the whole handshake instead of degrading to a ticket-less connection · Logic
The new check if (enc && 2 * ctx->ticketHint >= WOLFSSL_TICKET_KEY_LIFETIME) return WOLFSSL_TICKET_RET_FATAL; runs on every ticket-issuing (enc) call of the default callback. FATAL (-1) propagates DefTicketEncCb -> CreateTicket (goto error) -> SendTls13NewSessionTicket / TLS1.2 SendTicket -> wolfSSL_accept returns WOLFSSL_FATAL_ERROR, so the entire TLS handshake fails and the client cannot connect at all. Session tickets are optional per RFC 8446, so a server that merely cannot issue a resumption ticket could instead complete the handshake and serve a ticket-less (no-resumption) connection; in TLS 1.3 the NewSessionTicket is sent after the handshake is already cryptographically complete, so a pure configuration problem is surfaced as a full connection failure. This is an easily-triggered functional regression, not attacker-controlled: in OpenSSL-compat builds (OPENSSL_EXTRA && HAVE_SESSION_TICKET && !NO_WOLFSSL_SERVER), wolfSSL_CTX_set_timeout(ctx, to) copies the session timeout straight into ctx->ticketHint (src/ssl_sess.c:968, clamped only to [0,604800]). With the default WOLFSSL_TICKET_KEY_LIFETIME of 3600, any timeout >= 1800 (e.g. 3600, or OpenSSL's common 7200) makes 2*ticketHint >= 3600 true, so EVERY handshake that issues a ticket now fails. BEFORE this PR the same misconfiguration let the FIRST connection succeed (it generated the secondary key and issued a ticket) and only failed reused-CTX connections thereafter (issue #10796); AFTER this PR even the first connection fails. The PR's own test fixture had to install a custom callback to avoid this new limit…
Fix: Confirm with maintainers that failing the handshake is the intended contract. Prefer a less destructive response: clamp the advertised ticket lifetime hint down to a value the two-key rotation can cover, or skip issuing the ticket (full handshake next time) via a distinct 'skip ticket' return path in CreateTicket, instead of returning FATAL. At minimum, guard the OpenSSL-compat propagation in wolfSSL_CTX_set_timeout so large session timeouts do not silently push ticketHint past WOLFSSL_TICKET_KEY_LIFETIME/2, and document in release notes that SSL_CTX_set_timeout / ticket-hint values >= half the ticket key lifetime disable default-callback tickets and will fail the handshake.
| * Refuse to issue a ticket whose advertised lifetime it cannot cover. */ | ||
| if (enc && 2 * ctx->ticketHint >= WOLFSSL_TICKET_KEY_LIFETIME) { | ||
| WOLFSSL_MSG("Ticket hint exceeds half the ticket key lifetime"); | ||
| return WOLFSSL_TICKET_RET_FATAL; |
There was a problem hiding this comment.
🔵 [Low] New failure uses WOLFSSL_TICKET_RET_FATAL while other encrypt-side returns in this function use REJECT · Convention
Every other failure on the encryption (enc) path inside DefTicketEncCb returns WOLFSSL_TICKET_RET_REJECT (RNG failure at 41781, encrypt failure at 41843), while this new check returns WOLFSSL_TICKET_RET_FATAL. On the encrypt/create path both values are treated identically by the caller (both hit goto error at internal.c:40393 and fail the handshake), so this is not a functional defect. FATAL arguably communicates 'permanent misconfiguration' better than REJECT, but the inconsistency within the same function is worth a deliberate choice.
Fix: Either keep FATAL intentionally (permanent config error) with a brief comment, or use REJECT to match the other encrypt-side error returns in this function.
| && !defined(NO_WOLFSSL_SERVER) | ||
| /* Trivial custom ticket encryption callback: it has no key-lifetime constraint, | ||
| * so it must be able to issue a ticket for any hint. */ | ||
| static int test_TicketHint_custom_encCb(WOLFSSL* ssl, |
There was a problem hiding this comment.
⚪ [Info] Trivial custom ticket-encryption callback duplicated across two test files · Style
test_TicketHint_custom_encCb (test_ssl_ext.c) and test_tls13_max_lifetime_ticketEncCb (test_tls13.c:5737-5754) are byte-for-byte equivalent trivial XOR-0xA5 ticket callbacks. Cross-file duplication of small test helpers is common in this suite and not harmful, but a shared helper in tests/utils.* would avoid two copies drifting.
Fix: Optional: factor the trivial XOR ticket callback into a shared test utility if it is reused again; otherwise leave as-is.
Description
Disallows a
ticketHint > WOLFSSL_TICKET_KEY_LIFETIME / 2.The default ticket encryption callback rotates two keys and can only honor a hint below half the ticket key lifetime; a larger hint previously left no key available for encryption, failing every handshake after the first on a reused server CTX. Now these values are rejected.
Fixes #10796
Testing
Checklist