httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject mod_ssl/input filter review needed
Date Wed, 02 Jun 2004 11:12:01 GMT
I'm working on a fix for #12355, the (infamous?) "per-directory SSL
renegotiation vs request with bodies" bug.  The issue is explained at
length in ssl_engine_kernel.c; simply put: if an SSLRequire is specified
in a directory/location context, it's necessary to perform an SSL
handshake *after* the request body has been read.

mod_ssl for 1.3 solved this using an inelegant but effective hack which
read the entire request body into memory; it seems like a better
solution is possible for 2.0.

The approach I'm using is a new input filter which runs above (before)
the HTTP input filter, and waits for an EOS, then does the SSL
handshake.  All the data must be read from the socket before starting
the handshake, so it reads from all the non-metadata buckets to ensure
they're morphed if necessary.

Does this approach seem sane, am I missing any input filtering issues
here?  I'd appreciate some review; patch below is newer than that
attached to the bug report.

Index: modules/ssl/mod_ssl.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/mod_ssl.c,v
retrieving revision 1.97
diff -u -w -d -r1.97 mod_ssl.c
--- modules/ssl/mod_ssl.c	5 Mar 2004 02:41:39 -0000	1.97
+++ modules/ssl/mod_ssl.c	2 Jun 2004 11:06:00 -0000
@@ -469,6 +469,7 @@
 static void ssl_register_hooks(apr_pool_t *p)
 {
     ssl_io_filter_register(p);
+    ssl_reneg_filter_register(p);
 
     ap_hook_pre_connection(ssl_hook_pre_connection,NULL,NULL, APR_HOOK_MIDDLE);
     ap_hook_post_config   (ssl_init_Module,        NULL,NULL, APR_HOOK_MIDDLE);
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_kernel.c,v
retrieving revision 1.106
diff -u -w -d -r1.106 ssl_engine_kernel.c
--- modules/ssl/ssl_engine_kernel.c	25 May 2004 12:09:01 -0000	1.106
+++ modules/ssl/ssl_engine_kernel.c	2 Jun 2004 11:06:00 -0000
@@ -29,6 +29,8 @@
                                             -- Unknown                */
 #include "ssl_private.h"
 
+static int reneg_and_check(request_rec *r, int quick);
+
 /*
  *  Post Read Request Handler
  */
@@ -159,6 +161,99 @@
     return DECLINED;
 }
 
