This patch is only rewritten patch from bug #31383 to be able to fit Apache 2.2.x version.
Created attachment 19224 [details] OCSP Patch
Created attachment 19386 [details] New version of OCSP patch containg missing file ssl_ocsp.c In previous bug I forgot to add ssl_ocsp.c file.
Created attachment 19390 [details] New version of OCSP patch generalising validation check, and including ssl_ocsp.c file in Visual C++ project This implements the following validation workflow: 1. if (useOCSP) => check OCSP => if ( OCSP validation possible ) return status 2. Other online validation checks in the future (LDAP, etc.) => check new protocol => if ( validation possible ) return status 3. if (crl) => check CRL => If ( CRL validation possible ) return status 4. If ( forceValidation ) return !ok 5. return ok
Created attachment 19391 [details] Corrected a #include
Created attachment 19392 [details] Corrected a #include
Created attachment 19399 [details] Port to 2.2.4
Created attachment 19445 [details] Documentation patch
Created attachment 19446 [details] Validation schema Schema describing the certificates validation mechanism
From review of attachment in comment 6: A couple of things which make this code hard to review: - many code style issues with this code; tabs, many indenting problems, whitespace around if statements, see: http://httpd.apache.org/dev/styleguide.html and be familiar with existing httpd code - don't use C++-style comments - lots of stretches of code have been commented out rather than just deleted. If they aren't needed, delete them. General review: - don't log anything in the ssl_cmd_* functions, this doesn't add much - don't invent macros for logging in ssl_ocsp.c, just use ap_log_* directly - when and where is NO_OCSP supposed to be defined? this needs an autoconf check presumably; call the define MODSSL_something - if it's useful for users to be able configure a proxy make it properly configurable, otherwise remove the debugging code - X509_Int2Str() should be static and have a name outside a namespace owned by OpenSSL. Use of the static result buffer inside is not thread-safe. - use pools not malloc - using pools, and pool cleanups, or just better function structure, should be able to eliminate the excessive use of goto in VerifyOCSP - GetExtensionValue looks scary. Why is this not looking up extensions by NID, can X509_get_ext_d2i not be used here? - also a bit scared about using the toy HTTP/1.0 client in OpenSSL :(
I modified and cleaned up the code as requested; I will upload a new patch. Some questions: 1. I used the connection pool for memory allocation: c->pool from ssl_callback_SSLVerify_Validity(). Is that correct ? I did not use any pool cleanup, as this will be closed at the end of the connection. 2. I originally added the #ifdef NOOCSP in case you want a version that is compiled with this code. Is this really needed ? Can I remove it ? 3. Should I replace the HTTP connection by some calls to some Apache API ? Which API is available ? 4. Probably dependent to the previous question, is there any global setting defining a proxy to call when opening an outgoing HTTP(S) connection ? I could define it for the OCSP call only, but some other code (even external modules) could also need it; this would lead to the same info being defined several time. Should I implement it only for my code ? Or can I assume that the server will always have a direct access to the OCSP server ?
Is it possible to use mod_proxy connection pools ? This would make sense, although the outgoing connection pool should go out from mod_proxy to a separate module in order to be reusable by every module ... This would maybe be the future best track, but it sounds a bit heavy to reimplement all mod_proxy connection pool handling just for the OCSP connection. Can we leave this as a future improvement and currently use the OpenSSL connection (which works well in practice in several very big eGov sites) ?
(In reply to comment #10) > I modified and cleaned up the code as requested; I will upload a new patch. > > 3. Should I replace the HTTP connection by some calls to some Apache API ? Which > API is available ? > > 4. Probably dependent to the previous question, is there any global setting > defining a proxy to call when opening an outgoing HTTP(S) connection ? I could > define it for the OCSP call only, but some other code (even external modules) > could also need it; this would lead to the same info being defined several time. > Should I implement it only for my code ? Or can I assume that the server will > always have a direct access to the OCSP server ? > Probably not well thought out, but how about a sub request to the proxy_handler provided by mod_proxy?
> how about a sub request to the proxy_handler provided by mod_proxy? Is there any documentation on how to do that ? Would it be acceptable to oblige loading mod_proxy for OCSP validation ?
(In reply to comment #13) > > how about a sub request to the proxy_handler provided by mod_proxy? > > Is there any documentation on how to do that ? Not as far as I know. But having a look into mod_include for subrequest inclusion and at mod_rewrite for preparing a request to go to the proxy could be helpful. > Would it be acceptable to oblige loading mod_proxy for OCSP validation ? I think this should be discussed on dev@httpd.apache.org and not here.
Created attachment 19667 [details] Code following recommandations I followed all recommandations, except the connection that is still established by OpenSSL calls - using mod_proxy was too complex. Note that the OpenSSL connection works very well in big production environments.
I've been working on a cleanup of this patch - we need a CLA on file for all the contributors before anything can be committed, since it's a contribution of new code; I'm not sure who out of the following that means: 1) Marc Stern [no CLA] 2) Matthieu Estrade [CLA on file] has worked on this previously 3) Michal Prochazka [no CLA] wrote the initial port to 2.2 attached here. Marc, can you confirm the lineage of the patch most recently attached? http://issues.apache.org/bugzilla/attachment.cgi?id=19667
Joe, I submitted the CLA some time ago. Michal did only port the patch to the new version, no new code was added. Matthieu could provide a CLA if needed (don't know if need for apache.org members). Can you acknowledge that everything is OK. Thanks
(In reply to comment #17) > Joe, > > I submitted the CLA some time ago. Your iCLA has been registered a while ago (May 7th, see http://people.apache.org/~jim/committers.html and search for Marc Stern). So this is fine. > Michal did only port the patch to the new version, no new code was added. Not quite sure if an iCLA is needed in this case. Joe? > Matthieu could provide a CLA if needed (don't know if need for apache.org members). His iCLA is already on file. So this is fine too.
In ssl_ocsp.c, function ap_ocsp_verify_ocsp: ----- start ----- if (rc == SSL_OCSP_OK) { /* Get issuer */ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "Get Issuer"); rc = X509_STORE_CTX_get1_issuer(&issuer, ctx, cert); if (rc != 1) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "Cannot get issuer of '%s'. rc=%d", X509_SUBJ_NAME(cert), rc); rc = SSL_OCSP_ERROR_INTERNAL; } } if (rc == SSL_OCSP_OK) { ----- end ----- This can't work. If there's no issuer rc is set to SSL_OCSP_ERROR_INTERNAL. If there is one, rc stays "1". However, SSL_OCSP_OK is 0, 1 means SSL_OCSP_ERROR_PARSE_URL
Created attachment 20958 [details] Small corrections in error handling, OCSP response logging, and memory leaks corrections (remarks from OpenSSL developers)
Comment on attachment 20958 [details] Small corrections in error handling, OCSP response logging, and memory leaks corrections (remarks from OpenSSL developers) diff -uaEbwNp orig/ssl_ocsp.c ./ssl_ocsp.c --- orig/ssl_ocsp.c 1970-01-01 01:00:00.000000000 +0100 +++ ./ssl_ocsp.c 2007-10-11 16:37:20.866178500 +0200 @@ -0,0 +1,441 @@ +/* Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/* _ _ + * _ __ ___ ___ __| | ___ ___| | mod_ssl + * | '_ ` _ \ / _ \ / _` | / __/ __| | Apache Interface to OpenSSL + * | | | | | | (_) | (_| | \__ \__ \ | + * |_| |_| |_|\___/ \__,_|___|___/___/_| + * |_____| + * ssl_ocsp.c + * The SSL OCSP checking + * + * Developed by Marc Stern, for Approach Belgium / CSC / Belgian Government + * based on code developed by Zetes Pass + * + * This code was added to support the Belgian Electronic Identity Card + * + * The OCSP responder URL is read from the certificate itself + * + */ + /* ``When the only tool you own is a hammer, + every problem begins to resemble a nail.´´ + */ + +#include "mod_ssl.h" +#include "ssl_private.h" +#include "apr_base64.h" + +#define X509_NAME2STR(name_) X509_NAME_oneline(name_, NULL, 0) +#define X509_SUBJ_NAME(cert_) X509_NAME2STR(X509_get_subject_name(cert_)) +#define X509_ISSUER_NAME(cert_) X509_NAME2STR(X509_get_issuer_name(cert_)) + + +static char *ap_ocsp_ASN1_Int2Str(ASN1_INTEGER *data, apr_pool_t *pool) +{ + char *result = (char *)apr_palloc(pool, 100); /* 100 should be enough */ + char *buf = NULL; + BIGNUM *bn = ASN1_INTEGER_to_BN(data, NULL); + + *result = 0; + if (bn && !BN_is_zero(bn)) { + buf = BN_bn2hex(bn); + if (buf) { + strncpy(result, buf, sizeof(result) - 1); + result[sizeof(result) - 1 ] = 0; + } + } + + if (bn) BN_free(bn); + if (buf) OPENSSL_free(buf); + return result; +} + +static char *ap_ocsp_get_ocsp_uri(X509 *cert, apr_pool_t *pool) +{ + int crit, j; + STACK_OF(ACCESS_DESCRIPTION) *values = + (STACK_OF(ACCESS_DESCRIPTION) *) + X509_get_ext_d2i(cert, NID_info_access, &crit, NULL); + if (! values) return NULL; + + for (j = 0; j < sk_ACCESS_DESCRIPTION_num(values); j++) { + ACCESS_DESCRIPTION *value = sk_ACCESS_DESCRIPTION_value(values, j); + if(OBJ_obj2nid(value->method) == NID_ad_OCSP) { + /* Name found in extension */ + char *result; + + /* Check that it is a URI */ + if (value->location->type != GEN_URI) + continue; + + result = apr_pstrdup(pool, + (char *)value->location->d.uniformResourceIdentifier->data); + AUTHORITY_INFO_ACCESS_free(values); + return result; + } + } + + sk_ACCESS_DESCRIPTION_free(values); + //AUTHORITY_INFO_ACCESS_free(values); + return NULL; +} + + +static BIO *ap_ocsp_connect(const char *host, int port) +{ + BIO *connection = BIO_new_connect((char *)host); + if (!connection) return 0; + + BIO_set_conn_int_port(connection, &port); + if (BIO_do_connect(connection) <= 0) { + /* Not needed - default: BIO_set_close(connection, BIO_CLOSE); */ + BIO_free_all(connection); + return NULL; + } + + return connection; + +} + + +static OCSP_RESPONSE *ap_ocsp_sendreq(const char *ocspHost, const char *ocspPort, const char *ocspPath, OCSP_REQUEST *request, server_rec *s) +{ + BIO *bio = NULL; + OCSP_RESPONSE *response = NULL; + + /* establish a connection to the OCSP responder */ + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Connect to OCSP responder '%s:%s'", ocspHost, ocspPort); + bio = ap_ocsp_connect(ocspHost, atoi(ocspPort)); + if (!bio) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot connect to OCSP responder '%s:%s'", ocspHost, ocspPort); + return NULL; + } + + /* send the request and get a response */ + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + "sending request to OCSP responder"); + response = OCSP_sendreq_bio(bio, (char *)ocspPath, request); + if (!response) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot send request to OCSP responder '%s'", ocspHost); + } + + BIO_free_all(bio); + + return response; +} + + +static int ap_ocsp_verify_ocsp(X509 *cert, X509_STORE_CTX *ctx, server_rec *s, + int *ocspStatus, apr_pool_t *pool) +{ + int rc = SSL_OCSP_OK; + X509 *issuer = NULL; + char *ocspUrl = NULL, *ocspHost = NULL, *ocspPort = NULL, *ocspPath = NULL; + BIO * bio = NULL; + OCSP_RESPONSE * response = NULL; + OCSP_BASICRESP * basicResponse = NULL; + OCSP_REQUEST * request = NULL; + OCSP_CERTID * certID = NULL; + int ssl = 0; + SSLSrvConfigRec *sc = mySrvConfig(s); + char *subj_name = X509_SUBJ_NAME(cert); + char *issuer_name = X509_ISSUER_NAME(cert); + + *ocspStatus = V_OCSP_CERTSTATUS_UNKNOWN; + X509_STORE_CTX_set_error(ctx, X509_V_ERR_APPLICATION_VERIFICATION); + ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, + "OCSP check - cert='%s', issuer='%s'", subj_name, issuer_name); + + + /* First look if we force the responder url*/ + if (sc->server->OCSPForceResponderURL) { + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + "Force the url of responder to: %s", sc->server->OCSPForceResponderURL); + ocspUrl = sc->server->OCSPForceResponderURL; + } + /* if not, look inside the certificate if we have one */ + else { + /* Get OCSP Responder URI (only first one) */ + ocspUrl = ap_ocsp_get_ocsp_uri(cert, pool); + if (ocspUrl) + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + "OCSP responder from certificate: %s", ocspUrl); + } + + if (!ocspUrl && sc->server->OCSPDefaultResponderURL) { + ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, + "No Responder URL in certificate - using default: %s", + sc->server->OCSPDefaultResponderURL); + ocspUrl = sc->server->OCSPDefaultResponderURL; + } + + if (!ocspUrl) { + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s, + "Cannot get OCSP responder URL from '%s' and no default URL Responder", + subj_name); + rc = SSL_OCSP_ERROR_PARSE_URL; + } + + if (rc == SSL_OCSP_OK) { + if (!OCSP_parse_url(ocspUrl, &ocspHost, &ocspPort, &ocspPath, &ssl)) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot parse OCSP responder URL from '%s'", + subj_name); + rc = SSL_OCSP_ERROR_PARSE_URL; + } + } + + + if (rc == SSL_OCSP_OK) { + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "Create new OCSP request"); + request = OCSP_REQUEST_new(); + if (!request) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot create new OCSP request"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if (rc == SSL_OCSP_OK) { + /* Get issuer */ + int r; + /* Enhancement: ctx->chain is already ordered -> extract 2nd ? */ + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "Get Issuer"); + r = X509_STORE_CTX_get1_issuer(&issuer, ctx, cert); + if (r != 1) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot get issuer of '%s'. rc=%d", subj_name, rc); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if (rc == SSL_OCSP_OK) { + certID = OCSP_cert_to_id(0, cert, issuer); + if (!certID || !OCSP_request_add0_id(request, certID)) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s + , "Cannot get certificate id from '%s'", subj_name); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if (rc == SSL_OCSP_OK) { + OCSP_request_add1_nonce(request, 0, -1); + + /* To use a proxy, do the following + - ocspHost = proxyHost; + - ocspPort = proxyPort; + - ocspPath = ocspUrl; + */ + + /* establish a connection to the OCSP responder */ + response = ap_ocsp_sendreq(ocspHost, ocspPort, ocspPath, request, s); + if (!response) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot send request to OCSP responder '%s'", ocspHost); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + + if ( (rc == SSL_OCSP_OK) && (s->loglevel >= APLOG_DEBUG) ) { + /* Log OCSP answer (complete OpenSSL buffer) */ + char *buf = apr_palloc(pool, + apr_base64_encode_len(response->responseBytes->response->length) + 1); + if (buf) { + apr_base64_encode(buf, + (const char*)response->responseBytes->response->data, + response->responseBytes->response->length); + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + "OCSP response (OpenSSL bufer): serial=%s | dn=%s | %s", + ap_ocsp_ASN1_Int2Str(X509_get_serialNumber(cert), pool), + X509_SUBJ_NAME(cert), buf); + } + else { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "Cannot allocate buffer"); + } + } + + if (rc == SSL_OCSP_OK) { + int r; + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + "Analyse OCSP request answer"); + r = OCSP_response_status(response); + if (r != OCSP_RESPONSE_STATUS_SUCCESSFUL) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Bad OCSP responder answer. rc=%d", rc); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if ( (rc == SSL_OCSP_OK) && (s->loglevel >= APLOG_DEBUG) ) { + /* Log OCSP answer (only the "bare" response) */ + int len = i2d_OCSP_RESPONSE(response, NULL); + if (len <= 0) + rc = SSL_OCSP_ERROR_INTERNAL; + else { + unsigned char *buf1, *buf2; + buf1 = buf2 = (unsigned char *)apr_palloc(pool, len); + if (!buf1) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "Out of memory"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + else { + if (i2d_OCSP_RESPONSE(response, &buf1) != len) + rc = SSL_OCSP_ERROR_INTERNAL; + else { + /* contents is in buf2, because buf1 is now pointing + to the end of the structure */ + char h[] = "OCSP response : "; + int len64 = apr_base64_encode_len(len); + char *msg = (char *)apr_palloc(pool, len64 + strlen(h) + 1); + if (msg) { + strcpy(msg, h); + apr_base64_encode(msg + strlen(h), buf2, len); + msg[strlen(h) + len64 + 1] = 0; + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, msg); + } + } + } + } + } + + if (rc == SSL_OCSP_OK) { + basicResponse = OCSP_response_get1_basic(response); + if (!basicResponse) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Bad OCSP responder answer"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if (rc == SSL_OCSP_OK) { + if (OCSP_check_nonce(request, basicResponse) <= 0) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Bad OCSP responder answer (bad nonce)"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if (rc == SSL_OCSP_OK) { + if (OCSP_basic_verify(basicResponse, 0, ctx->ctx, + sc->server->OCSPResponderVerifyFlag) <= 0) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Error verifying OCSP responder answer"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if (rc == SSL_OCSP_OK) { + int ocspReason = -1; + ASN1_GENERALIZEDTIME * ocspProducedAt, *ocspThisUpdate, + *ocspNextUpdate; + rc = OCSP_resp_find_status(basicResponse, certID, ocspStatus, + &ocspReason, &ocspProducedAt, + &ocspThisUpdate, &ocspNextUpdate); + if (rc == 0) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "Bad OCSP status"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "OCSP validation completed: status=%d", *ocspStatus); + rc = SSL_OCSP_OK; + } + + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "Cleanup OCSP code"); + if (issuer) X509_free(issuer); + if (subj_name) OPENSSL_free(subj_name); + if (issuer_name) OPENSSL_free(issuer_name); + if (request) OCSP_REQUEST_free(request); + if (response) OCSP_RESPONSE_free(response); + if (basicResponse) OCSP_BASICRESP_free(basicResponse); + if (ocspHost) OPENSSL_free(ocspHost); + if (ocspPort) OPENSSL_free(ocspPort); + if (ocspPath) OPENSSL_free(ocspPath); + /* certID is just a pointer, nothing to free */ + + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "Ending cleanup OCSP code"); + return rc; +} + +int ssl_cmd_VerifyOCSP(X509_STORE_CTX *ctx, server_rec *s, int *ocspStatus, + apr_pool_t *pool) +{ + int rc = SSL_OCSP_OK, i; + X509 *cert = X509_STORE_CTX_get_current_cert(ctx); + X509_STORE_CTX *ocspCtx = NULL; + X509_STORE *store = NULL; + SSLSrvConfigRec *sc = mySrvConfig(s); + char *subj_name = X509_SUBJ_NAME(cert); + char *issuer_name = X509_ISSUER_NAME(cert); + + ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, + "Validating certificate '%s', issuer: '%s'", + subj_name, issuer_name); + + /* Store certif chain in a store */ + if (!ctx->chain) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "No certificate chain"); + return SSL_OCSP_ERROR_INTERNAL; + } + + /* + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + "certificates chain length: %d", ctx->chain->num); + */ + + store = X509_STORE_new(); + if (!store) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot create a new X509 store"); + return SSL_OCSP_ERROR_INTERNAL; + } + + for (i = 0; i < ctx->chain->num; i++) + if (!X509_STORE_add_cert(store, sk_X509_value(ctx->chain, i))) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot add certificate to X509 store"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + + if (rc == SSL_OCSP_OK) { + ocspCtx = X509_STORE_CTX_new(); + if (!ocspCtx) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot create a new X509 context"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + if (rc == SSL_OCSP_OK) { + if (X509_STORE_CTX_init(ocspCtx, store, cert, 0) != 1) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "Cannot initialise the new X509 context"); + rc = SSL_OCSP_ERROR_INTERNAL; + } + } + + if (rc == SSL_OCSP_OK) + rc = ap_ocsp_verify_ocsp(cert, ocspCtx, s, ocspStatus, pool); + + if (subj_name) OPENSSL_free(subj_name); + if (issuer_name) OPENSSL_free(issuer_name); + if (store) X509_STORE_free(store); + if (ocspCtx) X509_STORE_CTX_free(ocspCtx); + return rc; +}
I've been looking into updating the patch to use mod_proxy and the sub request mechanism for OCSP queries. Unfortunately the request_rec structure is not set in SSL_get_app_exdata2() at the time it is needed and there doesn't appear to be an easy way to obtain it. This may be because the requested page is unknown at this time because the SSL handshake is in progress and no HTTP headers have been sent. Does this make sub requests a non starter for OCSP or is it possible to use an alternative technique?
*** Bug 31383 has been marked as a duplicate of this bug. ***
Yes it doesn't look like use of mod_proxy subrequests will be possible unfortunately, per discussion on dev@. I've got a heavily cleaned up version of the latest patch which I will attach shortly. I notice the latest patches have a comment "based on code developed by Zetes Pass" which scares me again. It is important to understand that we cannot include code in the ASF repository unless the copyright status is clear. So again, Marc; is this an original contribution to which you entirely own the copyright? If some other party wrote half the code then we need a CLA from them too since this is a substantial contribution of new code.
> I notice the latest patches have a comment "based on code developed by Zetes > Pass" which scares me again. > is this an original contribution to which you entirely own the > copyright? You can remove the copyright, as 1. the code I mentionned was developped for the same project (Belgian Government) 2. Nothing exists anymore from the original code, I rewrote everything This was a kind of "recognition"
(In reply to comment #24) > I've got a heavily cleaned up version of the latest patch which I will attach > shortly. 1. Just to notice that ocsp.c must also be added to config.m4, but I guess you discovered this. 2. I also noticed that, in order to compile it with the latest Microsoft SDK (from Visual C++ 2008), we need to include "openssl/ocsp.h" at the very beginning of ssl_ocsp.h & ssl_engine_kernel.c. 3. I am also planning to move the directives (SSLUseOCSP, etc.) to a location level, to offer more flexibility. Do you want to tackle this at the same time, or do you prefer I do it ? 4. For which version did you plan to rework the patch ? 2.2.4, 2.2.6, Head ?
Created attachment 21130 [details] attempt 1 of refactored OCSP support This is the cleaned up version of Marc's OCSP patch, diff relative to the trunk. Relative changes: - moves OCSP code to ssl_engine_ocsp.c - heavily refactors, cleans up, simplifies code style etc in the above - tones down the debugging a lot. some common helper functions are needed in ssl_engine_log.c to log cert subject name etc, if desired - updates config.m4 - removed error handling for OpenSSL functions which can only fail on OOM - removed poorly-named SSLOCSPResponderVerify (can be added back separately) - removed addition of SSLForceValidation, which is orthogonal to basic OCSP support (likewise add separately later) - reworked the config options to be: SSLOCSPEnable <bool> SSLOCSPOverrideResponder <bool> SSLOCSPDefaultResponder <URL> rather than redundantly having two directives to supply a URL. - simplify unnecessarily complex status/error handling for OCSP code This is untested since my OCSP test setup is broken currently, so it probably doesn't actually work.
(In reply to comment #26) > 1. Just to notice that ocsp.c must also be added to config.m4, but I guess you > discovered this. Yup :) > 2. I also noticed that, in order to compile it with the latest Microsoft SDK > (from Visual C++ 2008), we need to include "openssl/ocsp.h" at the very > beginning of ssl_ocsp.h & ssl_engine_kernel.c. Why, what's the failure otherwise? The #include in ssl_toolkit_compat.h should be sufficient. > 3. I am also planning to move the directives (SSLUseOCSP, etc.) to a location > level, to offer more flexibility. Do you want to tackle this at the same time, > or do you prefer I do it ? Sounds useful, let's get the basic functionality committed first then deal with stuff like that incrementally. > 4. For which version did you plan to rework the patch ? 2.2.4, 2.2.6, Head ? trunk.
> > In order to compile it with the latest Microsoft SDK > > (from Visual C++ 2008), we need to include "openssl/ocsp.h" at the very > > beginning of ssl_ocsp.h & ssl_engine_kernel.c. > > Why, what's the failure otherwise? The #include in ssl_toolkit_compat.h > should be sufficient. Some general Apache include files end up in including standard MS SDK. In latest SDK, MS defines some types if they are not defined yet. These types are defined by OpenSSL, so they have to be included before SDK .h, so before Apache .h. This could also be done in ssl_private.h: either include "openssl/ocsp.h" before Apache files, or "ssl_toolkit_compat.h", or all "ssl*.h" files.
(In reply to comment #27) > - removed addition of SSLForceValidation, which is orthogonal to basic OCSP This is a very important feature (also for other validation mechanisms, like CRL or, maybe in the future LDAP). Without that, the administrator cannot decide what to do when no validation mechanism is available (OCSp responder not available, or CRL files not up to date). Depending on the server security level, the implementation will either be seen as conatining a security hole (the cert is accepted although it is invalid), or it will be considered as too strict (blocking valid users because of third party infrastsucture problems). This is a feature that we see as crucial for most of the application owners.
(In reply to comment #30) > (In reply to comment #27) > > - removed addition of SSLForceValidation, which is orthogonal to basic OCSP > This is a very important feature (also for other validation mechanisms, like Yes, that's fine, but it's also orthogonal to getting basic OCSP support working, so it can be added afterwards. Removing the intrusive changes to the verification callback makes the basic code easier to review and test.
Some comments on the latest patch. 1. The function extract_responder_uri() has a memory leak. It should call: AUTHORITY_INFO_ACCESS_free(values); instead of: sk_ACCESS_DESCRIPTION_free(values); 2. After the call to apr_uri_parse() shouldn't we check the scheme is really "http"? I've heard of some responders which use "https". There is also the possibility that the URL will be split up into a path and query string which should be concatenated when passed to OpenSSL. 3. The OCSP query code doesn't include a timeout. This is a problem with the OpenSSL's rather simplistic OCSP handler and the fact that there is no generalized socket timeout code in OpenSSL. There are several ways to work around this. The easiest is to use APR sockets with a timeout. See my OCSP query code in Bug 43822 4. The code unconditionally uses an OCSP nonce. Some responders do not sign every request but just server pre-cached responses. As a result the nonce value can't be honoured and an error will occur when attempting to use such responders. The most notable example is VeriSign's OCSP responder but there are others.
Created attachment 21193 [details] updated patch
Changes in second patch: 1) fixed to check URI scheme, and correctly free "values" stack per Steve's comment 2) drop the duplicate X509_STORE_CTX & X509_STORE creation. I can't see why this is necessary; Marc, can you explain what that was for? OCSP_basic_verify() creates its own X509_STORE_CTX anyway in which to do the verify the response signature, so it was never used directly. Dropping this doesn't seem to make any difference to result in testing, either. Was this just here to allow for future customisation of how the response signature is verified? 3) simplified some more logging/debugging. Uses the new ssl_log_cxerror() function added on the trunk to log cert details as context. Steve, thanks for a lot for the review - agree with your points (3) and (4) but would like to address these later.
[adding CC]
> > 2) drop the duplicate X509_STORE_CTX & X509_STORE creation. I can't see why > this is necessary; Marc, can you explain what that was for? I haven't tested it explicitly but I think the extra X509_STORE and ctx was intended to extract the issuer certificate from the client certificate in a reliable way. Note that X509_STORE_CTX_get1_issuer() will only retrieve the issuer certificate if it is trusted, hence the extra store to make all certificates trusted. To see why suppose you have this situation: Root->Intermediate->Cert Where Root only is trusted. The client would send Cert and Intermediate. The OpenSSL validation logic would then build the whole chain. A call to X509_STORE_CTX_get1_issuer() would fail because Intermediate is not in the trusted store. In actual fact it isn't necessary to create a separate store because the certificate chain has already been built and validated. All you should need to do is to extract the second member of the validated chain like this.... issuer = sk_X509_value(X509_STORE_CTX_get_chain(ctx), 1); if (issuer == NULL) /* Error */ Since issuer is an internal pointer it shouldn't be freed as it will be freed up when the ctx is cleaned up. Oh and btw you do need to free up certID.
OK, but the SSLVerify callback (and hence this OCSP validation code) is invoked for each and (necessarily) every cert from the root CA down to the peer's certificate, to verify the complete chain - so: 1) we must always be able to assume that the issuer of the X509_STORE_CTX_get_current_cert() cert is trusted, since otherwise we wouldn't get this far? 2) sk_X509_value(X509_STORE_CTX_get_chain(ctx), 1) is not necessarily the issuer of the current cert - it might *be* the current cert? ...right? Or am I missing something fundamental? On the CERTID front, if I add if (certID) OCSP_CERTID_free(certID); it crashes on that line: #0 0x0000003800e75edb in free () from /lib64/libc.so.6 #1 0x00000038094572fd in CRYPTO_free () from /lib64/libcrypto.so.6 #2 0x00000038094bcc37 in ASN1_STRING_free () from /lib64/libcrypto.so.6 ...
Created attachment 21201 [details] final patch Final patch before committing to trunk. Changes: 1) factors out the HTTP client into ssl_util_ocsp, and re-implements using APR functions directly; fixing I/O timeout handling, server address handling, and adding response memory use constraints rather than streaming into RAM indefinitely (!) as the OpenSSL code does. Also allows this code to be easily switched out for a Real HTTP Client (TM) later. 2) removes the debugging code which dumps base64-encoded which seems overkill; tcpdump/wireshark works for such case. 3) use a temporary pool to constrain connection pool memory use
> > 1) we must always be able to assume that the issuer of the > X509_STORE_CTX_get_current_cert() cert is trusted, since otherwise we wouldn't > get this far? > I'll check the current patch. As things stand I suspect if the server just trusts a root CA and the client sends root->intermediate->EE it will fail to find the intermediate CA because it isn't in the store. > 2) sk_X509_value(X509_STORE_CTX_get_chain(ctx), 1) is not necessarily the issuer > of the current cert - it might *be* the current cert? > > ...right? Or am I missing something fundamental? > I was missing something. I was assuming the OCSP calls were being made *after* the chain is validated instead of inside the verification callback. If you make OCSP calls inside the verification callback the chain may not be fully trusted when you make the OCSP requests. This would allow a carefully constructed certificate chain to persuade a server to make arbitrary OCSP requests to any URL. Some would regard this as undesirable. > On the CERTID front, if I add > > if (certID) OCSP_CERTID_free(certID); > > it crashes on that line: > Yes, I missed that, sorry. It will be freed when the request is freed.
(In reply to comment #39) > I was missing something. I was assuming the OCSP calls were being made *after* > the chain is validated instead of inside the verification callback. > > If you make OCSP calls inside the verification callback the chain may not be > fully trusted when you make the OCSP requests. This would allow a carefully > constructed certificate chain to persuade a server to make arbitrary OCSP > requests to any URL. Some would regard this as undesirable. If the cert being verified is not trusted the SSLVerify callback will get invoked with ok=0 though surely? (the OCSP code won't get invoked in that case, only if the cert *is* trusted) But I did find this confusing, anyway. Is it at all desirable to be doing OCSP validation of every cert in the chain, including whatever root CA? Marc, was the code written like this deliberately? It would be simple enough to only do the OCSP validation for the actual peer cert.
It wasn't quite as bad as I originally though. The final verification step is the signature validation of each cert in the chain. So if that is successful the callback is called ok==1 for each cert in the chain. I thought that the chain went leaf to root which would have allowed arbitrary URIs from a bogus chain. Instead it goes root to leaf which isn't as bad but would allow a bogus EE cert to trigger chain validation because it isn't checked until the end. As things stand the current_issuer field of X509_STORE_CTX can be used to obtain the issuer cert. Think that was first added in OpenSSL 0.9.7. The only other case is when ok is set to 1 because it tolerates an earlier error. That could end up doing an OCSP (and CRL) check twice AFAICS.
Committed to trunk: http://svn.apache.org/viewvc?view=rev&revision=599385 thanks to all for the patches, review, and patience to those who have worked on this. Further work: * add config options to configure whether CRL-and/or-OCSP validation is mandatory as in the "ForceValidation" config option, whether a nonce is used, what verification flags are passed to OCSP_basic_verify() * move verification to per-location context? patches welcome for all the above! Marking this fixed; for issues with the committed code please file new bugs or mail dev@httpd, likewise for discussion of above future work. (and as always, patches welcome!)
(In reply to comment #40) > Is it at all desirable to be doing OCSP validation of every cert in the chain, > including whatever root CA? > It would be simple enough to only do the OCSP validation for the actual peer > cert. If you compromise the intermediate certificate, you could create a fake OCSP server, with responses that will be accepted by Apache. The only way to ensure the OCSP response is valid is to validate its certificate, and the same up to the root.
I'm trying to add OCSP support to apache 2.2.9, and I've used this patch. But it doesn't work, because it doesn't find the function ssl_log_cxerror, that belongs (AFAIK) to apache 2.3 and up. I thought this patch was for the 2.2.x series, but if I'm wrong, what's the right patch to use with 2.2.x? Thanks in advance.
(In reply to comment #44) > I thought this patch was for the 2.2.x series, but if I'm wrong, what's the > right patch to use with 2.2.x? IMHO there is currently no patch for 2.2.x. So you need to backport the trunk version yourself.
I backported the basic OCSP support from trunk to 2.2.17. I tried to stay away from modifications out of the modules/ssl part of the source tree. This patch doesn't provide the full featured OCSP support (i.e. stapling) currently present in trunk in order to avoid a more complex patch. The lsdiff of the patch is: a/configure a/modules/ssl/mod_ssl.c a/modules/ssl/ssl_engine_config.c a/modules/ssl/ssl_engine_kernel.c a/modules/ssl/ssl_engine_log.c b/modules/ssl/ssl_engine_ocsp.c a/modules/ssl/ssl_private.h a/modules/ssl/ssl_toolkit_compat.h b/modules/ssl/ssl_util_ocsp.c and it is based in the successive application of the patches corresponding to the svn trunk revisions: 599385 599496 599497 600482 600493 600497 682788 683242 704917 757463 815719 815741 ... plus a specific patch needed to add the ssl_log_cxerror(...) function in ssl_engine_log.c. You have to define the HAVE_OCSP define flag at configure time (i.e. CPPFLAGS='-DHAVE_OCSP') in order to activate the support. All in all, it could be useful for those that can't wait for the stable version of 2.3 and needed OCSP support without stapling in 2.2.x series. Greets.
Created attachment 26420 [details] Add basic OCSP support to 2.2.17