Reject status_request_v2 ocsp-multi staples that bundle multiple SingleResponses#10747
Reject status_request_v2 ocsp-multi staples that bundle multiple SingleResponses#10747gasbytes wants to merge 1 commit into
Conversation
gasbytes
force-pushed
the
tls-1-2-ocsp-multi-stapl-check
branch
2 times, most recently
from
a1db565 to
1410138
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens OCSP stapling verification for status_request_v2 OCSP-multi by rejecting stapled BasicOCSPResponse values that bundle multiple SingleResponse entries, closing a case where a revoked certificate could be accepted if a benign SingleResponse appeared first.
Changes:
- Reject OCSP-multi (
WOLFSSL_CSR2_OCSP_MULTI) staples that contain more than oneSingleResponse. - Add a regression test that staples a multi-
SingleResponseleaf OCSP response where the revoked entry is not first. - Add a generated OCSP response blob and generator script entry to reproduce the behavior.
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 |
|---|---|
src/internal.c |
Adds a guard in DoCertificateStatus() to reject multi-SingleResponse staples for CSR2 OCSP-multi. |
tests/api/test_ocsp.c |
Adds a regression test and status callback that supplies the crafted multi-SingleResponse leaf staple. |
tests/api/test_ocsp.h |
Exposes the new regression test prototype. |
tests/api.c |
Registers the new test in the test runner. |
tests/api/test_ocsp_test_blobs.h |
Adds the new binary OCSP response blob used by the test. |
tests/api/create_ocsp_test_blobs.py |
Adds generation metadata for the new OCSP response blob. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gasbytes
force-pushed
the
tls-1-2-ocsp-multi-stapl-check
branch
from
1410138 to
7c287de
Compare
|
retest this please |
gasbytes
force-pushed
the
tls-1-2-ocsp-multi-stapl-check
branch
from
7c287de to
733d292
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review-securityOverall recommendation: APPROVE
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
One or more scans did not complete — the results below are partial.
Posted findings
- [Low] [review-security] New test's method array can be an empty initializer under TLS1.3-only + ocspstapling2 builds —
tests/api/test_ocsp.c:1270-1281 - [Info] [review-security] Fix rejects all multi-SingleResponse BasicOCSPResponses (conservative vs. RFC 6960) —
src/internal.c:17956-17959
Review generated by Skoll
gasbytes
force-pushed
the
tls-1-2-ocsp-multi-stapl-check
branch
from
733d292 to
00a6af5
Compare
|
Jenkins retest this please (timeout) |
| byte* root_resp = NULL; | ||
| int ret = WOLFSSL_OCSP_STATUS_CB_ALERT_FATAL; | ||
| (void)ioCtx; | ||
| leaf_resp = (byte*)XMALLOC(sizeof(resp_server1_revoked_good_first), NULL, 0); |
There was a problem hiding this comment.
Why is the XMALLOC using 0 in these for the DYNAMIC_TYPE?
There was a problem hiding this comment.
Right, it should be *_TMP_BUFFER, fixing it right now.
Thanks.
…leResponses So a revoked certificate's non-first single can no longer be accepted
gasbytes
force-pushed
the
tls-1-2-ocsp-multi-stapl-check
branch
from
00a6af5 to
d3527d5
Compare
Description
Reject status_request_v2 ocsp-multi staples that bundle multiple SingleResponses
So a revoked certificate's non-first single can no longer be accepted
Testing
Added regression test + generated blob with the certificate chain to reproduce the behaviour.
config used: ./configure --enable-all
Checklist