httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dirk-Willem van Gulik <di...@webweaving.org>
Subject Re: TLS renegotiation attack, mod_ssl and OpenSSL
Date Fri, 06 Nov 2009 18:45:49 GMT
Joe Orton wrote:
> On Fri, Nov 06, 2009 at 12:00:06AM +0000, Joe Orton wrote:

>> Here is a very rough first hack (for discussion/testing purposes only!):
>
> A second hack, slightly less rough hack:

Lovely - and that works exactly as I'd expect; and does also 'dro' the 
connection with the openssl s_client 'R' re-negotiate forcing as well.

So I guess the one thing we need now is to double check with the OpenSSL 
folks if the basic concept of this patch covers all basis. I.e. really 
sees every possible renegotiate - regardless of what or from where 
initiated. I am a bit worried that OpenSSL may have to clean an 
abstraction layer perhaps.

To recap (and verify my understanding - we register an info callback and 
then track the connection stat:

+    SSL_CTX_set_info_callback(ctx, ssl_callback_Info);
...
+void ssl_callback_Info(MODSSL_INFO_CB_ARG_TYPE ssl, int where, int rc)
+    /* If the reneg state is to reject renegotiations, check the SSL
+     * state machine and move to ABORT if a Client Hello is being
+     * read. */
+    if ((where & SSL_CB_ACCEPT_LOOP) &&
+                 scr->reneg_state == RENEG_REJECT) {
+        int state = SSL_get_state(ssl);
+
+        if (state == SSL3_ST_SR_CLNT_HELLO_A
+            || state == SSL23_ST_SR_CLNT_HELLO_A) {
+            scr->reneg_state = RENEG_ABORT;
+            ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c,
+                          "rejecting client initiated renegotiation");
+        }
+    }
+    /* If the first handshake is complete, change state to reject any
+     * subsequent client-initated renegotiation. */
+    else if (where & SSL_CB_HANDSHAKE_DONE &&
+                       scr->reneg_state == RENEG_INIT) {
+        scr->reneg_state = RENEG_REJECT;
+    }

And we then check this state on every read/write in the bio read/write 
callbacks:

  static int bio_filter_out_write/read(BIO *bio, const char *in, int inl)
  {
      bio_filter_out_ctx_t *outctx = (bio_filter_out_ctx_t *)(bio->ptr);

+    /* Abort early if the client has initiated a renegotiation. */
+    if (outctx->filter_ctx->config->reneg_state == RENEG_ABORT) {
+        outctx->rc = APR_ECONNABORTED;
+        return -1;
+    }

So it owuld be useful to have confirmed from the OpenSSL experts that 
this gives is visibility of each and every stage change.

And then we're good.

Dw.



Index: ssl_private.h
===================================================================
--- ssl_private.h	(revision 832979)
+++ ssl_private.h	(working copy)
@@ -335,6 +335,20 @@
      int is_proxy;
      int disabled;
      int non_ssl_request;
+
+    /* Track the handshake/renegotiation state for the connection so
+     * that all client-initiated renegotiations can be rejected, as a
+     * partial fix for CVE-2009-3555. */
+    enum {
+        RENEG_INIT = 0, /* Before initial handshake */
+        RENEG_REJECT, /* After initial handshake; any client-initiated
+                       * renegotiation should be rejected */
+        RENEG_ALLOW, /* A server-initated renegotiation is taking
+                      * place (as dictated by configuration) */
+        RENEG_ABORT /* Renegotiation initiated by client, abort the
+                     * connection */
+    } reneg_state;
+
      server_rec *server;
  } SSLConnRec;

@@ -618,7 +632,7 @@
  int          ssl_callback_NewSessionCacheEntry(SSL *, SSL_SESSION *);
  SSL_SESSION *ssl_callback_GetSessionCacheEntry(SSL *, unsigned char *, 
int, int *);
  void         ssl_callback_DelSessionCacheEntry(SSL_CTX *, SSL_SESSION *);
-void         ssl_callback_LogTracingState(MODSSL_INFO_CB_ARG_TYPE, int, 
int);
+void         ssl_callback_Info(MODSSL_INFO_CB_ARG_TYPE, int, int);
  #ifndef OPENSSL_NO_TLSEXT
  int          ssl_callback_ServerNameIndication(SSL *, int *, 
modssl_ctx_t *);
  #endif
Index: ssl_engine_init.c
===================================================================
--- ssl_engine_init.c	(revision 832979)
+++ ssl_engine_init.c	(working copy)
@@ -520,10 +520,7 @@
      SSL_CTX_set_tmp_rsa_callback(ctx, ssl_callback_TmpRSA);
      SSL_CTX_set_tmp_dh_callback(ctx,  ssl_callback_TmpDH);

