v3 Proof of concept patch to address the issue (with signed-of-by)

0001-v3-Remove-OCSP-stapling-info-from-X509-ex_data.patch (text/plain), 9.01 KB, created by Alex Bligh on 2014-09-21 09:09:56 UTC

(hide)

Description: v3 Proof of concept patch to address the issue (with signed-of-by)

Filename:

MIME Type:

Creator: Alex Bligh

Created: 2014-09-21 09:09:56 UTC

Size: 9.01 KB

patch

obsolete

>From 697ad9179b355eacf6001eb91a11dfabad4020f1 Mon Sep 17 00:00:00 2001 >From: Alex Bligh <alex@alex.org.uk> >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. > >Signed-off-by: Alex Bligh <alex@alex.org.uk> >--- > 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 > View Attachment As Raw

This is ASF Bugzilla: the Apache Software Foundation bug system. In case of problems with the functioning of ASF Bugzilla, please contact bugzilla-admin@apache.org. Please Note: this e-mail address is only for reporting problems with ASF Bugzilla. Mail about any other subject will be silently ignored.

• • Home
  • | New
  • | Browse
  • | Search
  • |
    [?]
  • | Reports
  • | Help
  • | New Account
  • | Log In
    Remember [x]
  • | Forgot Password
    Login: [x]