Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of jorton@redhat.com designates
 209.132.183.28 as permitted sender)
Date: Thu, 19 Nov 2009 15:58:54 +0000
From: Joe Orton <jorton@redhat.com>
To: dev@httpd.apache.org
Subject: Re: handling request splicing in case of server initiated
	renegotiation CVE-2009-3555
Message-ID: <20091119155854.GA25109@redhat.com>
Mail-Followup-To: dev@httpd.apache.org
References: <4B01BD20.1060300@adnovum.ch> <20091116221903.GB18036@redhat.com>
 <4B027E20.2030200@adnovum.ch> <20091117130812.GB29064@redhat.com>
 <4B02D989.1070604@adnovum.ch> <20091119093041.GA9262@redhat.com>
 <4B055EBE.5090307@adnovum.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4B055EBE.5090307@adnovum.ch>
User-Agent: Mutt/1.5.19 (2009-01-05)
Organization: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor,
 Berkshire, SL4 1TE, United Kingdom.
  Registered in UK and Wales under Company Registration No. 03798903
  Directors: Michael Cunningham (USA), Brendan Lane (Ireland),
 Matt Parson (USA), Charlie Peters (USA)

On Thu, Nov 19, 2009 at 04:05:34PM +0100, Hartmut Keil wrote:
> With the proposed change, we prevent request splitting attacks based 
> on the TSL renegotiation flaw. From my point of view without 
> drawbacks, since 'pipelining' clients must handle the closing of a 
> connection after a complete response in any case.

Yes, I agree, this seems very sensible, I can't see any problem with 
this.  

I would prefer to do it in a slightly more general way as below, which 
would catch the case where any other module's connection filter had 
buffered the data, and adds appropriate logging.

(more general but which required half a day tracking down an obscure bug 
in the BIO/filters, also fixed below...)

Testing on this version very welcome!

Index: ssl_engine_kernel.c
===================================================================
--- ssl_engine_kernel.c	(revision 882089)
+++ ssl_engine_kernel.c	(working copy)
@@ -87,6 +87,29 @@
     return APR_SUCCESS;
 }
 
+/* Do a non-blocking read from the connection filters to see whether
+ * there is any pending data on the connection.  Return non-zero if
+ * there is, else zero. */
+static int has_pending_data(request_rec *r) 
+{
+    apr_bucket_brigade *bb;
+    apr_off_t len;
+    apr_status_t rv;
+    int result;
+    
+    bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+    
+    rv = ap_get_brigade(r->connection->input_filters, bb, AP_MODE_SPECULATIVE,
+                        APR_NONBLOCK_READ, 1); 
+    result = rv == APR_SUCCESS
+        && apr_brigade_length(bb, 1, &len) == APR_SUCCESS
+        && len > 0;
+    
+    apr_brigade_destroy(bb);
+    
+    return result;
+}
+
 /*
  *  Post Read Request Handler
  */
@@ -724,6 +747,23 @@
         else {
             request_rec *id = r->main ? r->main : r;
 
+            /* Mitigation for CVE-2009-3555: At this point, before
+             * renegotiating, an (entire) request has been read from
+             * the connection.  An attacker may have sent further data
+             * to "prefix" any subsequent request by the victim's
+             * client after the renegotiation; this data may already
+             * have been read and buffered.  Forcing a connection
+             * closure after the first response ensures such data will
+             * be discarded.  Legimately pipelined HTTP requests will
+             * be retried anyway with this approach. */
+            if (has_pending_data(r)) {
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+                              "insecure SSL re-negotiation required, but "
+                              "a pipelined request is present; keepalive "
+                              "disabled");
+                r->connection->keepalive = AP_CONN_CLOSE;
+            }
+
             /* do a full renegotiation */
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
                           "Performing full renegotiation: "
Index: ssl_engine_io.c
===================================================================
--- ssl_engine_io.c	(revision 882089)
+++ ssl_engine_io.c	(working copy)
@@ -1344,9 +1344,17 @@
     }
     else {
         /* We have no idea what you are talking about, so return an error. */
-        return APR_ENOTIMPL;
+        status = APR_ENOTIMPL;
     }
 
+    /* It is possible for mod_ssl's BIO to be used outside of the
+     * direct control of mod_ssl's input or output filter -- notably,
+     * when mod_ssl initiates a renegotiation.  Switching the BIO mode
+     * back to "blocking" here ensures such operations don't fail with
+     * SSL_ERROR_WANT_READ. */
+    inctx->block = APR_BLOCK_READ;
+
+    /* Handle custom errors. */
     if (status != APR_SUCCESS) {
         return ssl_io_filter_error(f, bb, status);
     }