Mailing-List: contact cvs-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1002227 - in /httpd/httpd/branches/2.0.x: CHANGES STATUS
 modules/ssl/ssl_engine_io.c modules/ssl/ssl_engine_kernel.c
Date: Tue, 28 Sep 2010 15:49:08 -0000
To: cvs@httpd.apache.org
From: rjung@apache.org
Message-Id: <20100928154908.301B22388906@eris.apache.org>

Author: rjung
Date: Tue Sep 28 15:49:07 2010
New Revision: 1002227

URL: http://svn.apache.org/viewvc?rev=1002227&view=rev
Log:
Merge r891282 from trunk resp. 896900 from 2.2.x:

Further mitigation for the TLS renegotation attack, CVE-2009-3555:

* modules/ssl/ssl_engine_kernel.c (has_buffered_data): New function.
  (ssl_hook_Access): Forcibly disable keepalive for the connection if
  there is any buffered data readable from the input filter stack.

* modules/ssl/ssl_engine_io.c (ssl_io_filter_input): Ensure that the
  BIO uses blocking operations when invoked outside direct control of
  the httpd filter stack.

Thanks to Hartmut Keil <Hartmut.Keil adnovum.ch> for proposing this
technique.

Submitted by: jorton
Backport by: rjung
Reviewed by: pgollucci, wrowe

Modified:
    httpd/httpd/branches/2.0.x/CHANGES
    httpd/httpd/branches/2.0.x/STATUS
    httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_io.c
    httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_kernel.c

Modified: httpd/httpd/branches/2.0.x/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.0.x/CHANGES?rev=1002227&r1=1002226&r2=1002227&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/CHANGES [utf-8] (original)
+++ httpd/httpd/branches/2.0.x/CHANGES [utf-8] Tue Sep 28 15:49:07 2010
@@ -18,10 +18,11 @@ Changes with Apache 2.0.64
   *) SECURITY: CVE-2009-3555 (cve.mitre.org)
      mod_ssl: A partial fix for the TLS renegotiation prefix injection attack
      for OpenSSL versions prior to 0.9.8l; reject any client-initiated
-     renegotiations. Any configuration which requires renegotiation for
-     per-directory/location access control is still vulnerable, unless using
-     OpenSSL 0.9.8l or later.
-     [Joe Orton, Ruediger Pluem, Rainer Jung]
+     renegotiations. Forcibly disable keepalive for the connection if there
+     is any buffered data readable. Any configuration which requires
+     renegotiation for per-directory/location access control is still
+     vulnerable, unless using openssl 0.9.8l or later.
+     [Joe Orton, Ruediger Pluem, Hartmut Keil <Hartmut.Keil adnovum.ch>, Rainer Jung]
 
   *) SECURITY: CVE-2010-0434 (cve.mitre.org)
      Ensure each subrequest has a shallow copy of headers_in so that the

Modified: httpd/httpd/branches/2.0.x/STATUS
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.0.x/STATUS?rev=1002227&r1=1002226&r2=1002227&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/STATUS (original)
+++ httpd/httpd/branches/2.0.x/STATUS Tue Sep 28 15:49:07 2010
@@ -137,15 +137,6 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
       http://people.apache.org/~rjung/patches/SSLInsecureRenegotiation_httpd_2_0_x-backport-r917044.patch 
     +1: rjung, pgollucci (+1 2.0.64 w/ this), wrowe
 
-  * mod_ssl: Further mitigation for the TLS renegotation attack, CVE-2009-3555
-    Trunk version of patch:
-      http://svn.apache.org/viewvc?rev=891282&view=rev
-    Patch in 2.2.x branch:
-      http://svn.apache.org/viewvc?rev=896900&view=rev
-    Backport:
-      http://people.apache.org/~rjung/patches/cve-2009-3555_httpd_2_0_x-backport-r891282.patch
-    +1: rjung, pgollucci (+1 2.0.64 w/ this), wrowe
-
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   [ please place SVN revisions from trunk here, so it is easy to
     identify exactly what the proposed changes are!  Add all new

Modified: httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_io.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_io.c?rev=1002227&r1=1002226&r2=1002227&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_io.c (original)
+++ httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_io.c Tue Sep 28 15:49:07 2010
@@ -1261,9 +1261,17 @@ static apr_status_t ssl_io_filter_input(
     }
     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);
     }

Modified: httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_kernel.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_kernel.c?rev=1002227&r1=1002226&r2=1002227&view=diff
==============================================================================
--- httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_kernel.c (original)
+++ httpd/httpd/branches/2.0.x/modules/ssl/ssl_engine_kernel.c Tue Sep 28 15:49:07 2010
@@ -31,6 +31,29 @@
 #include "mod_ssl.h"
 
 static void ssl_configure_env(request_rec *r, SSLConnRec *sslconn);
+ 
+/* Perform a speculative (and non-blocking) read from the connection
+ * filters for the given request, to determine whether there is any
+ * pending data to read.  Return non-zero if there is, else zero. */
+static int has_buffered_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
@@ -602,6 +625,23 @@ int ssl_hook_Access(request_rec *r)
         else {
             request_rec *id = r->main ? r->main : r;
 
+            /* Additional 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 response ensures such data will be
+             * discarded.  Legimately pipelined HTTP requests will be
+             * retried anyway with this approach. */
+            if (has_buffered_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_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                          "Performing full renegotiation: "