Disallow matching URI type in CheckForAltNames. NULL *response on error in wolfSSL_d2i_OCSP_RESPONSE.#10509
Disallow matching URI type in CheckForAltNames. NULL *response on error in wolfSSL_d2i_OCSP_RESPONSE.#10509kareem-wolfssl wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses two certificate/OCSP edge cases: (1) TLS hostname matching should not treat URI SANs as DNSName SANs, and (2) wolfSSL_d2i_OCSP_RESPONSE() should NULL out a caller-supplied *response on failure to avoid leaving a dangling pointer.
Changes:
- Update
CheckForAltNames()to skipuniformResourceIdentifierSANs when performing DNS hostname checks. - Update
wolfSSL_d2i_OCSP_RESPONSE()error paths to clear*responsewhen the caller passed a reusable response pointer. - Add regression tests covering URI SAN hostname-matching behavior and OCSP response reuse failure semantics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
wolfcrypt/src/asn.c |
Updates in-code documentation clarifying URI SAN handling vs CN fallback behavior. |
src/internal.c |
Skips URI SAN entries during DNS hostname matching in CheckForAltNames(). |
src/ocsp.c |
Clears caller’s *response on allocation/decode failures in wolfSSL_d2i_OCSP_RESPONSE(). |
tests/api/test_ocsp.c |
Adds a test to ensure *response is NULLed on reuse + decode failure. |
tests/api/test_certman.h |
Registers a new certman API test. |
tests/api/test_certman.c |
Adds a test ensuring URI SANs don’t satisfy DNS hostname checks (and validates behavior when DNS SAN is also present). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (response != NULL && *response == resp) | ||
| *response = NULL; | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
@kareem-wolfssl this might be right. Using a unified exit could help clean this up.
There was a problem hiding this comment.
This should have been fixed in c54e0f3 with the calls to wolfSSL_OCSP_RESPONSE_free.
Unified exit added in the most recent commit.
|
|
The changes look good. Thank you for the prompt response. |
| continue; | ||
| } | ||
|
|
||
| /* RFC 6125 Sec. 6.4 / RFC 9525 Sec. 6.3: a DNS-ID reference |
There was a problem hiding this comment.
RFC 6125 is obsoleted by RFC 9525. No point referencing out of date specs.
| @@ -1315,6 +1324,8 @@ OcspResponse* wolfSSL_d2i_OCSP_RESPONSE(OcspResponse** response, | |||
| /* for just converting from a DER to an internal structure the CA may | |||
| * not yet be known to this function for signature verification */ | |||
| wolfSSL_OCSP_RESPONSE_free(resp); | |||
| if (response != NULL && *response == resp) | |||
| *response = NULL; | |||
| return NULL; | |||
There was a problem hiding this comment.
This is getting really messy. Please refactor it into a single section and goto on error.
| * - CheckForAltNames (TLS hostname matching): skips ASN_URI_TYPE | ||
| * for DNS hostname checks (RFC 6125 Sec. 6.4 / RFC 9525 Sec. 6.3) | ||
| * but URI SAN presence still suppresses CN fallback (RFC 6125 | ||
| * Sec. 6.4.4) because URI-ID is a distinct presented identifier. |
Thanks to Haruki Oyama (Waseda University) for the report.
Thanks to Zou Dikai for the report.
Remove outdated RFC, refactor into single error case, guard against negative/0 len and NULL *data pointer, don't set ownStatus until status is confirmed non-NULL.
Description
Fixes zd#21863, zd#21865
Testing
Built in tests, provided reproducers
Checklist