From 697ad9179b355eacf6001eb91a11dfabad4020f1 Mon Sep 17 00:00:00 2001 From: Alex Bligh Date: Sun, 14 Sep 2014 20:08:45 +0000 Subject: [PATCH] (v3) Remove OCSP stapling info from X509 ex_data Remove OSCP stapling info from X509 ex_data, and manage it within normal APR pools with server lifecyle. This is to address BZ 54357 and BZ 56919. Introduce a hash of stapling info indexed by the SHA1 hash of the certificate content. Note this code as been compile tested only at this stage and is submitted as a proof of concept. Changes since v2: * change stapling_info to stapling_cert_info * move init of stapling_cert_info hash to modssl_ctx_init_server * Drop unnecessary memory allocation failure checks * Simply extraction of uri string into apr memory management * Free aia structure * In stapling_get_cert_info check for X509_digest failure * Use SHA_DIGEST_LENGTH not hardcoded 20 * Fix up second call to ssl_stapling_init_cert * Remove ssl_stapling_ex_init() declaration from ssl_private.h Changes NOT done since v2 that were suggested by Kaspar Brand * move stapling_cert_info to SSLModConfigRec. Reasons include the fact that SSLStaplingForceURL etc are per vHost. * Drop use of idx member from cert_info structure. Reason: the index to the apr hash structure has to be held somewhere for all the recorded certificates. Whilst we do calculate the SHA1 hash on the fly, the apr hash algorithm needs something to compare it to (i.e. the SHA1s of all the recorded certificates) so it can index the correct URI. As the SHA1 value needs to be stored somewhere for each certificate, it might as well be stored in the struct. It's possible I've misunderstood what Kaspar was suggesting here. --- modules/ssl/ssl_engine_config.c | 4 +++ modules/ssl/ssl_engine_init.c | 7 ++--- modules/ssl/ssl_private.h | 8 ++++-- modules/ssl/ssl_util_stapling.c | 63 ++++++++++++++--------------------------- 4 files changed, 33 insertions(+), 49 deletions(-) diff --git a/modules/ssl/ssl_engine_config.c b/modules/ssl/ssl_engine_config.c index 55366ed..4391976 100644 --- a/modules/ssl/ssl_engine_config.c +++ b/modules/ssl/ssl_engine_config.c @@ -193,6 +193,10 @@ static void modssl_ctx_init_server(SSLSrvConfigRec *sc, mctx->pks->cert_files = apr_array_make(p, 3, sizeof(char *)); mctx->pks->key_files = apr_array_make(p, 3, sizeof(char *)); +#ifdef HAVE_OCSP_STAPLING + mctx->pks->stapling_cert_info = apr_hash_make(p); +#endif + #ifdef HAVE_TLS_SESSION_TICKETS mctx->ticket_key = apr_pcalloc(p, sizeof(*mctx->ticket_key)); #endif diff --git a/modules/ssl/ssl_engine_init.c b/modules/ssl/ssl_engine_init.c index 0cdf766..b82a1f2 100644 --- a/modules/ssl/ssl_engine_init.c +++ b/modules/ssl/ssl_engine_init.c @@ -277,9 +277,6 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_pool_t *plog, if (!ssl_mutex_init(base_server, p)) { return HTTP_INTERNAL_SERVER_ERROR; } -#ifdef HAVE_OCSP_STAPLING - ssl_stapling_ex_init(); -#endif /* * initialize session caching @@ -1093,7 +1090,7 @@ static apr_status_t ssl_init_server_certs(server_rec *s, * later, we defer to the code in ssl_init_server_ctx. */ if ((mctx->stapling_enabled == TRUE) && - !ssl_stapling_init_cert(s, mctx, cert)) { + !ssl_stapling_init_cert(s, mctx, cert, p)) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02567) "Unable to configure certificate %s for stapling", key_id); @@ -1450,7 +1447,7 @@ static apr_status_t ssl_init_server_ctx(server_rec *s, SSL_CERT_SET_FIRST); while (ret) { cert = SSL_CTX_get0_certificate(sc->server->ssl_ctx); - if (!cert || !ssl_stapling_init_cert(s, sc->server, cert)) { + if (!cert || !ssl_stapling_init_cert(s, sc->server, cert, p)) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02604) "Unable to configure certificate %s:%d " "for stapling", sc->vhost_id, i); diff --git a/modules/ssl/ssl_private.h b/modules/ssl/ssl_private.h index a0100d0..41b4047 100644 --- a/modules/ssl/ssl_private.h +++ b/modules/ssl/ssl_private.h @@ -513,6 +513,11 @@ typedef struct { * sent in the CertificateRequest message: */ const char *ca_name_path; const char *ca_name_file; + +#ifdef HAVE_OCSP_STAPLING + /* Hash of stapling information by SHA1 of certificate */ + apr_hash_t *stapling_cert_info; +#endif } modssl_pk_server_t; typedef struct { @@ -812,8 +817,7 @@ const char *ssl_cmd_SSLStaplingFakeTryLater(cmd_parms *, void *, int); const char *ssl_cmd_SSLStaplingResponderTimeout(cmd_parms *, void *, const char *); const char *ssl_cmd_SSLStaplingForceURL(cmd_parms *, void *, const char *); apr_status_t modssl_init_stapling(server_rec *, apr_pool_t *, apr_pool_t *, modssl_ctx_t *); -void ssl_stapling_ex_init(void); -int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x); +int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x, apr_pool_t *p); #endif #ifdef HAVE_SRP int ssl_callback_SRPServerParams(SSL *, int *, void *); diff --git a/modules/ssl/ssl_util_stapling.c b/modules/ssl/ssl_util_stapling.c index 2dc8fce..f897a88 100644 --- a/modules/ssl/ssl_util_stapling.c +++ b/modules/ssl/ssl_util_stapling.c @@ -46,35 +46,13 @@ /* Cached info stored in certificate ex_info. */ typedef struct { /* Index in session cache SHA1 hash of certificate */ - UCHAR idx[20]; + UCHAR idx[SHA_DIGEST_LENGTH]; /* Certificate ID for OCSP requests or NULL if ID cannot be determined */ OCSP_CERTID *cid; /* Responder details */ char *uri; } certinfo; -static void certinfo_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, - int idx, long argl, void *argp) -{ - certinfo *cinf = ptr; - - if (!cinf) - return; - if (cinf->uri) - OPENSSL_free(cinf->uri); - OPENSSL_free(cinf); -} - -static int stapling_ex_idx = -1; - -void ssl_stapling_ex_init(void) -{ - if (stapling_ex_idx != -1) - return; - stapling_ex_idx = X509_get_ex_new_index(0, "X509 cached OCSP info", 0, 0, - certinfo_free); -} - static X509 *stapling_get_issuer(modssl_ctx_t *mctx, X509 *x) { X509 *issuer = NULL; @@ -106,7 +84,7 @@ static X509 *stapling_get_issuer(modssl_ctx_t *mctx, X509 *x) } -int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x) +int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x, apr_pool_t *p) { certinfo *cinf; X509 *issuer = NULL; @@ -114,21 +92,8 @@ int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x) if (x == NULL) return 0; - cinf = X509_get_ex_data(x, stapling_ex_idx); - if (cinf) { - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02215) - "ssl_stapling_init_cert: certificate already initialized!"); - return 0; - } - cinf = OPENSSL_malloc(sizeof(certinfo)); - if (!cinf) { - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02216) - "ssl_stapling_init_cert: error allocating memory!"); - return 0; - } - cinf->cid = NULL; - cinf->uri = NULL; - X509_set_ex_data(x, stapling_ex_idx, cinf); + + cinf = apr_pcalloc(p, sizeof(certinfo)); issuer = stapling_get_issuer(mctx, x); @@ -146,7 +111,11 @@ int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x) aia = X509_get1_ocsp(x); if (aia) { - cinf->uri = sk_OPENSSL_STRING_pop(aia); + /* Ensure memory managed by apr */ + char *uri; + uri = sk_OPENSSL_STRING_value(aia, 0); + if (uri) + cinf->uri = apr_pstrdup(p, uri); X509_email_free(aia); } if (!cinf->uri && !mctx->stapling_force_url) { @@ -154,6 +123,15 @@ int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x) "ssl_stapling_init_cert: no responder URL"); return 0; } + + if (apr_hash_get(mctx->pks->stapling_cert_info, cinf->idx, sizeof(cinf->idx))) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02215) + "ssl_stapling_init_cert: certificate already initialized!"); + return 0; + } + + apr_hash_set(mctx->pks->stapling_cert_info, cinf->idx, sizeof(cinf->idx), cinf); + return 1; } @@ -162,10 +140,11 @@ static certinfo *stapling_get_cert_info(server_rec *s, modssl_ctx_t *mctx, { certinfo *cinf; X509 *x; + UCHAR idx[SHA_DIGEST_LENGTH]; x = SSL_get_certificate(ssl); - if (x == NULL) + if ((x == NULL) || (X509_digest(x, EVP_sha1(), idx, NULL) != 1)) return NULL; - cinf = X509_get_ex_data(x, stapling_ex_idx); + cinf = apr_hash_get(mctx->pks->stapling_cert_info, idx, sizeof(idx)); if (cinf && cinf->cid) return cinf; ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(01926) -- 1.9.1