From 6e24e496c7aee8aa1ff13a41dae71c91fe8c0bbe Mon Sep 17 00:00:00 2001 From: Alex Bligh Date: Thu, 6 Nov 2014 20:37:42 +0000 Subject: [PATCH] LP#1366174: Backport PR54357 to 2.4.7 - Crash during restart or at startup in mod_ssl, in certinfo_free() function registered by ssl_stapling_ex_init() Backport SVN: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1634529 details of which follow: Merge r1629372, r1629485, r1629519 from trunk: Move OCSP stapling information from a per-certificate store (ex_data attached to an X509 *) to a per-server hash which is allocated from the pconf pool. Fixes PR 54357, PR 56919 and a leak with the certinfo_free cleanup function (missing OCSP_CERTID_free). * modules/ssl/ssl_util_stapling.c: drop certinfo_free, and add ssl_stapling_certid_free (used with apr_pool_cleanup_register). Switch to a stapling_certinfo hash which is keyed by the SHA-1 digest of the certificate's DER encoding, rework ssl_stapling_init_cert to only store info once per certificate (allocated from the pconf to the extent possible) and extend the logging. * modules/ssl/ssl_private.h: adjust prototype for ssl_stapling_init_cert, replace ssl_stapling_ex_init with ssl_stapling_certinfo_hash_init * modules/ssl/ssl_engine_init.c: adjust ssl_stapling_* calls Based on initial work by Alex Bligh Follow up to r1629372: ensure compatibily with OpenSSL < 1.0 (sk_OPENSSL_STRING_value). Follow up to r1629372 and r1629485: ensure compatibily with OpenSSL < 1.0 (sk_OPENSSL_STRING_[num|value|pop] macros). Submitted by: kbrand, ylavic, ylavic Reviewed/backported by: jim --- debian/changelog | 8 +++ modules/ssl/ssl_engine_init.c | 12 ++-- modules/ssl/ssl_private.h | 14 ++++- modules/ssl/ssl_util_stapling.c | 133 +++++++++++++++++++++++----------------- 4 files changed, 104 insertions(+), 63 deletions(-) diff --git a/debian/changelog b/debian/changelog index 0428542..7142a59 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,11 @@ +apache2 (2.4.7-1ubuntu4.2~sslstapling1) trusty; urgency=medium + + * UPDATE: fix Crash during restart or at startup in mod_ssl, in + certinfo_free() function registered by ssl_stapling_ex_init() + - Backport of PR54357, LP#1366174 + + -- Alex Bligh Thu, 06 Nov 2014 20:45:53 +0000 + apache2 (2.4.7-1ubuntu4.1) trusty-security; urgency=medium * SECURITY UPDATE: denial of service in mod_proxy diff --git a/modules/ssl/ssl_engine_init.c b/modules/ssl/ssl_engine_init.c index f6e010d..8737063 100644 --- a/modules/ssl/ssl_engine_init.c +++ b/modules/ssl/ssl_engine_init.c @@ -200,7 +200,7 @@ int ssl_init_Module(apr_pool_t *p, apr_pool_t *plog, return HTTP_INTERNAL_SERVER_ERROR; } #ifdef HAVE_OCSP_STAPLING - ssl_stapling_ex_init(); + ssl_stapling_certinfo_hash_init(p); #endif /* @@ -818,6 +818,8 @@ static void ssl_init_ctx(server_rec *s, } static int ssl_server_import_cert(server_rec *s, + apr_pool_t *p, + apr_pool_t *ptemp, modssl_ctx_t *mctx, const char *id, int idx) @@ -852,7 +854,7 @@ static int ssl_server_import_cert(server_rec *s, #ifdef HAVE_OCSP_STAPLING if ((mctx->pkp == FALSE) && (mctx->stapling_enabled == TRUE)) { - if (!ssl_stapling_init_cert(s, mctx, cert)) { + if (!ssl_stapling_init_cert(s, p, ptemp, mctx, cert)) { ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02235) "Unable to configure server certificate for stapling"); } @@ -1000,10 +1002,10 @@ static void ssl_init_server_certs(server_rec *s, ecc_id = ssl_asn1_table_keyfmt(ptemp, vhost_id, SSL_AIDX_ECC); #endif - have_rsa = ssl_server_import_cert(s, mctx, rsa_id, SSL_AIDX_RSA); - have_dsa = ssl_server_import_cert(s, mctx, dsa_id, SSL_AIDX_DSA); + have_rsa = ssl_server_import_cert(s, p, ptemp, mctx, rsa_id, SSL_AIDX_RSA); + have_dsa = ssl_server_import_cert(s, p, ptemp, mctx, dsa_id, SSL_AIDX_DSA); #ifdef HAVE_ECC - have_ecc = ssl_server_import_cert(s, mctx, ecc_id, SSL_AIDX_ECC); + have_ecc = ssl_server_import_cert(s, p, ptemp, mctx, ecc_id, SSL_AIDX_ECC); #endif if (!(have_rsa || have_dsa diff --git a/modules/ssl/ssl_private.h b/modules/ssl/ssl_private.h index 4ea924f..615dfde 100644 --- a/modules/ssl/ssl_private.h +++ b/modules/ssl/ssl_private.h @@ -147,6 +147,13 @@ /* OCSP stapling */ #if !defined(OPENSSL_NO_OCSP) && defined(SSL_CTX_set_tlsext_status_cb) #define HAVE_OCSP_STAPLING +/* backward compatibility with OpenSSL < 1.0 */ +#ifndef sk_OPENSSL_STRING_num +#define sk_OPENSSL_STRING_num sk_num +#endif +#ifndef sk_OPENSSL_STRING_value +#define sk_OPENSSL_STRING_value sk_value +#endif #ifndef sk_OPENSSL_STRING_pop #define sk_OPENSSL_STRING_pop sk_pop #endif @@ -826,10 +833,11 @@ const char *ssl_cmd_SSLStaplingErrorCacheTimeout(cmd_parms *, void *, const char const char *ssl_cmd_SSLStaplingReturnResponderErrors(cmd_parms *, void *, int); 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 *); +const char *ssl_cmd_SSLStaplingForceURL(cmd_parms *, void *, const char *); void 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); +void ssl_stapling_certinfo_hash_init(apr_pool_t *); +int ssl_stapling_init_cert(server_rec *s, apr_pool_t *, apr_pool_t *, + modssl_ctx_t *mctx, X509 *x); #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 0387cf9..9bb097d 100644 --- a/modules/ssl/ssl_util_stapling.c +++ b/modules/ssl/ssl_util_stapling.c @@ -43,36 +43,32 @@ #define MAX_STAPLING_DER 10240 -/* Cached info stored in certificate ex_info. */ +/* Cached info stored in the global stapling_certinfo hash. */ typedef struct { - /* Index in session cache SHA1 hash of certificate */ - UCHAR idx[20]; - /* Certificate ID for OCSP requests or NULL if ID cannot be determined */ + /* Index in session cache (SHA-1 digest of DER encoded certificate) */ + UCHAR idx[SHA_DIGEST_LENGTH]; + /* Certificate ID for OCSP request */ OCSP_CERTID *cid; - /* Responder details */ + /* URI of the OCSP responder */ char *uri; } certinfo; -static void certinfo_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, - int idx, long argl, void *argp) +static apr_status_t ssl_stapling_certid_free(void *data) { - certinfo *cinf = ptr; + OCSP_CERTID *cid = data; - if (!cinf) - return; - if (cinf->uri) - OPENSSL_free(cinf->uri); - OPENSSL_free(cinf); + if (cid) { + OCSP_CERTID_free(cid); + } + + return APR_SUCCESS; } -static int stapling_ex_idx = -1; +static apr_hash_t *stapling_certinfo; -void ssl_stapling_ex_init(void) +void ssl_stapling_certinfo_hash_init(apr_pool_t *p) { - if (stapling_ex_idx != -1) - return; - stapling_ex_idx = X509_get_ex_new_index(0, "X509 cached OCSP info", 0, 0, - certinfo_free); + stapling_certinfo = apr_hash_make(p); } static X509 *stapling_get_issuer(modssl_ctx_t *mctx, X509 *x) @@ -106,69 +102,96 @@ 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, apr_pool_t *p, apr_pool_t *ptemp, + modssl_ctx_t *mctx, X509 *x) { - certinfo *cinf; + UCHAR idx[SHA_DIGEST_LENGTH]; + certinfo *cinf = NULL; X509 *issuer = NULL; + OCSP_CERTID *cid = NULL; STACK_OF(OPENSSL_STRING) *aia = NULL; - if (x == NULL) + if ((x == NULL) || (X509_digest(x, EVP_sha1(), idx, NULL) != 1)) return 0; - cinf = X509_get_ex_data(x, stapling_ex_idx); + + cinf = apr_hash_get(stapling_certinfo, idx, sizeof(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; + /* + * We already parsed the certificate, and no OCSP URI was found. + * The certificate might be used for multiple vhosts, though, + * so we check for a ForceURL for this vhost. + */ + if (!cinf->uri && !mctx->stapling_force_url) { + ssl_log_xerror(SSLLOG_MARK, APLOG_ERR, 0, ptemp, s, x, + APLOGNO(02814) "ssl_stapling_init_cert: no OCSP URI " + "in certificate and no SSLStaplingForceURL " + "configured for server %s", mctx->sc->vhost_id); + return 0; + } + return 1; } - cinf->cid = NULL; - cinf->uri = NULL; - X509_set_ex_data(x, stapling_ex_idx, cinf); - issuer = stapling_get_issuer(mctx, x); - - if (issuer == NULL) { - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02217) - "ssl_stapling_init_cert: Can't retrieve issuer certificate!"); + if (!(issuer = stapling_get_issuer(mctx, x))) { + ssl_log_xerror(SSLLOG_MARK, APLOG_ERR, 0, ptemp, s, x, APLOGNO(02217) + "ssl_stapling_init_cert: can't retrieve issuer " + "certificate!"); return 0; } - cinf->cid = OCSP_cert_to_id(NULL, x, issuer); + cid = OCSP_cert_to_id(NULL, x, issuer); X509_free(issuer); - if (!cinf->cid) + if (!cid) { + ssl_log_xerror(SSLLOG_MARK, APLOG_ERR, 0, ptemp, s, x, APLOGNO(02815) + "ssl_stapling_init_cert: can't create CertID " + "for OCSP request"); return 0; - X509_digest(x, EVP_sha1(), cinf->idx, NULL); + } aia = X509_get1_ocsp(x); - if (aia) - cinf->uri = sk_OPENSSL_STRING_pop(aia); - if (!cinf->uri && !mctx->stapling_force_url) { - ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02218) - "ssl_stapling_init_cert: no responder URL"); + if (!aia && !mctx->stapling_force_url) { + OCSP_CERTID_free(cid); + ssl_log_xerror(SSLLOG_MARK, APLOG_ERR, 0, ptemp, s, x, + APLOGNO(02218) "ssl_stapling_init_cert: no OCSP URI " + "in certificate and no SSLStaplingForceURL set"); + return 0; } - if (aia) + + /* At this point, we have determined that there's something to store */ + cinf = apr_pcalloc(p, sizeof(certinfo)); + memcpy (cinf->idx, idx, sizeof(idx)); + cinf->cid = cid; + /* make sure cid is also freed at pool cleanup */ + apr_pool_cleanup_register(p, cid, ssl_stapling_certid_free, + apr_pool_cleanup_null); + if (aia) { + /* allocate uri from the pconf pool */ + cinf->uri = apr_pstrdup(p, sk_OPENSSL_STRING_value(aia, 0)); X509_email_free(aia); + } + + ssl_log_xerror(SSLLOG_MARK, APLOG_TRACE1, 0, ptemp, s, x, + "ssl_stapling_init_cert: storing certinfo for server %s", + mctx->sc->vhost_id); + + apr_hash_set(stapling_certinfo, cinf->idx, sizeof(cinf->idx), cinf); + return 1; } -static certinfo *stapling_get_cert_info(server_rec *s, modssl_ctx_t *mctx, - SSL *ssl) +static certinfo *stapling_get_certinfo(server_rec *s, modssl_ctx_t *mctx, + SSL *ssl) { 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(stapling_certinfo, idx, sizeof(idx)); if (cinf && cinf->cid) return cinf; ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(01926) - "stapling_get_cert_info: stapling not supported for certificate"); + "stapling_get_certinfo: stapling not supported for certificate"); return NULL; } @@ -577,7 +600,7 @@ static int stapling_cb(SSL *ssl, void *arg) ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(01951) "stapling_cb: OCSP Stapling callback called"); - cinf = stapling_get_cert_info(s, mctx, ssl); + cinf = stapling_get_certinfo(s, mctx, ssl); if (cinf == NULL) { return SSL_TLSEXT_ERR_NOACK; } -- 1.9.1