Hartmut.Keil at adnovum
Nov 20, 2009, 4:40 AM
Post #10 of 14
(1799 views)
Permalink
|
|
Re: handling request splicing in case of server initiated renegotiation CVE-2009-3555
[In reply to]
|
|
Joe Orton wrote:
> 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.
>
Ok, I agree with your approach, giving more information what happens.
(maybe having a trace with info would be enough, since it can occurr under
normal circumstances)
> (more general but which required half a day tracking down an obscure bug
> in the BIO/filters, also fixed below...)
yep, that fix is essential for the case here
>
> Testing on this version very welcome!
If have successfully tested the change with the following setup
(the one described in my initial mail):
o for the location /cert/* SSLVerifyClient require is configured
o the MTIM attacker is injecting one complete request that causes the
server to initiated the renegotiation.
And a second incomplete one for request splitting
The proposed change is dropping the second incomplete request. The file
ssldump.patched in the attachment shows the output of ssldump with the
change, the file ssldump.injected without.
Regards
Hartmut
>
> 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);
> }
>
--
AdNovum Informatik AG
Hartmut Keil, Senior Software Engineer
Dipl. Physiker
Roentgenstrasse 22, CH-8005 Zurich
mailto:hartmut.keil [at] adnovum
phone: +41 44 272 6111, fax: +41 44 272 6312
http://www.adnovum.ch
AdNovum Locations: Bern, Budapest, San Mateo, Zurich (HQ)
|
ssldump.injected
