Fixes in TLS ECH, handle empty records, and ASN len check#10187
Fixes in TLS ECH, handle empty records, and ASN len check#10187embhorn wants to merge 5 commits intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR tightens TLS 1.3 parsing and certificate verification logic by adding protections against duplicate extensions (ECH), limiting empty application-data records (DoS mitigation), and correctly enforcing RFC 5280 pathLen constraints in more cases.
Changes:
- Treat TLS ECH as duplicate-detectable in
TLSX_Parse()and add a targeted TLS 1.3 test. - Add an empty TLS application-data record rate limit (
WOLFSSL_MAX_EMPTY_RECORDS), new error code, and corresponding TLS 1.3 tests. - Fix
ParseCertRelative()pathLen enforcement for self-issued certificates and when KeyUsage is absent; add regression tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/wolfcrypt/settings.h |
Adds configurable WOLFSSL_MAX_EMPTY_RECORDS default for empty-record DoS mitigation. |
wolfssl/internal.h |
Tracks empty application-data records per connection via emptyRecordCount. |
wolfssl/error-ssl.h |
Adds EMPTY_RECORD_LIMIT_E error code for empty-record limit violations. |
wolfcrypt/src/asn.c |
Fixes RFC 5280 pathLen enforcement logic for self-issued certs and missing KeyUsage. |
src/tls.c |
Includes ECH in extension duplicate-detection gating. |
src/internal.c |
Enforces empty-record rate limit and exposes the new error reason string. |
tests/api/test_tls13.h |
Registers new TLS 1.3 tests for duplicate ECH and empty-record limit. |
tests/api/test_tls13.c |
Adds TLS 1.3 regression tests for duplicate ECH and empty-record limit behavior. |
tests/api.c |
Adds certificate-generation/verification regression tests for pathLen enforcement cases. |
Comments suppressed due to low confidence (2)
src/internal.c:1
- The limit check is off-by-one relative to the macro name: with
>= WOLFSSL_MAX_EMPTY_RECORDSafter pre-increment, the code rejects the Nth empty record whereN == WOLFSSL_MAX_EMPTY_RECORDS. If the intent is “allow up toWOLFSSL_MAX_EMPTY_RECORDSempty records and fail on the next one”, change the comparison logic (e.g., increment then check>, or check first then increment) so the name matches the enforced behavior.
src/internal.c:1 - The reason string is a bit tautological/uninformative (“error” twice). Consider aligning the text with the enum comment for clarity, e.g. “Too many empty records received” or “Empty record limit exceeded”, which reads better in logs and user-facing diagnostics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Reviewed all three commits. Fixes look correct. Fix 1: Duplicate ECH Extension (1f75deb) Adding Note: this fixes ECH specifically, but extension types > 62 outside the hardcoded exceptions still bypass duplicate detection. A more general approach (hash set for seen types) would future-proof this, though that's a larger change. Fix 2: Empty Record Limit (d2a8735) Counter with default 32 matching OpenSSL, reset on non-empty — correct. Both test cases (exceeded limit + counter reset) have good coverage. Issue: #if WOLFSSL_MAX_EMPTY_RECORDS > 255
#error "WOLFSSL_MAX_EMPTY_RECORDS must be <= 255 (counter is byte)"
#endifFix 3: pathLen for Self-Issued Certs (d6fa2b1) This is the most important one. Changes are correct:
Tests Thanks for the fast turnaround - all three issues addressed with tests in under a day is impressive. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Fixes zd21587
Testing
Added tests:
test_tls13_duplicate_ech_extensiontest_tls13_empty_record_limittest_PathLenSelfIssuedandtest_PathLenNoKeyUsageChecklist