Fix: ocsp - validate response timestamps in wolfSSL_OCSP_check_validity - #11297
Fix: ocsp - validate response timestamps in wolfSSL_OCSP_check_validity#11297jackctj117 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
Can one of the admins verify this patch? |
Sorry, something went wrong.
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_validity in src/ocsp.c to validate thisUpdate presence/format and enforce skew/staleness, while treating a zero-length nextUpdate as “absent”.
- Added a focused test suite for wolfSSL_OCSP_check_validity edge cases and integrated it into the API test runner.
- Exposed the new test via tests/api/test_ocsp.h and registered it in tests/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. |
src/ocsp.c:819
- The nextUpdate expiration check is also strict (nextUpdate must be after now - sec). With the current wolfSSL_X509_cmp_time semantics (-1 for <=), this rejects a nextUpdate exactly at the edge of the allowed clock skew (nextUpdate == now - sec). Allow the boundary case by comparing against now - 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.
Sorry, something went wrong.
| int ret = WOLFSSL_SUCCESS; | ||
| int thisUpdOk = 0; | ||
| time_t now = wc_Time(0); | ||
| time_t cmp; | ||
|
|
| cmp = now - maxsec; | ||
| if (wolfSSL_X509_cmp_time(thisupd, &cmp) != 1) { |
| /* Without ASN time support the timestamps cannot be validated. */ | ||
| return WOLFSSL_SUCCESS; |
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.
Sorry, something went wrong.
| * seconds in the future. Anything other than "valid and not in the | ||
| * future" (1 == future, 0 == missing/malformed) is a failure. */ | ||
| cmp = now + sec; | ||
| if (wolfSSL_X509_cmp_time(thisupd, &cmp) != -1) { |
There was a problem hiding this comment.
check_validity always fails when WOLFSSL_OCSP_PARSE_STATUS is off · Logic errors
Without WOLFSSL_OCSP_PARSE_STATUS, wolfSSL_OCSP_resp_find_status and wolfSSL_OCSP_single_get0_status set *thisupd/*nextupd to NULL (src/ocsp.c:738, 1709), so the new NULL-thisupd rejection makes this function return WOLFSSL_FAILURE for every response. --enable-curl enables OPENSSL_EXTRA+OCSP but not WOLFSSL_OCSP_PARSE_STATUS, breaking OCSP stapling verification there.
Related known finding #10718 (similar but distinct): Both affect src/ocsp.c's wolfSSL_OCSP_check_validity timestamp decision. #10718 unconditionally accepted stale responses because validity inputs were ignored; this candidate rejects all responses in configurations where accessor timestamp outputs are NULL. The root causes and fixes differ: implementing validity checks fixes #10718, while enabling/parsing timestamp status data fixes this candidate.
Fix: Define WOLFSSL_OCSP_PARSE_STATUS in settings.h whenever HAVE_OCSP && OPENSSL_EXTRA so the compat getters return real timestamps.
Sorry, something went wrong.
| * seconds in the future. Anything other than "valid and not in the | ||
| * future" (1 == future, 0 == missing/malformed) is a failure. */ | ||
| cmp = now + sec; | ||
| if (wolfSSL_X509_cmp_time(thisupd, &cmp) != -1) { |
There was a problem hiding this comment.
check_validity rejects every response when WOLFSSL_OCSP_PARSE_STATUS is undefined · Logic errors
WOLFSSL_OCSP_PARSE_STATUS is auto-defined only for OPENSSL_ALL/NGINX/HAPROXY/LIGHTY/APACHE_HTTPD (settings.h:4292). In a plain --enable-opensslextra --enable-ocsp build the compat getters hardcode *thisupd = NULL (ocsp.c:739, 1710), so this check fails and every OCSP response is rejected.
Related known finding #10718 (similar but distinct): Both affect wolfSSL_OCSP_check_validity and OpenSSL-compatible OCSP timestamp validation, but #10718 unconditionally accepts stale responses because validity inputs are ignored, whereas this finding rejects responses because getters supply NULL timestamps without WOLFSSL_OCSP_PARSE_STATUS. The root causes and required patches are separate.
Fix: Also define WOLFSSL_OCSP_PARSE_STATUS in settings.h for HAVE_OCSP && OPENSSL_EXTRA builds so the getters return real timestamps.
Sorry, something went wrong.
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
This pull request implements comprehensive validation for OCSP response timestamps in the wolfSSL_OCSP_check_validity function 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:
Expanded OCSP validation testing: