Discussion:
svn commit: r1629372 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/ssl/ssl_engine_init.c modules/ssl/ssl_private.h modules/ssl/ssl_util_stapling.c
k***@apache.org
2014-10-04 10:58:49 UTC
Permalink
Author: kbrand
Date: Sat Oct 4 10:58:49 2014
New Revision: 1629372

URL: http://svn.apache.org/r1629372
Log:
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 <alex alex.org.uk>

Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
httpd/httpd/trunk/modules/ssl/ssl_private.h
httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c

Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1629372&r1=1629371&r2=1629372&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Sat Oct 4 10:58:49 2014
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.0

+ *) mod_ssl: Move OCSP stapling information from a per-certificate store to
+ a per-server hash. PR 54357, PR 56919. [Alex Bligh <alex alex.org.uk>,
+ Kaspar Brand]
+
*) mod_substitute: Restrict configuration in .htaccess to
FileInfo as documented. [Rainer Jung]


Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1629372&r1=1629371&r2=1629372&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
+++ httpd/httpd/trunk/docs/log-message-tags/next-number Sat Oct 4 10:58:49 2014
@@ -1 +1 @@
-2814
+2816

Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_init.c?rev=1629372&r1=1629371&r2=1629372&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Sat Oct 4 10:58:49 2014
@@ -278,7 +278,7 @@ apr_status_t ssl_init_Module(apr_pool_t
return HTTP_INTERNAL_SERVER_ERROR;
}
#ifdef HAVE_OCSP_STAPLING
- ssl_stapling_ex_init();
+ ssl_stapling_certinfo_hash_init(p);
#endif

/*
@@ -1093,7 +1093,7 @@ static apr_status_t ssl_init_server_cert
* 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, p, ptemp, mctx, cert)) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02567)
"Unable to configure certificate %s for stapling",
key_id);
@@ -1450,7 +1450,8 @@ static apr_status_t ssl_init_server_ctx(
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, p, ptemp, sc->server,
+ cert)) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02604)
"Unable to configure certificate %s:%d "
"for stapling", sc->vhost_id, i);

Modified: httpd/httpd/trunk/modules/ssl/ssl_private.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_private.h?rev=1629372&r1=1629371&r2=1629372&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_private.h (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_private.h Sat Oct 4 10:58:49 2014
@@ -810,10 +810,11 @@ const char *ssl_cmd_SSLStaplingErrorCach
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 *);
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);
+void ssl_stapling_certinfo_hash_init(apr_pool_t *);
+int ssl_stapling_init_cert(server_rec *, apr_pool_t *, apr_pool_t *,
+ modssl_ctx_t *, X509 *);
#endif
#ifdef HAVE_SRP
int ssl_callback_SRPServerParams(SSL *, int *, void *);

Modified: httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c?rev=1629372&r1=1629371&r2=1629372&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c Sat Oct 4 10:58:49 2014
@@ -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,70 +102,96 @@ static X509 *stapling_get_issuer(modssl_

}

-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;
- }
- 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!");
+ /*
+ * 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;
+ }
+
+ 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);
- X509_email_free(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;
}
+
+ /* 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,
+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;
}

@@ -585,7 +607,7 @@ static int stapling_cb(SSL *ssl, void *a
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;
}

Loading...