Rewrite legacy TLS unexpected handshake message handling.

Rewrite the code that handles unexpected handshake messages in the legacy
TLS stack. Parse the TLS message header up front, then process it based on
the message type. Overall the code should be more strict and we should
reject various invalid messages that would have previously been accepted.

I also reviewed steve's experimental code and fixed the bug that it
contained.

ok inoguchi@ tb@

1 files changed, 114 insertions, 78 deletions

diff --git a/src/lib/libssl/ssl_pkt.c b/src/lib/libssl/ssl_pkt.c
index 4dc7f3b610..57230f8fae 100644
--- a/src/lib/libssl/ssl_pkt.c
+++ b/src/lib/libssl/ssl_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_pkt.c,v 1.56 2022/03/14 16:49:35 jsing Exp $ */
+/* $OpenBSD: ssl_pkt.c,v 1.57 2022/03/17 17:28:08 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -818,7 +818,10 @@ static int
 ssl3_read_handshake_unexpected(SSL *s)
 {
         SSL3_RECORD_INTERNAL *rr = &s->s3->rrec;
-        int i;
+        uint32_t hs_msg_length;
+        uint8_t hs_msg_type;
+        CBS cbs;
+        int ret;
 
         /*
          * We need four bytes of handshake data so we have a handshake message
@@ -846,99 +849,132 @@ ssl3_read_handshake_unexpected(SSL *s)
          * belongs in the client/server handshake code.
          */
 
-        /* If we are a client, check for an incoming 'Hello Request': */
-        if ((!s->server) && (s->s3->handshake_fragment_len >= 4) &&
-            (s->s3->handshake_fragment[0] == SSL3_MT_HELLO_REQUEST) &&
-            (s->session != NULL) && (s->session->cipher != NULL)) {
-                s->s3->handshake_fragment_len = 0;
+        /* Parse handshake message header. */
+        CBS_init(&cbs, s->s3->handshake_fragment, s->s3->handshake_fragment_len);
+        if (!CBS_get_u8(&cbs, &hs_msg_type))
+                return -1;
+        if (!CBS_get_u24(&cbs, &hs_msg_length))
+                return -1;
+
+        if (hs_msg_type == SSL3_MT_HELLO_REQUEST) {
+                /*
+                 * Incoming HelloRequest messages should only be received by a
+                 * client. A server may send these at any time - a client should
+                 * ignore the message if received in the middle of a handshake.
+                 * See RFC 5246 sections 7.4 and 7.4.1.1.
+                 */
+                if (s->server) {
+                        SSLerror(s, SSL_R_UNEXPECTED_MESSAGE);
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                             SSL_AD_UNEXPECTED_MESSAGE);
+                        return -1;
+                }
 
-                if ((s->s3->handshake_fragment[1] != 0) ||
-                    (s->s3->handshake_fragment[2] != 0) ||
-                    (s->s3->handshake_fragment[3] != 0)) {
+                if (hs_msg_length != 0) {
                         SSLerror(s, SSL_R_BAD_HELLO_REQUEST);
                         ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
                         return -1;
                 }
 
                 ssl_msg_callback(s, 0, SSL3_RT_HANDSHAKE,
-                    s->s3->handshake_fragment, 4);
-
-                if (SSL_is_init_finished(s) &&
-                    !(s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS) &&
-                    !s->s3->renegotiate) {
-                        ssl3_renegotiate(s);
-                        if (ssl3_renegotiate_check(s)) {
-                                i = s->internal->handshake_func(s);
-                                if (i < 0)
-                                        return (i);
-                                if (i == 0) {
-                                        SSLerror(s, SSL_R_SSL_HANDSHAKE_FAILURE);
-                                        return (-1);
-                                }
-
-                                if (!(s->internal->mode & SSL_MODE_AUTO_RETRY)) {
-                                        if (s->s3->rbuf.left == 0) {
-                                                ssl_force_want_read(s);
-                                                return (-1);
-                                        }
-                                }
-                        }
+                    s->s3->handshake_fragment, s->s3->handshake_fragment_len);
+
+                s->s3->handshake_fragment_len = 0;
+
+                /*
+                 * It should be impossible to hit this, but keep the safety
+                 * harness for now...
+                 */
+                if (s->session == NULL || s->session->cipher == NULL)
+                        return 1;
+
+                /*
+                 * Ignore this message if we're currently handshaking,
+                 * renegotiation is already pending or renegotiation is disabled
+                 * via flags.
+                 */
+                if (!SSL_is_init_finished(s) || s->s3->renegotiate ||
+                    (s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS) != 0)
+                        return 1;
+
+                if (!ssl3_renegotiate(s))
+                        return 1;
+                if (!ssl3_renegotiate_check(s))
+                        return 1;
+
+        } else if (hs_msg_type == SSL3_MT_CLIENT_HELLO) {
+                /*
+                 * Incoming ClientHello messages should only be received by a
+                 * server. A client may send these in response to server
+                 * initiated renegotiation (HelloRequest) or in order to
+                 * initiate renegotiation by the client. See RFC 5246 section
+                 * 7.4.1.2.
+                 */
+                if (!s->server) {
+                        SSLerror(s, SSL_R_UNEXPECTED_MESSAGE);
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                             SSL_AD_UNEXPECTED_MESSAGE);
+                        return -1;
                 }
-                /* we either finished a handshake or ignored the request,
-                 * now try again to obtain the (application) data we were asked for */
-                return 1;
-        }
 
