Make sure we check an incoming reneg ClientHello in DTLS

In TLS we have a check to make sure an incoming reneg ClientHello is
acceptable. The equivalent check is missing in the DTLS code. This means
that if a client does not signal the ability to handle secure reneg in the
initial handshake, then a subsequent reneg handshake should be rejected by
the server. In the DTLS case the reneg was being allowed if the the 2nd
ClientHello had a renegotiation_info extension. This is incorrect.

While incorrect, this does not represent a security issue because if
the renegotiation_info extension is present in the second ClientHello it
also has to be *correct*. Therefore this will only work if both the client
and server believe they are renegotiating, and both know the previous
Finished result. This is not the case in an insecure rengotiation attack.

I have also tidied up the check in the TLS code and given a better check
for determining whether we are renegotiating or not.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5192)

diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 685d50a4f10ed9460228f82379d42134b5da618f..4e92e493fb4b598a609eeba6772331f507c45227 100644 (file)
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -1205,6 +1205,24 @@ int dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
         goto start;
     }
 
+    /*
+     * If we are a server and get a client hello when renegotiation isn't
+     * allowed send back a no renegotiation alert and carry on.
+     */
+    if (s->server
+            && SSL_is_init_finished(s)
+            && !s->s3->send_connection_binding
+            && s->d1->handshake_fragment_len >= DTLS1_HM_HEADER_LENGTH
+            && s->d1->handshake_fragment[0] == SSL3_MT_CLIENT_HELLO
+            && s->s3->previous_client_finished_len != 0
+            && (s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION) == 0) {
+        s->d1->handshake_fragment_len = 0;
+        rr->length = 0;
+        ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
+        goto start;
+    }
+
+
     if (s->d1->alert_fragment_len >= DTLS1_AL_HEADER_LENGTH) {
         int alert_level = s->d1->alert_fragment[0];
         int alert_descr = s->d1->alert_fragment[1];

diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index b2373dc0f547fb4293d3a7948b118382ecf6ccd3..d74a91d668528f3bbfffdf7da28a366773ada56c 100644 (file)
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -1421,26 +1421,25 @@ int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek)
          */
         goto start;
     }
+
     /*
      * If we are a server and get a client hello when renegotiation isn't
-     * allowed send back a no renegotiation alert and carry on. WARNING:
-     * experimental code, needs reviewing (steve)
+     * allowed send back a no renegotiation alert and carry on.
      */
-    if (s->server &&
-        SSL_is_init_finished(s) &&
-        !s->s3->send_connection_binding &&
-        (s->version > SSL3_VERSION) &&
-        (s->s3->handshake_fragment_len >= 4) &&
-        (s->s3->handshake_fragment[0] == SSL3_MT_CLIENT_HELLO) &&
-        (s->session != NULL) && (s->session->cipher != NULL) &&
-        !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) {
-        /*
-         * s->s3->handshake_fragment_len = 0;
-         */
+    if (s->server
+            && SSL_is_init_finished(s)
+            && !s->s3->send_connection_binding
+            && s->version > SSL3_VERSION
+            && s->s3->handshake_fragment_len >= SSL3_HM_HEADER_LENGTH
+            && s->s3->handshake_fragment[0] == SSL3_MT_CLIENT_HELLO
+            && s->s3->previous_client_finished_len != 0
+            && (s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION) == 0) {
+        s->s3->handshake_fragment_len = 0;
         rr->length = 0;
         ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
         goto start;
     }
+
     if (s->s3->alert_fragment_len >= 2) {
         int alert_level = s->s3->alert_fragment[0];
         int alert_descr = s->s3->alert_fragment[1];