TLS Extensions: add more extensions and improve current - #11362
Conversation
81d6404 to
e16541a
Compare
|
646253f to
55ec736
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11362
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Added support for record_size_limit in TLS 1.3 and TLS 1.2. Added compress_certificate support for TLS 1.3. Added signed_certificate_timestamp TLS 1.2 send and TLS 1.2 and 1.3 receive. Accepts server_name in CertificateRequest. Added API for setting signature algorithms for signature_algorithms_cert. Tests added and interop performed where possible.
55ec736 to
72e8586
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11362
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| if (!isServer && ssl->recordSizeLimit != 0 | ||
| #ifdef HAVE_MAX_FRAGMENT | ||
| && (ssl->recordSizeLimitSet || | ||
| TLSX_Find(ssl->extensions, |
There was a problem hiding this comment.
record_size_limit default ignores a CTX-level max_fragment_length · Logic errors
The guard that suppresses the default record_size_limit when the application asked for max_fragment_length only searches ssl->extensions, but wolfSSL_CTX_UseMaxFragment() stores the extension on ctx->extensions. Such a client advertises both; a peer following RFC 8449 Sect. 5 then ignores max_fragment_length, so it is never echoed and ssl->max_fragment never takes effect.
Related known finding #7004 (similar but distinct): Both concern record_size_limit support, but #7004 alleges the extension is entirely absent from registry parsing/emission, whereas this faults default-extension suppression when a CTX-level max_fragment_length is configured. The operations, root causes, and required patches differ.
Fix: Fall back to TLSX_Find(ssl->ctx->extensions, TLSX_MAX_FRAGMENT_LENGTH) as TLSX_MFL_Parse() does at src/tls.c:3387.
| if (ssl->sendingCompCert || | ||
| ((ssl->fragOffset == 0) && UseCompressedCertificate(ssl))) { | ||
| ssl->sendingCompCert = 1; | ||
| return SendTls13CompressedCertificate(ssl); |
There was a problem hiding this comment.
Compressed Certificate path skips the client's CertReqCtx release · Resource leaks on error paths
The compressed-certificate branch returns before the epilogue at src/tls13.c:11220 that pops and frees ssl->certReqCtx on a client. In-handshake client auth allocates a zero-length node (src/tls13.c:6388), and UseCompressedCertificate() permits compression for it, so the node stays allocated on the list for the life of the connection.
Fix: Perform the same client-side certReqCtx pop and free before returning from the compressed branch, when the send did not end in WANT_WRITE.
| /* What this end offered, as opposed to what the build can | ||
| * decompress: a CompressedCertificate is only acceptable when | ||
| * this handshake asked for one. */ | ||
| ssl->certCompAdvertised = 1; |
There was a problem hiding this comment.
Server sets certCompAdvertised without sending compress_certificate, defeating the unsolicited-CompressedCertificate… · TLS protocol issues
ssl->certCompAdvertised = 1 is set with no isServer check, so a TLS 1.3 server sets it while parsing every ClientHello — before deciding whether to send a CertificateRequest (the only message in which a server advertises compress_certificate). The !ssl->certCompAdvertised guard in SanityCheckTls13MsgReceived therefore never fires server-side, and a client that was never asked for a certificate can drive DoTls13CompressedCertificate into a WOLFSSL_MAX_CERT_COMP_SZ allocation plus wc_DeCompress.
Related known finding #7644 (similar but distinct): Both allow an unadvertised peer-selected certificate-related protocol choice to pass a missing solicitation invariant, but #7644 parses unsolicited certificate_type values on the client while this incorrectly sets server certCompAdvertised and admits an unsolicited CompressedCertificate. The roles, operations, causes, and patches are separate.
Fix: Set certCompAdvertised only when the extension is actually emitted: for the client in the ClientHello, and for the server when the CertificateRequest is written.
Description
Added support for record_size_limit in TLS 1.3 and TLS 1.2. Added compress_certificate support for TLS 1.3.
Added signed_certificate_timestamp TLS 1.2 send and TLS 1.2 and 1.3 receive. Accepts server_name in CertificateRequest.
Added API for setting signature algorithms for signature_algorithms_cert.
Tests added and interop performed where possible.
Testing
Regression tested TLS.