Repository navigation
Fix: ocsp - validate response timestamps in wolfSSL_OCSP_check_validity - #11297
jackctj117 wants to merge 4 commits into
Conversation
|
Can one of the admins verify this patch? |
jackctj117
changed the title
There was a problem hiding this comment.
Pull request overview
This PR tightens OpenSSL-compat OCSP response validation by making wolfSSL_OCSP_check_validity actually enforce thisUpdate/nextUpdate timestamp rules (clock skew and staleness), and adds API tests to guard the behavior.
Changes:
- Reworked
wolfSSL_OCSP_check_validityinsrc/ocsp.cto validatethisUpdatepresence/format and enforce skew/staleness, while treating a zero-lengthnextUpdateas “absent”. - Added a focused test suite for
wolfSSL_OCSP_check_validityedge cases and integrated it into the API test runner. - Exposed the new test via
tests/api/test_ocsp.hand registered it intests/api.c.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ocsp.c | Implements timestamp validation logic for OCSP responses (thisUpdate/nextUpdate). |
| tests/api/test_ocsp.c | Adds coverage for validity checks across future/stale/missing/malformed timestamp scenarios. |
| tests/api/test_ocsp.h | Declares the new OCSP validity test entry point. |
| tests/api.c | Registers the new OCSP validity test in the OCSP test group. |
Suppressed comments (1)
src/ocsp.c:819
- The
nextUpdateexpiration check is also strict (nextUpdatemust be afternow - sec). With the currentwolfSSL_X509_cmp_timesemantics (-1 for <=), this rejects anextUpdateexactly at the edge of the allowed clock skew (nextUpdate == now - sec). Allow the boundary case by comparing againstnow - sec - 1.
cmp = now - sec;
if (wolfSSL_X509_cmp_time(nextupd, &cmp) != 1) {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11297
Scan targets checked: wolfcrypt-rs-bugs, wolfssl-bugs, wolfssl-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
|
|
Retest this please. |
julek-wolfssl
left a comment
julek-wolfssl
left a comment
There was a problem hiding this comment.
The changes look correct but the comments are getting out of hand. Please bring them down to a necessary minimum and don't state the obvious.
philljj
left a comment
philljj
left a comment
There was a problem hiding this comment.
Let's rebase this to be safe
Only check nextUpdate once thisUpdate has passed, drop the redundant thisUpdOk flag and cut the comments down to what is not obvious from the code. Restore test_ocsp_forged_responder_cert_rejected, which the previous commit dropped by mistake. Since dacd8ee the compat getters return NULL for an absent nextUpdate, so treat only NULL as absent and require any non-NULL nextUpdate to be valid, as OpenSSL does.
DecodeSingleResponse set thisDateLen and nextDateLen inside the first "if (ret == 0)" block and read them in the WOLFSSL_OCSP_PARSE_STATUS blocks further down. MSVC cannot follow that flow and reports C4701 (potentially uninitialized) now that OPENSSL_EXTRA builds enable WOLFSSL_OCSP_PARSE_STATUS, which made the Windows Jenkins job UNSTABLE. Initialize both at declaration instead.
This pull request implements comprehensive validation for OCSP response timestamps in the
wolfSSL_OCSP_check_validityfunction and adds a thorough suite of tests to ensure correct behavior, especially regarding clock skew, staleness, and absent fields. The changes also integrate the new test into the test suite.OCSP response timestamp validation improvements:
src/ocsp.c: RewrotewolfSSL_OCSP_check_validityto properly validate thethisUpdateandnextUpdatetimestamps, enforcing clock skew (sec), maximum staleness (maxsec), and handling absent or malformed times as failures, except for absentnextUpdatewhich is now correctly accepted when represented as a zero-length structure.Expanded OCSP validation testing:
tests/api/test_ocsp.c: Added a comprehensive test functiontest_ocsp_check_validitycovering all relevant edge cases, including future, stale, missing, and malformedthisUpdate, expired or inconsistentnextUpdate, and correct handling of absentnextUpdateas a zero-length structure. Also tests integration with the OCSP status getter.tests/api/test_ocsp.h: Declared the new test function for use in the test suite.tests/api.c: Registered the new test case in the OCSP test group, ensuring it runs as part of the standard test suite.