From 10f896f6ac570f482e38641da38b39280843879b Mon Sep 17 00:00:00 2001 From: jackctj117 Date: Fri, 28 Aug 2026 13:37:23 -0600 Subject: [PATCH 1/4] ocsp: validate response timestamps in wolfSSL_OCSP_check_validity --- src/ocsp.c | 73 +++++++++++++++++++++++++++++++++++- wolfssl/wolfcrypt/settings.h | 8 +++- 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/src/ocsp.c b/src/ocsp.c index 2d49b0dcbd2..ae46cfba183 100644 --- a/src/ocsp.c +++ b/src/ocsp.c @@ -772,12 +772,83 @@ const char *wolfSSL_OCSP_cert_status_str(long s) int wolfSSL_OCSP_check_validity(WOLFSSL_ASN1_TIME* thisupd, WOLFSSL_ASN1_TIME* nextupd, long sec, long maxsec) { +#ifndef NO_ASN_TIME + int ret = WOLFSSL_SUCCESS; + int thisUpdOk = 0; + time_t now = wc_Time(0); + time_t cmp; + + /* OpenSSL-compatible OCSP response time validation (see + * OCSP_check_validity): `sec` is the permitted clock skew and `maxsec`, + * when non-negative, bounds the age of thisUpdate - the only staleness + * bound available when nextUpdate is absent, which is exactly the case a + * replayed pre-revocation response exploits. + * + * Unlike OpenSSL, which passes NULL for an absent nextUpdate, wolfSSL's + * compat getters (wolfSSL_OCSP_resp_find_status, + * wolfSSL_OCSP_single_get0_status) return a zero-length WOLFSSL_ASN1_TIME + * when the response omits the optional nextUpdate, so both a NULL and an + * empty nextupd are treated as "absent" below. settings.h enables + * WOLFSSL_OCSP_PARSE_STATUS whenever HAVE_OCSP && OPENSSL_EXTRA, so those + * getters populate the timestamps in every build where this function is + * compiled; a NULL thisupd therefore means the caller supplied no time and + * validation fails closed. + * + * A negative `sec` (a nonsensical clock skew) would invert the + * comparisons below, so clamp it to zero. + * + * wolfSSL_X509_cmp_time(t, &cmp) returns 1 when t is after cmp, -1 when t + * is at/before cmp, and 0 when t is NULL or malformed. */ + if (sec < 0) + sec = 0; + + /* thisUpdate is required, must be valid, and must not be more than `sec` + * 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) { + WOLFSSL_MSG("OCSP thisUpdate missing, malformed or not yet valid"); + ret = WOLFSSL_FAILURE; + } + else { + thisUpdOk = 1; + /* Bound the age only when maxsec is non-negative (matching OpenSSL). */ + if (maxsec >= 0) { + cmp = now - maxsec; + if (wolfSSL_X509_cmp_time(thisupd, &cmp) != 1) { + WOLFSSL_MSG("OCSP thisUpdate is too old"); + ret = WOLFSSL_FAILURE; + } + } + } + + /* nextUpdate is optional; a NULL or zero-length value means absent. When + * present it must be valid and must not be more than `sec` seconds in the + * past (i.e. the response has expired). */ + if ((nextupd != NULL) && (nextupd->length > 0)) { + cmp = now - sec; + if (wolfSSL_X509_cmp_time(nextupd, &cmp) != 1) { + WOLFSSL_MSG("OCSP nextUpdate has passed or is malformed"); + ret = WOLFSSL_FAILURE; + } + /* nextUpdate must not precede thisUpdate (only meaningful when + * thisUpdate was itself a usable time). */ + else if (thisUpdOk && + (wolfSSL_ASN1_TIME_compare(nextupd, thisupd) < 0)) { + WOLFSSL_MSG("OCSP nextUpdate precedes thisUpdate"); + ret = WOLFSSL_FAILURE; + } + } + + return ret; +#else (void)thisupd; (void)nextupd; (void)sec; (void)maxsec; - /* Dates validated in DecodeSingleResponse. */ + /* Without ASN time support the timestamps cannot be validated. */ return WOLFSSL_SUCCESS; +#endif /* !NO_ASN_TIME */ } void wolfSSL_OCSP_CERTID_free(WOLFSSL_OCSP_CERTID* certId) diff --git a/wolfssl/wolfcrypt/settings.h b/wolfssl/wolfcrypt/settings.h index 6564f4aa52a..7d021053fb0 100644 --- a/wolfssl/wolfcrypt/settings.h +++ b/wolfssl/wolfcrypt/settings.h @@ -4454,9 +4454,15 @@ #endif #if defined(HAVE_OCSP) && !defined(WOLFCRYPT_ONLY) && \ - (defined(OPENSSL_ALL) || defined(WOLFSSL_NGINX) || \ + (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA) || defined(WOLFSSL_NGINX) || \ defined(WOLFSSL_HAPROXY) || defined(HAVE_LIGHTY) || \ defined(WOLFSSL_APACHE_HTTPD)) + /* The OpenSSL-compat OCSP timestamp getters (wolfSSL_OCSP_resp_find_status, + * wolfSSL_OCSP_single_get0_status) and wolfSSL_OCSP_check_validity are only + * useful when thisUpdate/nextUpdate are parsed out, so enable this for any + * OPENSSL_EXTRA build with OCSP (e.g. --enable-curl), not just the app + * bundles - otherwise those getters return NULL and check_validity cannot + * validate. */ #undef WOLFSSL_OCSP_PARSE_STATUS #define WOLFSSL_OCSP_PARSE_STATUS #endif From bd9afd6bc31003b6ba9f09b3a25d3ec417c2faaf Mon Sep 17 00:00:00 2001 From: jackctj117 Date: Fri, 28 Aug 2026 13:37:23 -0600 Subject: [PATCH 2/4] ocsp: test wolfSSL_OCSP_check_validity timestamp validation (issue 10718) --- tests/api.c | 2 +- tests/api/test_ocsp.c | 183 +++++++++++++++++++++++++++++++++--------- tests/api/test_ocsp.h | 2 +- 3 files changed, 149 insertions(+), 38 deletions(-) diff --git a/tests/api.c b/tests/api.c index 1d60e540cda..98d75a0e3ed 100644 --- a/tests/api.c +++ b/tests/api.c @@ -48023,10 +48023,10 @@ TEST_CASE testCases[] = { TEST_DECL_GROUP("ocsp", test_ocsp_ctx_stapling_v2_nonce_per_connection), TEST_DECL_GROUP("ocsp", test_ocsp_ancestor_responder_rejected), - TEST_DECL_GROUP("ocsp", test_ocsp_forged_responder_cert_rejected), TEST_DECL_GROUP("ocsp", test_ocsp_responder_keyhash_binding), TEST_DECL_GROUP("ocsp", test_ocsp_response_parsing), TEST_DECL_GROUP("ocsp", test_ocsp_certid_enc_dec), + TEST_DECL_GROUP("ocsp", test_ocsp_check_validity), TEST_DECL_GROUP("ocsp", test_ocsp_certid_dup), TEST_DECL_GROUP("ocsp", test_ocsp_resp_find_status_serial_prefix), TEST_DECL_GROUP("ocsp", test_ocsp_resp_times), diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index 8d61ef50e9f..885e704aafa 100644 --- a/tests/api/test_ocsp.c +++ b/tests/api/test_ocsp.c @@ -333,42 +333,6 @@ int test_ocsp_ancestor_responder_rejected(void) } #endif -#if defined(HAVE_OCSP) && !defined(NO_SHA) && !defined(NO_RSA) -/* The responder certificate embedded in an OCSP response must have its own - * signature verified against the CA it claims issued it (RFC 6960 4.2.2.2: - * the delegation certificate and the certificate being checked must be signed - * by the same key). resp_forged_responder_cert carries a responder - * certificate that names the legitimate root CA as its issuer and repeats - * that CA's key identifier, so the signer lookup resolves to the real root - * CA, but it was signed by the imposter root CA's key. Matching names and key - * identifiers only select a candidate issuer; they do not authenticate one. - * Nothing else about the response is wrong, so accepting it would mean the - * responder certificate's signature was never checked. */ -int test_ocsp_forged_responder_cert_rejected(void) -{ - EXPECT_DECLS; - struct test_conf conf; - - conf.resp = (unsigned char*)resp_forged_responder_cert; - conf.respSz = sizeof(resp_forged_responder_cert); - conf.ca0 = root_ca_cert_pem; - conf.ca0Sz = sizeof(root_ca_cert_pem); - conf.ca1 = NULL; - conf.ca1Sz = 0; - conf.targetCert = intermediate1_ca_cert_pem; - conf.targetCertSz = sizeof(intermediate1_ca_cert_pem); - ExpectIntEQ(test_ocsp_response_with_cm(&conf, OCSP_LOOKUP_FAIL), - TEST_SUCCESS); - - return EXPECT_RESULT(); -} -#else -int test_ocsp_forged_responder_cert_rejected(void) -{ - return TEST_SKIPPED; -} -#endif - #if defined(HAVE_OCSP) && (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA)) && \ !defined(NO_RSA) static int test_ocsp_create_x509store(WOLFSSL_X509_STORE** store, @@ -6937,3 +6901,150 @@ int test_wolfIO_DecodeUrl_host_bounds(void) return TEST_SKIPPED; } #endif /* HAVE_HTTP_CLIENT */ + + +/* wolfSSL_OCSP_check_validity (OpenSSL OCSP_check_validity) must validate the + * response timestamps: reject a thisUpdate that is missing/malformed/in the + * future, reject an expired nextUpdate, and - crucially, since that is what a + * replayed pre-revocation response looks like - enforce maxsec as the maximum + * age of thisUpdate when nextUpdate is absent. `sec` is the permitted clock + * skew. Before the fix the function ignored every argument and always + * succeeded (issue 10718). + * + * wolfSSL's compat getters return a zero-length WOLFSSL_ASN1_TIME (not NULL) + * for an absent nextUpdate, so that case must be treated as "absent" and + * accepted, not rejected. Times are built relative to the current time so the + * test is clock-independent. */ +#if defined(HAVE_OCSP) && (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA)) && \ + !defined(NO_ASN_TIME) && !defined(USER_TIME) && !defined(TIME_OVERRIDES) +int test_ocsp_check_validity(void) +{ + EXPECT_DECLS; + time_t now = wc_Time(0); + WOLFSSL_ASN1_TIME thisUpd; + WOLFSSL_ASN1_TIME nextUpd; + const long DAY = 24L * 60 * 60; + + /* Fresh thisUpdate, no nextUpdate, within maxsec: accepted. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), + WOLFSSL_SUCCESS); + + /* Stale thisUpdate (~1 day old), no nextUpdate, maxsec 300: the response + * is older than allowed, so it must be rejected (the replay case). */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), + WOLFSSL_FAILURE); + + /* thisUpdate far in the future (beyond clock skew): not yet valid. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, DAY)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), + WOLFSSL_FAILURE); + + /* thisUpdate slightly in the future but within `sec` clock skew: accepted. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, 100)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), + WOLFSSL_SUCCESS); + + /* Missing thisUpdate (NULL): rejected. */ + ExpectIntEQ(wolfSSL_OCSP_check_validity(NULL, NULL, 300, 300), + WOLFSSL_FAILURE); + + /* Malformed/zero-length thisUpdate: rejected. */ + XMEMSET(&thisUpd, 0, sizeof(thisUpd)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), + WOLFSSL_FAILURE); + + /* Fresh thisUpdate, expired nextUpdate (~1 day past): rejected. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, -DAY)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), + WOLFSSL_FAILURE); + + /* Fresh thisUpdate, nextUpdate slightly in the past but within `sec` skew: + * accepted (verifies the skew is applied to nextUpdate too). */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -120)); + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, -100)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, DAY), + WOLFSSL_SUCCESS); + + /* Fresh thisUpdate, valid nextUpdate in the future: accepted. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, DAY)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), + WOLFSSL_SUCCESS); + + /* nextUpdate precedes thisUpdate: rejected as inconsistent. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, -120)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), + WOLFSSL_FAILURE); + + /* Absent nextUpdate as the compat getters actually represent it: a + * zero-length WOLFSSL_ASN1_TIME, NOT NULL. Must be treated as absent and + * accepted (regression guard for issue 10718 review: a non-NULL but empty + * nextUpdate must not hard-fail every response that omits the field). */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); + XMEMSET(&nextUpd, 0, sizeof(nextUpd)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, DAY), + WOLFSSL_SUCCESS); + /* ... and staleness is still enforced through that same empty-nextUpdate + * path. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); + XMEMSET(&nextUpd, 0, sizeof(nextUpd)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), + WOLFSSL_FAILURE); + + /* Stale thisUpdate but maxsec < 0 disables the age check: accepted. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, -1), + WOLFSSL_SUCCESS); + + /* Stale thisUpdate within a large maxsec: age bound not exceeded, accepted. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); + ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 2 * DAY), + WOLFSSL_SUCCESS); + +#ifdef WOLFSSL_OCSP_PARSE_STATUS + /* End-to-end through the real getter: a response whose nextUpdate is + * absent leaves CertStatus.nextDateParsed zero-length, and + * wolfSSL_OCSP_single_get0_status hands that back (non-NULL). Feeding the + * getter output straight into check_validity - the canonical OpenSSL + * pattern - must accept a fresh response and reject a stale one. */ + { + WOLFSSL_OCSP_SINGLERESP single; + CertStatus certStatus; + WOLFSSL_ASN1_TIME* gThis = NULL; + WOLFSSL_ASN1_TIME* gNext = NULL; + + XMEMSET(&single, 0, sizeof(single)); + XMEMSET(&certStatus, 0, sizeof(certStatus)); + certStatus.status = CERT_GOOD; + single.status = &certStatus; + + /* Fresh thisUpdate, nextUpdate absent (nextDateParsed left zeroed). */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&certStatus.thisDateParsed, now, 0, + -60)); + ExpectIntEQ(wolfSSL_OCSP_single_get0_status(&single, NULL, NULL, + &gThis, &gNext), CERT_GOOD); + ExpectIntEQ(wolfSSL_OCSP_check_validity(gThis, gNext, 300, DAY), + WOLFSSL_SUCCESS); + + /* Same shape but a stale thisUpdate: rejected. */ + ExpectNotNull(wolfSSL_ASN1_TIME_adj(&certStatus.thisDateParsed, now, 0, + -DAY)); + ExpectIntEQ(wolfSSL_OCSP_single_get0_status(&single, NULL, NULL, + &gThis, &gNext), CERT_GOOD); + ExpectIntEQ(wolfSSL_OCSP_check_validity(gThis, gNext, 300, 300), + WOLFSSL_FAILURE); + } +#endif /* WOLFSSL_OCSP_PARSE_STATUS */ + + return EXPECT_RESULT(); +} +#else +int test_ocsp_check_validity(void) +{ + return TEST_SKIPPED; +} +#endif diff --git a/tests/api/test_ocsp.h b/tests/api/test_ocsp.h index 0c0c6eb2d27..361f0a2043c 100644 --- a/tests/api/test_ocsp.h +++ b/tests/api/test_ocsp.h @@ -23,6 +23,7 @@ #define WOLFSSL_TEST_OCSP_H int test_ocsp_certid_enc_dec(void); +int test_ocsp_check_validity(void); int test_ocsp_certid_dup(void); int test_ocsp_resp_find_status_serial_prefix(void); int test_ocsp_resp_times(void); @@ -70,7 +71,6 @@ int test_dtls12_nonblock_ocsp(void); int test_ocsp_ctx_request_cache(void); int test_ocsp_responder(void); int test_ocsp_ancestor_responder_rejected(void); -int test_ocsp_forged_responder_cert_rejected(void); int test_wolfIO_DecodeUrl_crlf_reject(void); int test_wolfIO_DecodeUrl_host_bounds(void); #endif /* WOLFSSL_TEST_OCSP_H */ From 81b780f8655843aff91b13e607a3e5a87101a5ab Mon Sep 17 00:00:00 2001 From: jackctj117 Date: Tue, 13 Oct 2026 16:29:37 -0600 Subject: [PATCH 3/4] ocsp: address review for wolfSSL_OCSP_check_validity 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 dacd8ee72 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. --- src/ocsp.c | 55 ++++---------------- tests/api.c | 1 + tests/api/test_ocsp.c | 98 ++++++++++++++++++++---------------- tests/api/test_ocsp.h | 1 + wolfssl/wolfcrypt/settings.h | 14 ++---- 5 files changed, 72 insertions(+), 97 deletions(-) diff --git a/src/ocsp.c b/src/ocsp.c index ae46cfba183..c5b1f0ed102 100644 --- a/src/ocsp.c +++ b/src/ocsp.c @@ -774,67 +774,35 @@ int wolfSSL_OCSP_check_validity(WOLFSSL_ASN1_TIME* thisupd, { #ifndef NO_ASN_TIME int ret = WOLFSSL_SUCCESS; - int thisUpdOk = 0; time_t now = wc_Time(0); time_t cmp; - /* OpenSSL-compatible OCSP response time validation (see - * OCSP_check_validity): `sec` is the permitted clock skew and `maxsec`, - * when non-negative, bounds the age of thisUpdate - the only staleness - * bound available when nextUpdate is absent, which is exactly the case a - * replayed pre-revocation response exploits. - * - * Unlike OpenSSL, which passes NULL for an absent nextUpdate, wolfSSL's - * compat getters (wolfSSL_OCSP_resp_find_status, - * wolfSSL_OCSP_single_get0_status) return a zero-length WOLFSSL_ASN1_TIME - * when the response omits the optional nextUpdate, so both a NULL and an - * empty nextupd are treated as "absent" below. settings.h enables - * WOLFSSL_OCSP_PARSE_STATUS whenever HAVE_OCSP && OPENSSL_EXTRA, so those - * getters populate the timestamps in every build where this function is - * compiled; a NULL thisupd therefore means the caller supplied no time and - * validation fails closed. - * - * A negative `sec` (a nonsensical clock skew) would invert the - * comparisons below, so clamp it to zero. - * - * wolfSSL_X509_cmp_time(t, &cmp) returns 1 when t is after cmp, -1 when t - * is at/before cmp, and 0 when t is NULL or malformed. */ + /* Same semantics as OpenSSL's OCSP_check_validity: sec is the allowed + * clock skew and maxsec, when non-negative, the maximum age of thisUpdate. + * wolfSSL_X509_cmp_time() returns 0 for a NULL or malformed time. */ if (sec < 0) sec = 0; - /* thisUpdate is required, must be valid, and must not be more than `sec` - * 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) { - WOLFSSL_MSG("OCSP thisUpdate missing, malformed or not yet valid"); + WOLFSSL_MSG("OCSP thisUpdate missing, malformed or in the future"); ret = WOLFSSL_FAILURE; } - else { - thisUpdOk = 1; - /* Bound the age only when maxsec is non-negative (matching OpenSSL). */ - if (maxsec >= 0) { - cmp = now - maxsec; - if (wolfSSL_X509_cmp_time(thisupd, &cmp) != 1) { - WOLFSSL_MSG("OCSP thisUpdate is too old"); - ret = WOLFSSL_FAILURE; - } + else if (maxsec >= 0) { + cmp = now - maxsec; + if (wolfSSL_X509_cmp_time(thisupd, &cmp) != 1) { + WOLFSSL_MSG("OCSP thisUpdate is too old"); + ret = WOLFSSL_FAILURE; } } - /* nextUpdate is optional; a NULL or zero-length value means absent. When - * present it must be valid and must not be more than `sec` seconds in the - * past (i.e. the response has expired). */ - if ((nextupd != NULL) && (nextupd->length > 0)) { + if ((ret == WOLFSSL_SUCCESS) && (nextupd != NULL)) { cmp = now - sec; if (wolfSSL_X509_cmp_time(nextupd, &cmp) != 1) { WOLFSSL_MSG("OCSP nextUpdate has passed or is malformed"); ret = WOLFSSL_FAILURE; } - /* nextUpdate must not precede thisUpdate (only meaningful when - * thisUpdate was itself a usable time). */ - else if (thisUpdOk && - (wolfSSL_ASN1_TIME_compare(nextupd, thisupd) < 0)) { + else if (wolfSSL_ASN1_TIME_compare(nextupd, thisupd) < 0) { WOLFSSL_MSG("OCSP nextUpdate precedes thisUpdate"); ret = WOLFSSL_FAILURE; } @@ -846,7 +814,6 @@ int wolfSSL_OCSP_check_validity(WOLFSSL_ASN1_TIME* thisupd, (void)nextupd; (void)sec; (void)maxsec; - /* Without ASN time support the timestamps cannot be validated. */ return WOLFSSL_SUCCESS; #endif /* !NO_ASN_TIME */ } diff --git a/tests/api.c b/tests/api.c index 98d75a0e3ed..0e2747575fa 100644 --- a/tests/api.c +++ b/tests/api.c @@ -48023,6 +48023,7 @@ TEST_CASE testCases[] = { TEST_DECL_GROUP("ocsp", test_ocsp_ctx_stapling_v2_nonce_per_connection), TEST_DECL_GROUP("ocsp", test_ocsp_ancestor_responder_rejected), + TEST_DECL_GROUP("ocsp", test_ocsp_forged_responder_cert_rejected), TEST_DECL_GROUP("ocsp", test_ocsp_responder_keyhash_binding), TEST_DECL_GROUP("ocsp", test_ocsp_response_parsing), TEST_DECL_GROUP("ocsp", test_ocsp_certid_enc_dec), diff --git a/tests/api/test_ocsp.c b/tests/api/test_ocsp.c index 885e704aafa..8afa237e079 100644 --- a/tests/api/test_ocsp.c +++ b/tests/api/test_ocsp.c @@ -333,6 +333,42 @@ int test_ocsp_ancestor_responder_rejected(void) } #endif +#if defined(HAVE_OCSP) && !defined(NO_SHA) && !defined(NO_RSA) +/* The responder certificate embedded in an OCSP response must have its own + * signature verified against the CA it claims issued it (RFC 6960 4.2.2.2: + * the delegation certificate and the certificate being checked must be signed + * by the same key). resp_forged_responder_cert carries a responder + * certificate that names the legitimate root CA as its issuer and repeats + * that CA's key identifier, so the signer lookup resolves to the real root + * CA, but it was signed by the imposter root CA's key. Matching names and key + * identifiers only select a candidate issuer; they do not authenticate one. + * Nothing else about the response is wrong, so accepting it would mean the + * responder certificate's signature was never checked. */ +int test_ocsp_forged_responder_cert_rejected(void) +{ + EXPECT_DECLS; + struct test_conf conf; + + conf.resp = (unsigned char*)resp_forged_responder_cert; + conf.respSz = sizeof(resp_forged_responder_cert); + conf.ca0 = root_ca_cert_pem; + conf.ca0Sz = sizeof(root_ca_cert_pem); + conf.ca1 = NULL; + conf.ca1Sz = 0; + conf.targetCert = intermediate1_ca_cert_pem; + conf.targetCertSz = sizeof(intermediate1_ca_cert_pem); + ExpectIntEQ(test_ocsp_response_with_cm(&conf, OCSP_LOOKUP_FAIL), + TEST_SUCCESS); + + return EXPECT_RESULT(); +} +#else +int test_ocsp_forged_responder_cert_rejected(void) +{ + return TEST_SKIPPED; +} +#endif + #if defined(HAVE_OCSP) && (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA)) && \ !defined(NO_RSA) static int test_ocsp_create_x509store(WOLFSSL_X509_STORE** store, @@ -6902,19 +6938,9 @@ int test_wolfIO_DecodeUrl_host_bounds(void) } #endif /* HAVE_HTTP_CLIENT */ - -/* wolfSSL_OCSP_check_validity (OpenSSL OCSP_check_validity) must validate the - * response timestamps: reject a thisUpdate that is missing/malformed/in the - * future, reject an expired nextUpdate, and - crucially, since that is what a - * replayed pre-revocation response looks like - enforce maxsec as the maximum - * age of thisUpdate when nextUpdate is absent. `sec` is the permitted clock - * skew. Before the fix the function ignored every argument and always - * succeeded (issue 10718). - * - * wolfSSL's compat getters return a zero-length WOLFSSL_ASN1_TIME (not NULL) - * for an absent nextUpdate, so that case must be treated as "absent" and - * accepted, not rejected. Times are built relative to the current time so the - * test is clock-independent. */ +/* thisUpdate must be present, no more than sec seconds in the future and no + * older than maxsec; nextUpdate is optional but when present must be valid + * and not have passed. Issue 10718. */ #if defined(HAVE_OCSP) && (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA)) && \ !defined(NO_ASN_TIME) && !defined(USER_TIME) && !defined(TIME_OVERRIDES) int test_ocsp_check_validity(void) @@ -6925,92 +6951,78 @@ int test_ocsp_check_validity(void) WOLFSSL_ASN1_TIME nextUpd; const long DAY = 24L * 60 * 60; - /* Fresh thisUpdate, no nextUpdate, within maxsec: accepted. */ + /* Fresh thisUpdate, no nextUpdate. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), WOLFSSL_SUCCESS); - /* Stale thisUpdate (~1 day old), no nextUpdate, maxsec 300: the response - * is older than allowed, so it must be rejected (the replay case). */ + /* thisUpdate older than maxsec (replayed response). */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), WOLFSSL_FAILURE); - /* thisUpdate far in the future (beyond clock skew): not yet valid. */ + /* thisUpdate in the future beyond the skew. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, DAY)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), WOLFSSL_FAILURE); - /* thisUpdate slightly in the future but within `sec` clock skew: accepted. */ + /* thisUpdate in the future within the skew. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, 100)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), WOLFSSL_SUCCESS); - /* Missing thisUpdate (NULL): rejected. */ + /* NULL thisUpdate. */ ExpectIntEQ(wolfSSL_OCSP_check_validity(NULL, NULL, 300, 300), WOLFSSL_FAILURE); - /* Malformed/zero-length thisUpdate: rejected. */ + /* Zero-length thisUpdate. */ XMEMSET(&thisUpd, 0, sizeof(thisUpd)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 300), WOLFSSL_FAILURE); - /* Fresh thisUpdate, expired nextUpdate (~1 day past): rejected. */ + /* Expired nextUpdate. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, -DAY)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), WOLFSSL_FAILURE); - /* Fresh thisUpdate, nextUpdate slightly in the past but within `sec` skew: - * accepted (verifies the skew is applied to nextUpdate too). */ + /* nextUpdate in the past within the skew. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -120)); ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, -100)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, DAY), WOLFSSL_SUCCESS); - /* Fresh thisUpdate, valid nextUpdate in the future: accepted. */ + /* nextUpdate in the future. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, DAY)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), WOLFSSL_SUCCESS); - /* nextUpdate precedes thisUpdate: rejected as inconsistent. */ + /* nextUpdate before thisUpdate. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); ExpectNotNull(wolfSSL_ASN1_TIME_adj(&nextUpd, now, 0, -120)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), WOLFSSL_FAILURE); - /* Absent nextUpdate as the compat getters actually represent it: a - * zero-length WOLFSSL_ASN1_TIME, NOT NULL. Must be treated as absent and - * accepted (regression guard for issue 10718 review: a non-NULL but empty - * nextUpdate must not hard-fail every response that omits the field). */ + /* Zero-length nextUpdate. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -60)); XMEMSET(&nextUpd, 0, sizeof(nextUpd)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, DAY), - WOLFSSL_SUCCESS); - /* ... and staleness is still enforced through that same empty-nextUpdate - * path. */ - ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); - XMEMSET(&nextUpd, 0, sizeof(nextUpd)); - ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, &nextUpd, 300, 300), WOLFSSL_FAILURE); - /* Stale thisUpdate but maxsec < 0 disables the age check: accepted. */ + /* Negative maxsec disables the age check. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, -1), WOLFSSL_SUCCESS); - /* Stale thisUpdate within a large maxsec: age bound not exceeded, accepted. */ + /* thisUpdate within a larger maxsec. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&thisUpd, now, 0, -DAY)); ExpectIntEQ(wolfSSL_OCSP_check_validity(&thisUpd, NULL, 300, 2 * DAY), WOLFSSL_SUCCESS); #ifdef WOLFSSL_OCSP_PARSE_STATUS - /* End-to-end through the real getter: a response whose nextUpdate is - * absent leaves CertStatus.nextDateParsed zero-length, and - * wolfSSL_OCSP_single_get0_status hands that back (non-NULL). Feeding the - * getter output straight into check_validity - the canonical OpenSSL - * pattern - must accept a fresh response and reject a stale one. */ + /* Through wolfSSL_OCSP_single_get0_status, which returns NULL for an + * absent nextUpdate. */ { WOLFSSL_OCSP_SINGLERESP single; CertStatus certStatus; @@ -7022,7 +7034,6 @@ int test_ocsp_check_validity(void) certStatus.status = CERT_GOOD; single.status = &certStatus; - /* Fresh thisUpdate, nextUpdate absent (nextDateParsed left zeroed). */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&certStatus.thisDateParsed, now, 0, -60)); ExpectIntEQ(wolfSSL_OCSP_single_get0_status(&single, NULL, NULL, @@ -7030,7 +7041,6 @@ int test_ocsp_check_validity(void) ExpectIntEQ(wolfSSL_OCSP_check_validity(gThis, gNext, 300, DAY), WOLFSSL_SUCCESS); - /* Same shape but a stale thisUpdate: rejected. */ ExpectNotNull(wolfSSL_ASN1_TIME_adj(&certStatus.thisDateParsed, now, 0, -DAY)); ExpectIntEQ(wolfSSL_OCSP_single_get0_status(&single, NULL, NULL, diff --git a/tests/api/test_ocsp.h b/tests/api/test_ocsp.h index 361f0a2043c..618b8f58e2c 100644 --- a/tests/api/test_ocsp.h +++ b/tests/api/test_ocsp.h @@ -71,6 +71,7 @@ int test_dtls12_nonblock_ocsp(void); int test_ocsp_ctx_request_cache(void); int test_ocsp_responder(void); int test_ocsp_ancestor_responder_rejected(void); +int test_ocsp_forged_responder_cert_rejected(void); int test_wolfIO_DecodeUrl_crlf_reject(void); int test_wolfIO_DecodeUrl_host_bounds(void); #endif /* WOLFSSL_TEST_OCSP_H */ diff --git a/wolfssl/wolfcrypt/settings.h b/wolfssl/wolfcrypt/settings.h index 7d021053fb0..e6ca017db5a 100644 --- a/wolfssl/wolfcrypt/settings.h +++ b/wolfssl/wolfcrypt/settings.h @@ -4454,15 +4454,11 @@ #endif #if defined(HAVE_OCSP) && !defined(WOLFCRYPT_ONLY) && \ - (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA) || defined(WOLFSSL_NGINX) || \ - defined(WOLFSSL_HAPROXY) || defined(HAVE_LIGHTY) || \ - defined(WOLFSSL_APACHE_HTTPD)) - /* The OpenSSL-compat OCSP timestamp getters (wolfSSL_OCSP_resp_find_status, - * wolfSSL_OCSP_single_get0_status) and wolfSSL_OCSP_check_validity are only - * useful when thisUpdate/nextUpdate are parsed out, so enable this for any - * OPENSSL_EXTRA build with OCSP (e.g. --enable-curl), not just the app - * bundles - otherwise those getters return NULL and check_validity cannot - * validate. */ + (defined(OPENSSL_ALL) || defined(OPENSSL_EXTRA) || \ + defined(WOLFSSL_NGINX) || defined(WOLFSSL_HAPROXY) || \ + defined(HAVE_LIGHTY) || defined(WOLFSSL_APACHE_HTTPD)) + /* OPENSSL_EXTRA: the compat getters must return the parsed + * thisUpdate/nextUpdate for wolfSSL_OCSP_check_validity. */ #undef WOLFSSL_OCSP_PARSE_STATUS #define WOLFSSL_OCSP_PARSE_STATUS #endif From c9eaa03fa70b4db5b8bfd7f9fd716b74b98b4006 Mon Sep 17 00:00:00 2001 From: jackctj117 Date: Fri, 6 Nov 2026 11:24:43 -0700 Subject: [PATCH 4/4] asn: initialize OCSP date lengths at declaration 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. --- wolfcrypt/src/asn.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 46090474893..d6f8f2fbca2 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -37349,8 +37349,10 @@ static int DecodeSingleResponse(const byte* source, word32* ioIndex, DECL_ASNGETDATA(dataASN, singleResponseASN_Length); int ret = 0; CertStatus* cs = NULL; - word32 thisDateLen; - word32 nextDateLen; + /* Maximum lengths for the date buffers. Initialized here so MSVC does not + * warn (C4701) about the WOLFSSL_OCSP_PARSE_STATUS uses below. */ + word32 thisDateLen = MAX_DATE_SIZE; + word32 nextDateLen = MAX_DATE_SIZE; word32 certIdSeqIdx; (void)wrapperSz; @@ -37361,10 +37363,6 @@ static int DecodeSingleResponse(const byte* source, word32* ioIndex, if (ret == 0) { cs = single->status; - /* Set maximum lengths for data. */ - thisDateLen = MAX_DATE_SIZE; - nextDateLen = MAX_DATE_SIZE; - /* Set OID type, buffers to hold data and variables to hold size. */ GetASN_Buffer(&dataASN[SINGLERESPONSEASN_IDX_THISUPDATE_GT], cs->thisDate, &thisDateLen);