+static ap_filter_rec_t *reneg_filter_rec;
+
+/* The renegotiation input filter is inserted into the input filter
+ * stack to perform an SSL renegotatiation after the request body has
+ * been read.  It runs before the HTTP input filter and waits for it to return
+ * an EOS; at which point it is safe to perform the SSL handshake. */
+static apr_status_t reneg_in_filter(ap_filter_t *f, 
+                                    apr_bucket_brigade *bb,
+                                    ap_input_mode_t mode,
+                                    apr_read_type_e block,
+                                    apr_off_t bytes)
+{
+    apr_bucket *bkt;
+    apr_status_t rv;
+
+    /* This filter needs to buffer each brigade into memory to ensure
+     * that when an EOS is found, all data really has been read from
+     * the socket.  So, ensure that not too much is buffered: */
+    if (bytes > HUGE_STRING_LEN) {
+        bytes = HUGE_STRING_LEN;
+    }
+
+    rv = ap_get_brigade(f->next, bb, mode, block, bytes);
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+
+    for (bkt = APR_BRIGADE_FIRST(bb);
+         bkt != APR_BRIGADE_SENTINEL(bb);
+         bkt = APR_BUCKET_NEXT(bkt))
+    {
+        if (APR_BUCKET_IS_EOS(bkt)) {
+            /* No more work for this filter. */
+            ap_remove_input_filter(f);
+
+            /* Now really do the negotiation and access control checks. */
+            if (reneg_and_check(f->r, 0)) {
+                
+                /* Access control checks failed: send a 403. */
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, f->r,
+                              "renegotiation failed; sending 403 error");
+                bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+                bkt = ap_bucket_error_create(HTTP_FORBIDDEN, NULL,
+                                             f->r->pool, f->c->bucket_alloc);
+                APR_BRIGADE_INSERT_TAIL(bb, bkt);
+                bkt = apr_bucket_eos_create(f->c->bucket_alloc);
+                APR_BRIGADE_INSERT_TAIL(bb, bkt);
+                
+                rv = ap_pass_brigade(f->r->output_filters, bb);
+                if (rv)
+                    ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r,
+                                  "could not send 403 after renegotiation"
+                                  " failure");
+                
+                /* Don't give anything back to the caller, just return
+                 * an error. */
+                apr_brigade_cleanup(bb);
+                return APR_EACCES;
+            }
+
+            /* ignore the rest of the brigade */
+            break;
+        }
+
+        /* For any non-metadata buckets, read from the bucket to
+         * ensure that it is morphed into a heap bucket if it's a
+         * morphing bucket type. */
+        if (!APR_BUCKET_IS_METADATA(bkt)) {
+            const char *buf;
+            apr_size_t len;
+
+            rv = apr_bucket_read(bkt, &buf, &len, block);
+            if (rv) {
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, f->r,
+                              "bucket read failed");
+                apr_brigade_cleanup(bb);
+                return rv;
+            }
+        }
+    }
+
+    return APR_SUCCESS;
+}
+
+void ssl_reneg_filter_register(apr_pool_t *p)
+{
+    /* only requirement is that this input filter comes before the
+     * ap_http_filter. */
+    reneg_filter_rec =
+        ap_register_input_filter("SSL_RENEG", reneg_in_filter, NULL,
+                                 AP_FTYPE_PROTOCOL - 1);
+}
+
 /*
  *  Access Handler
  */
@@ -169,18 +264,12 @@
     SSLConnRec *sslconn = myConnConfig(r->connection);
     SSL *ssl            = sslconn ? sslconn->ssl : NULL;
     SSL_CTX *ctx = NULL;
-    apr_array_header_t *requires;
-    ssl_require_t *ssl_requires;
     char *cp;
-    int ok, i;
     BOOL renegotiate = FALSE, renegotiate_quick = FALSE;
-    X509 *cert;
     X509 *peercert;
-    X509_STORE *cert_store = NULL;
-    X509_STORE_CTX cert_store_ctx;
     STACK_OF(SSL_CIPHER) *cipher_list_old = NULL, *cipher_list = NULL;
     SSL_CIPHER *cipher = NULL;
-    int depth, verify_old, verify, n;
+    int verify_old, verify, n;
 
     if (ssl) {
         ctx = SSL_get_SSL_CTX(ssl);
@@ -499,79 +588,56 @@
     }
 #endif /* HAVE_SSL_SET_CERT_STORE */
 
