From: Rainer Jung Date: Tue, 28 Sep 2010 15:49:07 +0000 (+0000) Subject: Merge r891282 from trunk resp. 896900 from 2.2.x: X-Git-Tag: 2.0.64~24 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=870602222496d2e861875bc7779c0d9689f57f1e;p=thirdparty%2Fapache%2Fhttpd.git 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 for proposing this technique. Submitted by: jorton Backport by: rjung Reviewed by: pgollucci, wrowe git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.0.x@1002227 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 96c9e1ebc7c..8bb961296f9 100644 --- a/CHANGES +++ b/CHANGES @@ -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 , Rainer Jung] *) SECURITY: CVE-2010-0434 (cve.mitre.org) Ensure each subrequest has a shallow copy of headers_in so that the diff --git a/STATUS b/STATUS index 5017f0ede72..5ebb1eb17a0 100644 --- a/STATUS +++ b/STATUS @@ -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 diff --git a/modules/ssl/ssl_engine_io.c b/modules/ssl/ssl_engine_io.c index 9d4a23b0d0a..c5fe6b8c276 100644 --- a/modules/ssl/ssl_engine_io.c +++ b/modules/ssl/ssl_engine_io.c @@ -1261,9 +1261,17 @@ static apr_status_t ssl_io_filter_input(ap_filter_t *f, } 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); } diff --git a/modules/ssl/ssl_engine_kernel.c b/modules/ssl/ssl_engine_kernel.c index ecf254ebe43..8aec2fdb317 100644 --- a/modules/ssl/ssl_engine_kernel.c +++ b/modules/ssl/ssl_engine_kernel.c @@ -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: "