Fenrir fixes#10398
Open
julek-wolfssl wants to merge 9 commits intowolfSSL:masterfrom
Open
Conversation
Server-side OCSP stapling was unconditionally folding OCSP_CERT_REVOKED, OCSP_CERT_UNKNOWN, and OCSP_LOOKUP_FAIL into a success result so a stapling failure would not break the handshake. OCSP_CERT_REVOKED, however, is an explicit positive assertion of revocation by the responder and must not be ignored: silently suppressing it lets a server keep advertising a revoked certificate to clients that rely on stapling for revocation status. Drop OCSP_CERT_REVOKED from the suppression list in CreateOcspResponse, the CSR2_OCSP_MULTI handler in SendCertificateStatus, and ProcessChainOCSPRequest. Continue suppressing OCSP_CERT_UNKNOWN and OCSP_LOOKUP_FAIL, which are true soft-fail responder conditions where the responder cannot answer. F-1820
DTLS minor versions decrease as the protocol version increases (DTLS 1.0=0xFF, DTLS 1.2=0xFD, DTLS 1.3=0xFC), but the ticket version comparisons in DoClientTicketCheckVersion used the TLS direction unconditionally. As a result a DTLS server resuming a session ticket from a different DTLS version could land on the wrong branch: a ticket from a newer DTLS version would be treated as a downgrade instead of being rejected, and a ticket from an older DTLS version would be flagged as 'greater version' and refused outright. The minDowngrade check at the bottom had the same inversion bug. Branch on ssl->options.dtls so the greater-version, lesser-version, and minDowngrade comparisons all use the right direction for the active protocol family. TLS behavior is unchanged. F-1828
When resuming a session wolfSSL_SetSession unconditionally overwrote ssl->version with the version stored in the cached session, even if that version was below the WOLFSSL's configured minDowngrade. The overwritten version then fed straight into SendClientHello, so a client configured to require TLS 1.2 or higher could still emit a ClientHello advertising e.g. TLS 1.0 when resuming an old cached session. The ServerHello path catches the actual downgrade, but the ClientHello version is already a protocol-conformance issue and can confuse middleboxes. Reject the session if its stored minor version is below ssl->options.minDowngrade. The check is DTLS-aware: DTLS minor versions decrease as the protocol version increases, so the direction of the comparison is flipped for DTLS. F-2105
DoTls13NewSessionTicket rejects a ticket lifetime greater than MAX_LIFETIME (RFC 8446 Section 4.6.1, 7 days), but no test exercised the rejection: every server in the suite stays well within the limit, so a mutation deleting that bound check would go unnoticed. Add a manual memio test that pokes ctx_s->ticketHint to MAX_LIFETIME + 1 (the public setter clamps to 604800), runs a full TLS 1.3 handshake, and reads the post-handshake NewSessionTicket on the client. The test confirms the over-limit lifetime surfaces from wolfSSL_read as SERVER_HINT_ERROR. F-2121
wolfSSL_set_SSL_CTX is the OpenSSL-compatible entry point that an SNI callback uses to swap in the per-vhost certificate during the handshake. By design it only copies the certificate chain and private key from the new CTX. Verification settings, the trusted CA store, CRL/OCSP configuration, minimum key-size requirements, and cipher/version policy stay attached to the original CTX. For multi-tenant servers where each virtual host has its own security policy, that means one host's verification rules silently apply to a connection meant for another. Expand the leading comment with an explicit SECURITY WARNING that lists the settings which are NOT inherited and points at the WOLFSSL*-level setters callers must use inside the SNI callback when virtual hosts have different policies. The behavior of the function is unchanged. F-2902
CheckOcspRequest used to return CERT_GOOD whenever a certificate lacked an AIA extension and no override URL was configured, with the rationale 'Cert has no OCSP URL, assuming CERT_GOOD'. That is a fail-open soft-fail: an operator who turned on WOLFSSL_OCSP_CHECKALL expecting every certificate in the chain to be revocation-checked would still silently accept a certificate that omits its OCSP responder URL, letting a misconfigured (or attacker-controlled) issuer bypass revocation for non-stapled flows. Gate the fail-open path on cm->ocspCheckAll. When the caller has asked for full-chain OCSP checking, return OCSP_NEED_URL so the chain is refused. The legacy behavior is preserved when ocspCheckAll is not set, keeping the soft-fail default for plain WOLFSSL_OCSP_ENABLE users. F-3227
Contributor
There was a problem hiding this comment.
Pull request overview
This PR bundles several security and protocol-correctness fixes in wolfSSL, mainly around OCSP handling, TLS/DTLS session resumption version checks, and TLS 1.3 session ticket validation, plus related regression coverage and API documentation.
Changes:
- Adds a TLS 1.3 regression test for rejecting
NewSessionTicketlifetimes above RFC 8446’s maximum. - Tightens OCSP behavior by no longer soft-failing revoked responses and by changing how missing responder URLs are handled under
OCSP_CHECKALL. - Adds minimum-protocol/version checks for cached sessions and DTLS tickets, and expands
wolfSSL_set_SSL_CTX()guidance for SNI-driven context switching.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/api/test_tls13.h |
Registers the new TLS 1.3 regression test. |
tests/api/test_tls13.c |
Adds a memio test for oversized TLS 1.3 ticket lifetime rejection. |
src/tls.c |
Updates OCSP chain-response handling to stop suppressing revoked status. |
src/ssl.c |
Expands the wolfSSL_set_SSL_CTX() warning/documentation for SNI use cases. |
src/ssl_sess.c |
Rejects cached sessions below the configured minimum protocol version. |
src/ocsp.c |
Changes missing-URL handling for OCSP requests when CHECKALL is enabled. |
src/internal.c |
Updates stapling OCSP soft-fail rules and fixes DTLS ticket version comparisons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+548
to
+556
| * configured. The legacy behavior is to fail-open and return CERT_GOOD, | ||
| * but a caller that asked for ocspCheckAll requested strict checking | ||
| * of the entire chain - silently accepting a cert that omits its AIA | ||
| * extension would let a non-stapled flow bypass revocation entirely. | ||
| * Surface OCSP_NEED_URL so the caller can refuse the chain. */ | ||
| if (ocsp->cm->ocspCheckAll) { | ||
| WOLFSSL_MSG("Cert has no OCSP URL and ocspCheckAll is set"); | ||
| return OCSP_NEED_URL; | ||
| } |
Comment on lines
+15759
to
+15776
| * Per-host security policy is NOT copied from the new CTX. In particular | ||
| * the following settings are inherited from the original CTX and will | ||
| * apply to the connection regardless of which virtual host the SNI | ||
| * callback selects: | ||
| * - peer-verification mode (SSL_VERIFY_PEER, verifyNone, failNoCert) | ||
| * - the trusted CA store and any verify callback | ||
| * - CRL and OCSP configuration (including OCSP must-staple policy) | ||
| * - minimum key-size requirements | ||
| * - cipher-suite preferences and protocol-version bounds | ||
| * | ||
| * Callers that need different verification policies per virtual host | ||
| * (e.g. one host requires mTLS while another does not, or hosts trust | ||
| * different CA bundles) MUST propagate those settings onto the WOLFSSL* | ||
| * manually inside the SNI callback (e.g. via wolfSSL_set_verify, | ||
| * wolfSSL_UseCRL, wolfSSL_UseOCSP, wolfSSL_set_cipher_list, ...). | ||
| * Failing to do so silently keeps the original CTX's policy and can | ||
| * result in client certificates being skipped for a host that requires | ||
| * them, or peer certificates from the wrong CA bundle being accepted. |
Comment on lines
+1609
to
+1613
| /* Reject sessions whose protocol version is below the configured | ||
| * minimum so a stale cached session cannot make the client send a | ||
| * ClientHello advertising a version it isn't allowed to negotiate. | ||
| * DTLS minor versions are inverted: a higher minor means an older | ||
| * protocol, so the comparison flips. */ |
Comment on lines
+39624
to
+39629
| /* DTLS minor versions decrease as the protocol version increases | ||
| * (DTLS 1.0=0xFF, DTLS 1.2=0xFD, DTLS 1.3=0xFC), so the version | ||
| * comparisons are inverted relative to TLS. */ | ||
| byte greaterVersion; | ||
| byte lesserVersion; | ||
| byte belowMinDowngrade; |
| * (e.g. one host requires mTLS while another does not, or hosts trust | ||
| * different CA bundles) MUST propagate those settings onto the WOLFSSL* | ||
| * manually inside the SNI callback (e.g. via wolfSSL_set_verify, | ||
| * wolfSSL_UseCRL, wolfSSL_UseOCSP, wolfSSL_set_cipher_list, ...). |
Comment on lines
+15771
to
+15776
| * different CA bundles) MUST propagate those settings onto the WOLFSSL* | ||
| * manually inside the SNI callback (e.g. via wolfSSL_set_verify, | ||
| * wolfSSL_UseCRL, wolfSSL_UseOCSP, wolfSSL_set_cipher_list, ...). | ||
| * Failing to do so silently keeps the original CTX's policy and can | ||
| * result in client certificates being skipped for a host that requires | ||
| * them, or peer certificates from the wrong CA bundle being accepted. |
|
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.
F-3227
F-2902
F-2121
F-2105
F-1828
F-1820