-    if (s->loglevel >= APLOG_DEBUG) {
-        /* this callback only logs if LogLevel >= info */
-        SSL_CTX_set_info_callback(ctx, ssl_callback_LogTracingState);
-    }
+    SSL_CTX_set_info_callback(ctx, ssl_callback_Info);
  }

  static void ssl_init_ctx_verify(server_rec *s,
Index: ssl_engine_io.c
===================================================================
--- ssl_engine_io.c	(revision 832979)
+++ ssl_engine_io.c	(working copy)
@@ -103,6 +103,7 @@
      ap_filter_t        *pInputFilter;
      ap_filter_t        *pOutputFilter;
      int                nobuffer; /* non-zero to prevent buffering */
+    SSLConnRec         *config;
  } ssl_filter_ctx_t;

  typedef struct {
@@ -193,7 +194,13 @@
  static int bio_filter_out_write(BIO *bio, const char *in, int inl)
  {
      bio_filter_out_ctx_t *outctx = (bio_filter_out_ctx_t *)(bio->ptr);
-
+
+    /* Abort early if the client has initiated a renegotiation. */
+    if (outctx->filter_ctx->config->reneg_state == RENEG_ABORT) {
+        outctx->rc = APR_ECONNABORTED;
+        return -1;
+    }
+
      /* when handshaking we'll have a small number of bytes.
       * max size SSL will pass us here is about 16k.
       * (16413 bytes to be exact)
@@ -476,6 +483,12 @@
      if (!in)
          return 0;

+    /* Abort early if the client has initiated a renegotiation. */
+    if (inctx->filter_ctx->config->reneg_state == RENEG_ABORT) {
+        inctx->rc = APR_ECONNABORTED;
+        return -1;
+    }
+
      /* In theory, OpenSSL should flush as necessary, but it is known
       * not to do so correctly in some cases; see PR 46952.
       *
@@ -1699,6 +1712,8 @@

      filter_ctx = apr_palloc(c->pool, sizeof(ssl_filter_ctx_t));

+    filter_ctx->config          = myConnConfig(c);
+
      filter_ctx->nobuffer        = 0;
      filter_ctx->pOutputFilter   = ap_add_output_filter(ssl_io_filter,
                                                         filter_ctx, r, c);
Index: ssl_engine_kernel.c
===================================================================
--- ssl_engine_kernel.c	(revision 832979)
+++ ssl_engine_kernel.c	(working copy)
@@ -733,6 +733,10 @@
                                         (unsigned char *)&id,
                                         sizeof(id));

+            /* Toggle the renegotiation state to allow the new
+             * handshake to proceed. */
+            sslconn->reneg_state = RENEG_ALLOW;
+
              SSL_renegotiate(ssl);
              SSL_do_handshake(ssl);

@@ -754,6 +758,8 @@
              SSL_set_state(ssl, SSL_ST_ACCEPT);
              SSL_do_handshake(ssl);

+            sslconn->reneg_state = RENEG_REJECT;
+
              if (SSL_get_state(ssl) != SSL_ST_OK) {
                  ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
                                "Re-negotiation handshake failed: "
@@ -1814,76 +1820,55 @@
      return;
  }

-/*
- * This callback function is executed while OpenSSL processes the
- * SSL handshake and does SSL record layer stuff. We use it to
- * trace OpenSSL's processing in out SSL logfile.
- */
-void ssl_callback_LogTracingState(MODSSL_INFO_CB_ARG_TYPE ssl, int 
where, int rc)
+/* Dump debugginfo trace to the log file. */
+static void log_tracing_state(MODSSL_INFO_CB_ARG_TYPE ssl, server_rec *s,
+                              conn_rec *c, int where, int rc)
  {
-    conn_rec *c;
-    server_rec *s;
-    SSLSrvConfigRec *sc;
-
      /*
-     * find corresponding server
+     * create the various trace messages
       */
-    if (!(c = (conn_rec *)SSL_get_app_data((SSL *)ssl))) {
-        return;
+    if (where & SSL_CB_HANDSHAKE_START) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
+                     "%s: Handshake: start", SSL_LIBRARY_NAME);
      }
-
-    s = mySrvFromConn(c);
-    if (!(sc = mySrvConfig(s))) {
-        return;
+    else if (where & SSL_CB_HANDSHAKE_DONE) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
+                     "%s: Handshake: done", SSL_LIBRARY_NAME);
      }
-
-    /*
-     * create the various trace messages
-     */
-    if (s->loglevel >= APLOG_DEBUG) {
-        if (where & SSL_CB_HANDSHAKE_START) {
+    else if (where & SSL_CB_LOOP) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
+                     "%s: Loop: %s",
+                     SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
+    }
+    else if (where & SSL_CB_READ) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
+                     "%s: Read: %s",
+                     SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
+    }
+    else if (where & SSL_CB_WRITE) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
+                     "%s: Write: %s",
+                     SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
+    }
+    else if (where & SSL_CB_ALERT) {
+        char *str = (where & SSL_CB_READ) ? "read" : "write";
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
+                     "%s: Alert: %s:%s:%s",
+                     SSL_LIBRARY_NAME, str,
+                     SSL_alert_type_string_long(rc),
+                     SSL_alert_desc_string_long(rc));
+    }
+    else if (where & SSL_CB_EXIT) {
+        if (rc == 0) {
              ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                         "%s: Handshake: start", SSL_LIBRARY_NAME);
-        }
-        else if (where & SSL_CB_HANDSHAKE_DONE) {
-            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                         "%s: Handshake: done", SSL_LIBRARY_NAME);
-        }
-        else if (where & SSL_CB_LOOP) {
-            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                         "%s: Loop: %s",
+                         "%s: Exit: failed in %s",
                           SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
          }