-    /* 
-     * SSL renegotiations in conjunction with HTTP
-     * requests using the POST method are not supported.
-     *
-     * Background:
-     *
-     * 1. When the client sends a HTTP/HTTPS request, Apache's core code
-     * reads only the request line ("METHOD /path HTTP/x.y") and the
-     * attached MIME headers ("Foo: bar") up to the terminating line ("CR
-     * LF"). An attached request body (for instance the data of a POST
-     * method) is _NOT_ read. Instead it is read by mod_cgi's content
-     * handler and directly passed to the CGI script.
-     *
-     * 2. mod_ssl supports per-directory re-configuration of SSL parameters.
-     * This is implemented by performing an SSL renegotiation of the
-     * re-configured parameters after the request is read, but before the
-     * response is sent. In more detail: the renegotiation happens after the
-     * request line and MIME headers were read, but _before_ the attached
-     * request body is read. The reason simply is that in the HTTP protocol
-     * usually there is no acknowledgment step between the headers and the
-     * body (there is the 100-continue feature and the chunking facility
-     * only), so Apache has no API hook for this step.
-     *
-     * 3. the problem now occurs when the client sends a POST request for
-     * URL /foo via HTTPS the server and the server has SSL parameters
-     * re-configured on a per-URL basis for /foo. Then mod_ssl has to
-     * perform an SSL renegotiation after the request was read and before
-     * the response is sent. But the problem is the pending POST body data
-     * in the receive buffer of SSL (which Apache still has not read - it's
-     * pending until mod_cgi sucks it in). When mod_ssl now tries to perform
-     * the renegotiation the pending data leads to an I/O error.
-     *
-     * Solution Idea:
-     *
-     * There are only two solutions: Either to simply state that POST
-     * requests to URLs with SSL re-configurations are not allowed, or to
-     * renegotiate really after the _complete_ request (i.e. including
-     * the POST body) was read. Obviously the latter would be preferred,
-     * but it cannot be done easily inside Apache, because as already
-     * mentioned, there is no API step between the body reading and the body
-     * processing. And even when we mod_ssl would hook directly into the
-     * loop of mod_cgi, we wouldn't solve the problem for other handlers, of
-     * course. So the only general solution is to suck in the pending data
-     * of the request body from the OpenSSL BIO into the Apache BUFF. Then
-     * the renegotiation can be done and after this step Apache can proceed
-     * processing the request as before.
-     *
-     * Solution Implementation:
-     *
-     * We cannot simply suck in the data via an SSL_read-based loop because of
-     * HTTP chunking. Instead we _have_ to use the Apache API for this step which
-     * is aware of HTTP chunking. So the trick is to suck in the pending request
-     * data via the Apache API (which uses Apache's BUFF code and in the
-     * background mod_ssl's I/O glue code) and re-inject it later into the Apache
-     * BUFF code again. This way the data flows twice through the Apache BUFF, of
-     * course. But this way the solution doesn't depend on any Apache specifics
-     * and is fully transparent to Apache modules.
-     *
-     * !! BUT ALL THIS IS STILL NOT RE-IMPLEMENTED FOR APACHE 2.0 !!
-     */
-    if (renegotiate && !renegotiate_quick && (r->method_number == M_POST))
{
-        ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
-                     "SSL Re-negotiation in conjunction "
-                     "with POST method not supported!\n"
-                     "hint: try SSLOptions +OptRenegotiate");
+    if (!renegotiate) {
+        /* Nothing more to do here. */
+        return DECLINED;
+    }
 
-        return HTTP_METHOD_NOT_ALLOWED;
+    /* This function is called after reading the request-header.  If
+     * the HTTP request includes a message body, then the client may
+     * already have sent all the SSL records containing that body.
+     * It's not possible to do a renegotiation until all those SSL
+     * records have been read and processed, so the renegotiation has
+     * to be delayed until that point (when an EOS is returned by the
+     * HTTP input filter). */
+
+    if (!renegotiate_quick &&
+        (apr_table_get(r->headers_in, "Transfer-Encoding") ||
+         ((cp = (char *)apr_table_get(r->headers_in, "Content-Length")) != NULL
+          && strcmp(cp, "0")))) {
+        ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
+                     "SSL re-negotiation needed for request with body: "
+                     "delaying until after body has been read");
+        ap_add_input_filter_handle(reneg_filter_rec, NULL, r, r->connection);
+
+        return DECLINED;
     }
 