-        /* Disallow client initiated renegotiation if configured. */
-        if (s->server && SSL_is_init_finished(s) &&
-            s->s3->handshake_fragment_len >= 4 &&
-            s->s3->handshake_fragment[0] == SSL3_MT_CLIENT_HELLO &&
-            (s->internal->options & SSL_OP_NO_CLIENT_RENEGOTIATION)) {
-                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_NO_RENEGOTIATION);
-                return -1;
-        }
+                /*
+                 * A client should not be sending a ClientHello unless we're not
+                 * currently handshaking.
+                 */
+                if (!SSL_is_init_finished(s)) {
+                        SSLerror(s, SSL_R_UNEXPECTED_MESSAGE);
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                            SSL_AD_UNEXPECTED_MESSAGE);
+                        return -1;
+                }
 
-        /* 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)
-         */
-        if (s->server &&
-            SSL_is_init_finished(s) &&
-            !s->s3->send_connection_binding &&
-            (s->s3->handshake_fragment_len >= 4) &&
-            (s->s3->handshake_fragment[0] == SSL3_MT_CLIENT_HELLO) &&
-            (s->session != NULL) && (s->session->cipher != NULL)) {
-                /*s->s3->handshake_fragment_len = 0;*/
-                rr->length = 0;
-                ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
-                return 1;
-        }
+                if ((s->internal->options & SSL_OP_NO_CLIENT_RENEGOTIATION) != 0) {
+                        ssl3_send_alert(s, SSL3_AL_FATAL,
+                            SSL_AD_NO_RENEGOTIATION);
+                        return -1;
+                }
 
-        /* Unexpected handshake message (Client Hello, or protocol violation) */
-        if ((s->s3->handshake_fragment_len >= 4) && !s->internal->in_handshake) {
-                if (((s->s3->hs.state&SSL_ST_MASK) == SSL_ST_OK) &&
-                    !(s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS)) {
-                        s->s3->hs.state = s->server ? SSL_ST_ACCEPT : SSL_ST_CONNECT;
-                        s->internal->renegotiate = 1;
-                        s->internal->new_session = 1;
+                if (s->session == NULL || s->session->cipher == NULL) {
+                        SSLerror(s, ERR_R_INTERNAL_ERROR);
+                        return -1;
                 }
-                i = s->internal->handshake_func(s);
-                if (i < 0)
-                        return (i);
-                if (i == 0) {
-                        SSLerror(s, SSL_R_SSL_HANDSHAKE_FAILURE);
-                        return (-1);
+
+                /* Client requested renegotiation but it is not permitted. */
+                if (!s->s3->send_connection_binding ||
+                    (s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS) != 0) {
+                        ssl3_send_alert(s, SSL3_AL_WARNING,
+                            SSL_AD_NO_RENEGOTIATION);
+                        return 1;
                 }
 
-                if (!(s->internal->mode & SSL_MODE_AUTO_RETRY)) {
-                        if (s->s3->rbuf.left == 0) {
-                                ssl_force_want_read(s);
-                                return (-1);
-                        }
+                s->s3->hs.state = SSL_ST_ACCEPT;
+                s->internal->renegotiate = 1;
+                s->internal->new_session = 1;
+
+        } else {
+                SSLerror(s, SSL_R_UNEXPECTED_MESSAGE);
+                ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+                return -1;
+        }
+
+        if ((ret = s->internal->handshake_func(s)) < 0)
+                return ret;
+        if (ret == 0) {
+                SSLerror(s, SSL_R_SSL_HANDSHAKE_FAILURE);
+                return -1;
+        }
+
+        if (!(s->internal->mode & SSL_MODE_AUTO_RETRY)) {
+                if (s->s3->rbuf.left == 0) {
+                        ssl_force_want_read(s);
+                        return -1;
                 }
-                return 1;
         }
 
+        /*
+         * We either finished a handshake or ignored the request, now try again
+         * to obtain the (application) data we were asked for.
+         */
         return 1;
 }