Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ssl_engine_ocsp.c #501

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Update ssl_engine_ocsp.c #501

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 144 additions & 2 deletions modules/ssl/ssl_engine_ocsp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ignore the opt-out config of SSLOCSPUseRequestNonce?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The decision to enforce the inclusion of the nonce in OCSP requests, regardless of the SSLOCSPUseRequestNonce configuration, was made to enhance security. Nonce inclusion helps mitigate replay attacks, where a malicious actor might reuse a previously valid OCSP response to falsely indicate the validity of a revoked certificate.
By ensuring the nonce is always added, the integrity and freshness of the OCSP responses can be guaranteed. While this approach overrides the opt-out feature, it prioritizes preventing potential vulnerabilities.


return req;
}

static int verify_ocsp_status(X509 *cert, X509_STORE_CTX *ctx, conn_rec *c,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks duplicated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The create_request function is not duplicated. Returns req which is used later in verify_ocsp_status. The separation of the two functions is aimed at maintaining a clear division of responsibilities: create_request is responsible for generating the OCSP request, while verify_ocsp_status is responsible for verifying its status.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am referring to verify_ocsp_status(). The commit seems to add a second copy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the code again and found no errors. Tell me if you don't understand something and I'll try to explain better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @covener says it looks like you have inserted a second copy of verify_ocsp_status or the patch you pushed is corrupted or something.

sh-5.2$ curl -s 'https://patch-diff.githubusercontent.com/raw/apache/httpd/pull/501.patch'  | patch -p1
patching file modules/ssl/ssl_engine_ocsp.c
sh-5.2$ make -sj8 -C modules/ssl/ mod_ssl.a
ssl_engine_ocsp.c: In function 'create_request':
ssl_engine_ocsp.c:120:26: error: invalid storage class for function 'create_request'
  120 |     static OCSP_REQUEST *create_request(X509_STORE_CTX *ctx, X509 *cert,
      |                          ^~~~~~~~~~~~~~
ssl_engine_ocsp.c:120:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  120 |     static OCSP_REQUEST *create_request(X509_STORE_CTX *ctx, X509 *cert,
...

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;
Expand Down