-        else if (where & SSL_CB_READ) {
+        else if (rc < 0) {
              ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                         "%s: Read: %s",
+                         "%s: Exit: error in %s",
                           SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
          }
-        else if (where & SSL_CB_WRITE) {
-            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                         "%s: Write: %s",
-                         SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
-        }
-        else if (where & SSL_CB_ALERT) {
-            char *str = (where & SSL_CB_READ) ? "read" : "write";
-            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                         "%s: Alert: %s:%s:%s",
-                         SSL_LIBRARY_NAME, str,
-                         SSL_alert_type_string_long(rc),
-                         SSL_alert_desc_string_long(rc));
-        }
-        else if (where & SSL_CB_EXIT) {
-            if (rc == 0) {
-                ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                             "%s: Exit: failed in %s",
-                             SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
-            }
-            else if (rc < 0) {
-                ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
-                             "%s: Exit: error in %s",
-                             SSL_LIBRARY_NAME, SSL_state_string_long(ssl));
-            }
-        }
      }

      /*
@@ -1903,6 +1888,60 @@
      }
  }

+/*
+ * This callback function is executed while OpenSSL processes the SSL
+ * handshake and does SSL record layer stuff.  It's used to trap
+ * client-initiated renegotiations, and for dumping everything to the
+ * log.
+ */
+void ssl_callback_Info(MODSSL_INFO_CB_ARG_TYPE ssl, int where, int rc)
+{
+    conn_rec *c;
+    server_rec *s;
+    SSLSrvConfigRec *sc;
+    SSLConnRec *scr;
+
+    /*
+     * find corresponding server
+     */
+    if (!(c = (conn_rec *)SSL_get_app_data((SSL *)ssl))) {
+        return;
+    }
+
+    scr = myConnConfig(c);
+    if (!scr) {
+        return;
+    }
+
+    /* If the reneg state is to reject renegotiations, check the SSL
+     * state machine and move to ABORT if a Client Hello is being
+     * read. */
+    if ((where & SSL_CB_ACCEPT_LOOP) && scr->reneg_state == RENEG_REJECT)
{
+        int state = SSL_get_state(ssl);
+
+        if (state == SSL3_ST_SR_CLNT_HELLO_A
+            || state == SSL23_ST_SR_CLNT_HELLO_A) {
+            scr->reneg_state = RENEG_ABORT;
+            ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c,
+                          "rejecting client initiated renegotiation");
+        }
+    }
+    /* If the first handshake is complete, change state to reject any
+     * subsequent client-initated renegotiation. */
+    else if (where & SSL_CB_HANDSHAKE_DONE && scr->reneg_state == 
RENEG_INIT) {
+        scr->reneg_state = RENEG_REJECT;
+    }
+
+    s = mySrvFromConn(c);
+    if (!(sc = mySrvConfig(s))) {
+        return;
+    }
+
+    if (s->loglevel >= APLOG_DEBUG) {
+        log_tracing_state(ssl, s, c, where, rc);
+    }
+}
+
  #ifndef OPENSSL_NO_TLSEXT
  /*
   * This callback function is executed when OpenSSL encounters an extended


Mime
View raw message