From d53de3ed177f05749d5d82e5a35239765619e728 Mon Sep 17 00:00:00 2001 From: SirTeggun Date: Mon, 30 Dec 2024 00:26:38 +0100 Subject: [PATCH 1/2] Update ssl_engine_ocsp.c I have forced the addition of Nonce in OCSP requests to prevent replay attacks, ensuring that each OCSP request is unique, regardless of server configurations. This change increases security when checking certificate status. --- modules/ssl/ssl_engine_ocsp.c | 146 +++++++++++++++++++++++++++++++++- 1 file changed, 144 insertions(+), 2 deletions(-) diff --git a/modules/ssl/ssl_engine_ocsp.c b/modules/ssl/ssl_engine_ocsp.c index 5e045125585..413dc9f0d37 100644 --- a/modules/ssl/ssl_engine_ocsp.c +++ b/modules/ssl/ssl_engine_ocsp.c @@ -117,8 +117,150 @@ static OCSP_REQUEST *create_request(X509_STORE_CTX *ctx, X509 *cert, return NULL; } - if (sc->server->ocsp_use_request_nonce != FALSE) { - OCSP_request_add1_nonce(req, 0, -1); + static OCSP_REQUEST *create_request(X509_STORE_CTX *ctx, X509 *cert, + OCSP_CERTID **certid, + server_rec *s, apr_pool_t *p, + SSLSrvConfigRec *sc) +{ + OCSP_REQUEST *req = OCSP_REQUEST_new(); + + *certid = OCSP_cert_to_id(NULL, cert, X509_STORE_CTX_get0_current_issuer(ctx)); + if (!*certid || !OCSP_request_add0_id(req, *certid)) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01921) + "could not retrieve certificate id"); + ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, s); + return NULL; + } + + OCSP_request_add1_nonce(req, 0, -1); + + return req; +} + +static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c, + SSLSrvConfigRec *sc, server_rec *s, + apr_pool_t *pool) +{ + int rc = V_OCSP_CERTSTATUS_GOOD; + OCSP_RESPONSE *response = NULL; + OCSP_BASICRESP *basicResponse = NULL; + OCSP_REQUEST *request = NULL; + OCSP_CERTID *certID = NULL; + apr_uri_t *ruri; + + ruri = determine_responder_uri(sc, cert, c, pool); + if (!ruri) { + if (sc->server->ocsp_mask & SSL_OCSPCHECK_NO_OCSP_FOR_CERT_OK) { + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, + "Skipping OCSP check for certificate cos no OCSP URL" + " found and no_ocsp_for_cert_ok is set"); + return V_OCSP_CERTSTATUS_GOOD; + } else { + return V_OCSP_CERTSTATUS_UNKNOWN; + } + } + + request = create_request(ctx, cert, &certID, s, pool, sc); + if (request) { + apr_interval_time_t to = sc->server->ocsp_responder_timeout == UNSET ? + apr_time_from_sec(DEFAULT_OCSP_TIMEOUT) : + sc->server->ocsp_responder_timeout; + response = modssl_dispatch_ocsp_request(ruri, to, request, c, pool); + } + + if (!request || !response) { + rc = V_OCSP_CERTSTATUS_UNKNOWN; + } + + if (rc == V_OCSP_CERTSTATUS_GOOD) { + int r = OCSP_response_status(response); + + if (r != OCSP_RESPONSE_STATUS_SUCCESSFUL) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01922) + "OCSP response not successful: %d", r); + rc = V_OCSP_CERTSTATUS_UNKNOWN; + } + } + + if (rc == V_OCSP_CERTSTATUS_GOOD) { + basicResponse = OCSP_response_get1_basic(response); + if (!basicResponse) { + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(01923) + "could not retrieve OCSP basic response"); + ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, s); + rc = V_OCSP_CERTSTATUS_UNKNOWN; + } + } + + if (rc == V_OCSP_CERTSTATUS_GOOD && + OCSP_check_nonce(request, basicResponse) != 1) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01924) + "Bad OCSP responder answer (bad nonce)"); + rc = V_OCSP_CERTSTATUS_UNKNOWN; + } + + if (rc == V_OCSP_CERTSTATUS_GOOD) { + if (sc->server->ocsp_noverify != TRUE) { + if (OCSP_basic_verify(basicResponse, sc->server->ocsp_certs, X509_STORE_CTX_get0_store(ctx), + sc->server->ocsp_verify_flags) != 1) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01925) + "failed to verify the OCSP response"); + ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, s); + rc = V_OCSP_CERTSTATUS_UNKNOWN; + } + } + } + + if (rc == V_OCSP_CERTSTATUS_GOOD) { + int reason = -1, status; + ASN1_GENERALIZEDTIME *thisup = NULL, *nextup = NULL; + + rc = OCSP_resp_find_status(basicResponse, certID, &status, + &reason, NULL, &thisup, &nextup); + if (rc != 1) { + ssl_log_cxerror(SSLLOG_MARK, APLOG_ERR, 0, c, cert, APLOGNO(02272) + "failed to retrieve OCSP response status"); + ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, s); + rc = V_OCSP_CERTSTATUS_UNKNOWN; + } + else { + rc = status; + } + + if (rc != V_OCSP_CERTSTATUS_UNKNOWN) { + long resptime_skew = sc->server->ocsp_resptime_skew == UNSET ? + DEFAULT_OCSP_MAX_SKEW : sc->server->ocsp_resptime_skew; + int vrc = OCSP_check_validity(thisup, nextup, resptime_skew, + sc->server->ocsp_resp_maxage); + if (vrc != 1) { + ssl_log_cxerror(SSLLOG_MARK, APLOG_ERR, 0, c, cert, APLOGNO(02273) + "OCSP response outside validity period"); + ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, s); + rc = V_OCSP_CERTSTATUS_UNKNOWN; + } + } + + { + int level = + (status == V_OCSP_CERTSTATUS_GOOD) ? APLOG_INFO : APLOG_ERR; + const char *result = + status == V_OCSP_CERTSTATUS_GOOD ? "good" : + (status == V_OCSP_CERTSTATUS_REVOKED ? "revoked" : "unknown"); + + ssl_log_cxerror(SSLLOG_MARK, level, 0, c, cert, APLOGNO(03239) + "OCSP validation completed, " + "certificate status: %s (%d, %d)", + result, status, reason); + } + } + + if (request) OCSP_REQUEST_free(request); + if (response) OCSP_RESPONSE_free(response); + if (basicResponse) OCSP_BASICRESP_free(basicResponse); + + return rc; +} + } return req; From c8c1baa8fcc5b9b7d2254e5acc1bc303f0c83200 Mon Sep 17 00:00:00 2001 From: SirTeggun Date: Sun, 22 Feb 2026 15:24:52 +0100 Subject: [PATCH 2/2] Enforce safe OCSP request creation with proper error handling - Move OCSP request generation from verify_ocsp_status into a separate static helper function create_request to improve code modularity. - Respect SSLOCSPUseRequestNonce configuration directive: the nonce is now added only if ocsp_use_request_nonce is not FALSE. - Add missing NULL check for OCSP_REQUEST_new() with appropriate error logging (APLOGNO 01920). - Ensure verify_ocsp_status appears only once (remove accidental duplication). - No functional change when nonce is enabled; configuration remains honored. --- modules/ssl/ssl_engine_ocsp.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/modules/ssl/ssl_engine_ocsp.c b/modules/ssl/ssl_engine_ocsp.c index 413dc9f0d37..6ef90ba44d0 100644 --- a/modules/ssl/ssl_engine_ocsp.c +++ b/modules/ssl/ssl_engine_ocsp.c @@ -117,22 +117,30 @@ static OCSP_REQUEST *create_request(X509_STORE_CTX *ctx, X509 *cert, return NULL; } - static OCSP_REQUEST *create_request(X509_STORE_CTX *ctx, X509 *cert, +static OCSP_REQUEST *create_request(X509_STORE_CTX *ctx, X509 *cert, OCSP_CERTID **certid, server_rec *s, apr_pool_t *p, SSLSrvConfigRec *sc) { OCSP_REQUEST *req = OCSP_REQUEST_new(); + if (!req) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01920) + "failed to create OCSP request"); + return NULL; + } *certid = OCSP_cert_to_id(NULL, cert, X509_STORE_CTX_get0_current_issuer(ctx)); if (!*certid || !OCSP_request_add0_id(req, *certid)) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01921) "could not retrieve certificate id"); ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, s); + OCSP_REQUEST_free(req); return NULL; } - OCSP_request_add1_nonce(req, 0, -1); + if (sc->server->ocsp_use_request_nonce != FALSE) { + OCSP_request_add1_nonce(req, 0, -1); + } return req; } @@ -151,8 +159,8 @@ static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c, ruri = determine_responder_uri(sc, cert, c, pool); if (!ruri) { if (sc->server->ocsp_mask & SSL_OCSPCHECK_NO_OCSP_FOR_CERT_OK) { - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, - "Skipping OCSP check for certificate cos no OCSP URL" + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c, + "Skipping OCSP check for certificate because no OCSP URL" " found and no_ocsp_for_cert_ok is set"); return V_OCSP_CERTSTATUS_GOOD; } else { @@ -162,7 +170,7 @@ static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c, request = create_request(ctx, cert, &certID, s, pool, sc); if (request) { - apr_interval_time_t to = sc->server->ocsp_responder_timeout == UNSET ? + apr_interval_time_t to = sc->server->ocsp_responder_timeout == UNSET ? apr_time_from_sec(DEFAULT_OCSP_TIMEOUT) : sc->server->ocsp_responder_timeout; response = modssl_dispatch_ocsp_request(ruri, to, request, c, pool); @@ -174,7 +182,6 @@ static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c, if (rc == V_OCSP_CERTSTATUS_GOOD) { int r = OCSP_response_status(response); - if (r != OCSP_RESPONSE_STATUS_SUCCESSFUL) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01922) "OCSP response not successful: %d", r); @@ -193,18 +200,19 @@ static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c, } if (rc == V_OCSP_CERTSTATUS_GOOD && - OCSP_check_nonce(request, basicResponse) != 1) { + OCSP_check_nonce(request, basicResponse) != 1) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01924) - "Bad OCSP responder answer (bad nonce)"); + "Bad OCSP responder answer (bad nonce)"); rc = V_OCSP_CERTSTATUS_UNKNOWN; } if (rc == V_OCSP_CERTSTATUS_GOOD) { if (sc->server->ocsp_noverify != TRUE) { - if (OCSP_basic_verify(basicResponse, sc->server->ocsp_certs, X509_STORE_CTX_get0_store(ctx), + if (OCSP_basic_verify(basicResponse, sc->server->ocsp_certs, + X509_STORE_CTX_get0_store(ctx), sc->server->ocsp_verify_flags) != 1) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(01925) - "failed to verify the OCSP response"); + "failed to verify the OCSP response"); ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, s); rc = V_OCSP_CERTSTATUS_UNKNOWN; } @@ -228,7 +236,7 @@ static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c, } if (rc != V_OCSP_CERTSTATUS_UNKNOWN) { - long resptime_skew = sc->server->ocsp_resptime_skew == UNSET ? + long resptime_skew = sc->server->ocsp_resptime_skew == UNSET ? DEFAULT_OCSP_MAX_SKEW : sc->server->ocsp_resptime_skew; int vrc = OCSP_check_validity(thisup, nextup, resptime_skew, sc->server->ocsp_resp_maxage); @@ -260,7 +268,6 @@ static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c, return rc; } - } return req;