]> granicus.if.org Git - apache/commitdiff
Merge r1629372, r1629485, r1629519 from trunk:
authorJim Jagielski <jim@apache.org>
Mon, 27 Oct 2014 12:50:05 +0000 (12:50 +0000)
committerJim Jagielski <jim@apache.org>
Mon, 27 Oct 2014 12:50:05 +0000 (12:50 +0000)
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>

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

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1634529 13f79535-47bb-0310-9956-ffa450edef68

CHANGES
STATUS
modules/ssl/ssl_engine_init.c
modules/ssl/ssl_private.h
modules/ssl/ssl_util_stapling.c

diff --git a/CHANGES b/CHANGES
index 7b7f2d1349e4f7bef280f0a006902ba175091b78..8dc4114b12825cc0d2ad1a899f0238f11cdf1cf3 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,10 @@
 
 Changes with Apache 2.4.11
 
+  *) 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>,
+     Yann Ylavic, Kaspar Brand]
+
   *) mod_cache_socache: Change average object size hint from 32 bytes to
      2048 bytes.  [Rainer Jung]
 
diff --git a/STATUS b/STATUS
index c3cc519ff27fa59a9303f8960eb7212896ecdbaa..2902968d56eb5500366ebc204a2934806f5e37a8 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -102,12 +102,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-   * mod_ssl: Move OCSP stapling information to a per-server hash. PR 54357.
-     trunk patches: https://svn.apache.org/r1629372
-                    https://svn.apache.org/r1629485
-                    https://svn.apache.org/r1629519
-     2.4.x patch: https://people.apache.org/~kbrand/mod_ssl-2.4.x-PR54357.diff
-     +1: kbrand, ylavic, jim
 
 
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
index 3c2440c2d5db65167037f35b139cf8563aa7fbec..6648f7c2df164df3590dd2b35bf55d5c72e70810 100644 (file)
@@ -272,7 +272,7 @@ apr_status_t 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
 
     /*
@@ -1067,7 +1067,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, p, ptemp, mctx, cert)) {
             ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02567)
                          "Unable to configure certificate %s for stapling",
                          key_id);
@@ -1425,7 +1425,8 @@ 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, 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);
index e8e2cff08b0bad231c7094351b9a8369e61c0367..9de0bd886937ee4f584019b986644bcd185ca3d7 100644 (file)
 /* 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
@@ -812,10 +819,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 *);
 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 *);
index 2dc8fceaaa8a2419a627c0e8d1d946b06ea0fc37..81e95b41cad7ee7e653e977f1b13045985fef801 100644 (file)
 
 #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_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);
-        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 *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;
     }