From 2af879005391a8d541216d87875a382a516631b0 Mon Sep 17 00:00:00 2001 From: Alex Bligh Date: Sun, 14 Sep 2014 20:08:45 +0000 Subject: [PATCH] (v4) Remove OCSP stapling info from X509 ex_data Remove OSCP stapling info from X509 ex_data, and manage it within normal APR pools with SSLModConfigRec 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 v3: * move stapling_cert_info to SSLModConfigRec 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 v3 that were suggested by Kaspar Brand * 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. Signed-off-by: Alex Bligh --- modules/ssl/ssl_engine_config.c | 1 + modules/ssl/ssl_engine_init.c | 3 --- modules/ssl/ssl_private.h | 4 ++- modules/ssl/ssl_util_stapling.c | 58 ++++++++++++----------------------------- 4 files changed, 21 insertions(+), 45 deletions(-) diff --git a/modules/ssl/ssl_engine_config.c b/modules/ssl/ssl_engine_config.c index 55366ed..25c3eeb 100644 --- a/modules/ssl/ssl_engine_config.c +++ b/modules/ssl/ssl_engine_config.c @@ -72,6 +72,7 @@ SSLModConfigRec *ssl_config_global_create(server_rec *s) #ifdef HAVE_OCSP_STAPLING mc->stapling_cache = NULL; mc->stapling_mutex = NULL; + mc->stapling_cert_info = apr_hash_make(pool); #endif apr_pool_userdata_set(mc, SSL_MOD_CONFIG_KEY, diff --git a/modules/ssl/ssl_engine_init.c b/modules/ssl/ssl_engine_init.c index 0cdf766..c7a59b3 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 diff --git a/modules/ssl/ssl_private.h b/modules/ssl/ssl_private.h index a0100d0..72729d1 100644 --- a/modules/ssl/ssl_private.h +++ b/modules/ssl/ssl_private.h @@ -499,6 +499,9 @@ typedef struct { const ap_socache_provider_t *stapling_cache; ap_socache_instance_t *stapling_cache_context; apr_global_mutex_t *stapling_mutex; + + /* Hash of stapling information by SHA1 of certificate */ + apr_hash_t *stapling_cert_info; #endif } SSLModConfigRec; @@ -812,7 +815,6 @@ 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); #endif #ifdef HAVE_SRP diff --git a/modules/ssl/ssl_util_stapling.c b/modules/ssl/ssl_util_stapling.c index 2dc8fce..857b064 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; @@ -111,24 +89,13 @@ int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x) certinfo *cinf; X509 *issuer = NULL; STACK_OF(OPENSSL_STRING) *aia = NULL; + SSLModConfigRec *mc = myModConfig(s); + apr_pool_t *p = mc->pPool; 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 +113,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 +125,9 @@ int ssl_stapling_init_cert(server_rec *s, modssl_ctx_t *mctx, X509 *x) "ssl_stapling_init_cert: no responder URL"); return 0; } + + apr_hash_set(mc->stapling_cert_info, cinf->idx, sizeof(cinf->idx), cinf); + return 1; } @@ -162,10 +136,12 @@ static certinfo *stapling_get_cert_info(server_rec *s, modssl_ctx_t *mctx, { certinfo *cinf; X509 *x; + UCHAR idx[SHA_DIGEST_LENGTH]; + SSLModConfigRec *mc = myModConfig(s); 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(mc->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