Re: [PATCH] Disable SSL renegotiation (CVE-2009-3555).

[Maxim Dounin <mdounin@xxxxxxxxxx>]

Hello!

On Mon, Nov 09, 2009 at 03:20:03PM +0300, Maxim Dounin wrote:

> Hello!
> 
> On Mon, Nov 09, 2009 at 02:50:52PM +0300, Igor Sysoev wrote:
> 
> [...]
> 
> > > > > p.s. Для серверных соединений ещё теоретически остаётся проблема 
> > > > > Server Gated Certs (SGC) при работе со старыми браузерами с 
> > > > > экспортными ограничениям, но вот на это IMHO стоит забить.
> > > > 
> > > > Насколько я понимаю, SGC работает без renegotiation.
> > > 
> > > Если верить README.GlobalID из поставки mod_ssl - он renegotiation 
> > > использует при первом запросе, проделывая его сразу после 
> > > исходного handshake'а (если видит сертификат с нужным 
> > > extKeyUsage).  В последующих запросах renegotiation не 
> > > используется.
> > > 
> > > Я не смог под рукой найти ни одного браузера с экспортными 
> > > ограничениями - так что сам не проверял... :)
> > 
> > Полный renegotiation происходит только при Netscape International
> > Setp-Up. Во время SGC nginx увидит завершённый handshake только
> > уже после дополнительного renegotiation.
> 
> Ok, I see.  Т.е. всё даже лучше чем я предполагал.  Точно забить.

Updated patch, fixes null pointer dereference for SSLv2 
connections.

Maxim Dounin

# HG changeset patch
# User Maxim Dounin <mdounin@xxxxxxxxxx>
# Date 1257792777 -10800
# Node ID c3abcc1c95af1a564220aabb0bed575a7ded1e7d
# Parent  8da5668048b423cb58a77b9d496956e9cad96709
Disable SSL renegotiation (CVE-2009-3555).

It was recently discovered that SSL and TLS are vulnerable to
man-in-the-middle attacks due to renegotiation feature (see
http://extendedsubset.com/?p=8).

Most recent version of openssl (0.9.8l) disables renegotiation unless
explicitly enabled by application (not recommended though).  Since nginx
doesn't require renegotiation to work - try to disable it for older
openssl versions too.

Note that openssl (up to most recent 0.9.8l) doesn't handle disabled
renegotiation gracefully (i.e. goes to an unconsistent state when client
asks for renegotiation instead of sending back NO_RENEGOTIATION alert)
and we have to explicitly drop connection if we detect renegotiation
attempt.

diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -15,6 +15,8 @@ typedef struct {
 
 
 static int ngx_http_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store);
+static void ngx_ssl_info_callback(const ngx_ssl_conn_t *ssl_conn, int where,
+    int ret);
 static void ngx_ssl_handshake_handler(ngx_event_t *ev);
 static ngx_int_t ngx_ssl_handle_recv(ngx_connection_t *c, int n);
 static void ngx_ssl_write_handler(ngx_event_t *wev);
@@ -175,6 +177,8 @@ ngx_ssl_create(ngx_ssl_t *ssl, ngx_uint_
 
     SSL_CTX_set_read_ahead(ssl->ctx, 1);
 
+    SSL_CTX_set_info_callback(ssl->ctx, ngx_ssl_info_callback);
+
     return NGX_OK;
 }
 
@@ -349,6 +353,33 @@ ngx_http_ssl_verify_callback(int ok, X50
     return 1;
 }
 
+static void
+ngx_ssl_info_callback(const ngx_ssl_conn_t *ssl_conn, int where, int ret)
+{
+    ngx_connection_t  *c;
+
+    if (where & SSL_CB_HANDSHAKE_START && ssl_conn->server) {
+        c = ngx_ssl_get_connection((ngx_ssl_conn_t *) ssl_conn);
+
+        if (c->ssl->handshaked) {
+            ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0,
+                          "unexpected SSL renegotiation detected");
+
+            c->ssl->renegotiation = 1;
+
+            /*
+             * rearm disable flag as openssl (as of 0.9.8l at least) loses
+             * it due to bug; this doesn't really matter since we will close
+             * this connection anyway, but just to be sure
+             */
+
+            if (c->ssl->connection->s3) {
+                c->ssl->connection->s3->flags |=
+                    SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS;
+            }
+        }
+    }
+}
 
 ngx_int_t
 ngx_ssl_generate_rsa512_key(ngx_ssl_t *ssl)
@@ -587,6 +618,12 @@ ngx_ssl_handshake(ngx_connection_t *c)
         c->recv_chain = ngx_ssl_recv_chain;
         c->send_chain = ngx_ssl_send_chain;
 
+        /* initial handshake done, disable renegotiation (CVE-2009-3555) */
+
+        if (c->ssl->connection->server && c->ssl->connection->s3) {
+            c->ssl->connection->s3->flags |= SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS;
+        }
+
         return NGX_OK;
     }
 
@@ -789,6 +826,21 @@ ngx_ssl_handle_recv(ngx_connection_t *c,
     int        sslerr;
     ngx_err_t  err;
 
+    if (c->ssl->renegotiation) {
+        /*
+         * openssl (at least up to 0.9.8l) doesn't handle disabled
+         * renegotiation gracefully, so drop connection here
+         */
+
+        ngx_log_error(NGX_LOG_ALERT, c->log, 0,
+                      "unexpected SSL renegotiation");
+
+        c->ssl->no_wait_shutdown = 1;
+        c->ssl->no_send_shutdown = 1;
+
+        return NGX_ERROR;
+    }
+
     if (n > 0) {
 
         if (c->ssl->saved_write_handler) {
diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
--- a/src/event/ngx_event_openssl.h
+++ b/src/event/ngx_event_openssl.h
@@ -41,6 +41,7 @@ typedef struct {
     ngx_event_handler_pt        saved_write_handler;
 
     unsigned                    handshaked:1;
+    unsigned                    renegotiation:1;
     unsigned                    buffer:1;
     unsigned                    no_wait_shutdown:1;
     unsigned                    no_send_shutdown:1;