-    /*
-     * now do the renegotiation if anything was actually reconfigured
-     */
-    if (renegotiate) {
+    /* Now actually perform the SSL renegotiation; return DECLINED
+     * here instead of OK, to allow mod_auth and other modules to deny
+     * access. */
+    return (reneg_and_check(r, renegotiate_quick) 
+            ? HTTP_FORBIDDEN : DECLINED);
+}
+
+/* Do an SSL renegotiation and perform access control checks; do a
+ * "quick" renegotation (which doesn't actually perform an SSL
+ * handshake), if 'quick' is non-zero.  Returns non-zero if access
+ * control checks failed. */
+static int reneg_and_check(request_rec *r, int quick)
+{
+    SSLDirConfigRec *dc = myDirConfig(r);
+    SSLConnRec *sslconn = myConnConfig(r->connection);
+    SSL *ssl            = sslconn ? sslconn->ssl : NULL;
+    apr_array_header_t *requires;
+    ssl_require_t *ssl_requires;
+    X509_STORE *cert_store = NULL;
+    X509_STORE_CTX cert_store_ctx;
+    X509 *cert;
+    SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);
+    int depth, i, ok;
+    char *cp;
+
         /*
          * Now we force the SSL renegotation by sending the Hello Request
          * message to the client. Here we have to do a workaround: Actually
@@ -586,7 +652,7 @@
         ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
                      "Requesting connection re-negotiation");
 
-        if (renegotiate_quick) {
+    if (quick) {
             STACK_OF(X509) *cert_stack;
 
             /* perform just a manual re-verification of the peer */
@@ -612,7 +678,7 @@
                 ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                              "Cannot find peer certificate chain");
 
-                return HTTP_FORBIDDEN;
+            return 1;
             }
 
             if (!(cert_store ||
@@ -621,7 +687,7 @@
                 ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                              "Cannot find certificate storage");
 
-                return HTTP_FORBIDDEN;
+            return 1;
             }
 
             if (!cert) {
@@ -673,7 +739,7 @@
                              "Re-negotiation request failed");
 
                 r->connection->aborted = 1;
-                return HTTP_FORBIDDEN;
+            return 1;
             }
 
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
@@ -692,7 +758,7 @@
                         "Not accepted by client!?");
 
                 r->connection->aborted = 1;
-                return HTTP_FORBIDDEN;
+            return 1;
             }
         }
 
@@ -717,23 +783,22 @@
                 ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                              "Re-negotiation handshake failed: "
                              "Client verification failed");
-
-                return HTTP_FORBIDDEN;
+            return 1;
             }
 
             if (do_verify) {
+            X509 *peercert;
+
                 if ((peercert = SSL_get_peer_certificate(ssl)) == NULL) {
                     ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                                  "Re-negotiation handshake failed: "
                                  "Client certificate missing");
-
-                    return HTTP_FORBIDDEN;
+                return 1;
                 }
 
                 X509_free(peercert);
             }
         }
-    }
 
     /*
      * Check SSLRequire boolean expressions
@@ -758,7 +823,7 @@
             /* remember forbidden access for strict require option */
             apr_table_setn(r->notes, "ssl-access-forbidden", "1");
 
-            return HTTP_FORBIDDEN;
+            return 1;
         }
 
         if (ok != 1) {
@@ -779,18 +844,11 @@
             /* remember forbidden access for strict require option */
             apr_table_setn(r->notes, "ssl-access-forbidden", "1");
 
-            return HTTP_FORBIDDEN;
+            return 1;
         }
     }
 
-    /*
-     * Else access is granted from our point of view (except vendor
-     * handlers override). But we have to return DECLINED here instead
-     * of OK, because mod_auth and other modules still might want to
-     * deny access.
-     */
-
-    return DECLINED;
+    return 0;
 }
 
 /*
Index: modules/ssl/ssl_private.h
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_private.h,v
retrieving revision 1.4
diff -u -w -d -r1.4 ssl_private.h
--- modules/ssl/ssl_private.h	27 May 2004 09:20:00 -0000	1.4
+++ modules/ssl/ssl_private.h	2 Jun 2004 11:06:01 -0000
@@ -577,6 +577,7 @@
 void         ssl_io_filter_init(conn_rec *, SSL *);
 void         ssl_io_filter_register(apr_pool_t *);
 long         ssl_io_data_cb(BIO *, int, MODSSL_BIO_CB_ARG_TYPE *, int, long, long);
+void         ssl_reneg_filter_register(apr_pool_t *p);
 
 /*  PRNG  */
 int          ssl_rand_seed(server_rec *, apr_pool_t *, ssl_rsctx_t, char *);

Mime
View raw message