Rewrite legacy DTLS unexpected handshake message handling.

Rewrite the code that handles unexpected handshake messages in the legacy
DTLS stack. Parse the DTLS 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.

ok inoguchi@ tb@

1 files changed, 139 insertions, 83 deletions

diff --git a/lib/libssl/d1_pkt.c b/lib/libssl/d1_pkt.c
index f6f07052a1a..9072315e72f 100644
--- a/lib/libssl/d1_pkt.c
+++ b/lib/libssl/d1_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.120 2022/03/14 16:49:35 jsing Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.121 2022/03/18 18:00:54 jsing Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -486,116 +486,172 @@ static int
 dtls1_read_handshake_unexpected(SSL *s)
 {
 	SSL3_RECORD_INTERNAL *rr = &s->s3->rrec;
-	int i;
+	struct hm_header_st hs_msg_hdr;
+	CBS cbs;
+	int ret;
 
 	if (s->internal->in_handshake) {
 		SSLerror(s, ERR_R_INTERNAL_ERROR);
 		return -1;
 	}
 
-	/* If we are a client, check for an incoming 'Hello Request': */
-	if (!s->server && rr->type == SSL3_RT_HANDSHAKE &&
-	    rr->length >= DTLS1_HM_HEADER_LENGTH && rr->off == 0 &&
-	    rr->data[0] == SSL3_MT_HELLO_REQUEST &&
-	    s->session != NULL && s->session->cipher != NULL) {
-		struct hm_header_st msg_hdr;
-		CBS cbs;
-
-		CBS_init(&cbs, rr->data, rr->length);
-		if (!dtls1_get_message_header(&cbs, &msg_hdr))
-			return -1; /* XXX - probably should drop/continue. */
-		if (msg_hdr.msg_len != 0) {
+	if (rr->off != 0) {
+		SSLerror(s, ERR_R_INTERNAL_ERROR);
+		return -1;
+	}
+
+	/* Parse handshake message header. */
+	CBS_init(&cbs, rr->data, rr->length);
+	if (!dtls1_get_message_header(&cbs, &hs_msg_hdr))
+		return -1; /* XXX - probably should drop/continue. */
+
+	/* This may just be a stale retransmit. */
+	if (rr->epoch != tls12_record_layer_read_epoch(s->internal->rl)) {
+		rr->length = 0;
+		return 1;
+	}
+
+	if (hs_msg_hdr.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;
+		}
+
+		/* XXX - should also check frag offset/length. */
+		if (hs_msg_hdr.msg_len != 0) {
 			SSLerror(s, SSL_R_BAD_HELLO_REQUEST);
 			ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
 			return -1;
 		}
-		rr->length = 0;
 
-		/* no need to check sequence number on HELLO REQUEST messages */
-
-		ssl_msg_callback(s, 0, SSL3_RT_HANDSHAKE, rr->data, 4);
-
-		if (SSL_is_init_finished(s) &&
-		    !(s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS) &&
-		    !s->s3->renegotiate) {
-			s->d1->handshake_read_seq++;
-			s->internal->new_session = 1;
-			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);
-					}
-				}
-			}
-		}
-		/* we either finished a handshake or ignored the request,
-		 * now try again to obtain the (application) data we were asked for */
+		ssl_msg_callback(s, 0, SSL3_RT_HANDSHAKE, rr->data,
+		    DTLS1_HM_HEADER_LENGTH);
+
 		rr->length = 0;
-		return 1;
-	}
 
-	/* Unexpected handshake message (Client Hello, or protocol violation) */
-	if (rr->type == SSL3_RT_HANDSHAKE &&
-	    rr->length >= DTLS1_HM_HEADER_LENGTH && rr->off == 0 &&
-	    !s->internal->in_handshake) {
-		struct hm_header_st msg_hdr;
-		CBS cbs;
-
-		/* this may just be a stale retransmit */
-		CBS_init(&cbs, rr->data, rr->length);
-		if (!dtls1_get_message_header(&cbs, &msg_hdr))
-			return -1; /* XXX - this should probably drop/continue. */
-		if (rr->epoch != tls12_record_layer_read_epoch(s->internal->rl)) {
-			rr->length = 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;
-		}
 
-		/* If we are server, we may have a repeated FINISHED of the
-		 * client here, then retransmit our CCS and FINISHED.
+		/*
+		 * Ignore this message if we're currently handshaking,
+		 * renegotiation is already pending or renegotiation is disabled
+		 * via flags.
 		 */
-		if (msg_hdr.type == SSL3_MT_FINISHED) {
-			if (dtls1_check_timeout_num(s) < 0)
-				return -1;
+		if (!SSL_is_init_finished(s) || s->s3->renegotiate ||
+		    (s->s3->flags & SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS) != 0)
+			return 1;
 
-			dtls1_retransmit_buffered_messages(s);
-			rr->length = 0;
+		s->d1->handshake_read_seq++;
+
+		/* XXX - why is this set here but not in ssl3? */
+		s->internal->new_session = 1;
+
+		if (!ssl3_renegotiate(s))
+			return 1;
+		if (!ssl3_renegotiate_check(s))
 			return 1;
+
+	} else if (hs_msg_hdr.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;
+		}
+
+		/*
+		 * 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 (((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->internal->options & SSL_OP_NO_CLIENT_RENEGOTIATION) != 0) {
+			ssl3_send_alert(s, SSL3_AL_FATAL,
+			    SSL_AD_NO_RENEGOTIATION);
+			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);
+
+		if (s->session == NULL || s->session->cipher == NULL) {
+			SSLerror(s, ERR_R_INTERNAL_ERROR);
+			return -1;
 		}
 
-		if (!(s->internal->mode & SSL_MODE_AUTO_RETRY)) {
-			if (s->s3->rbuf.left == 0) {
-				ssl_force_want_read(s);
-				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;
 		}
+
+		s->s3->hs.state = SSL_ST_ACCEPT;
+		s->internal->renegotiate = 1;
+		s->internal->new_session = 1;
+
+	} else if (hs_msg_hdr.type == SSL3_MT_FINISHED && s->server) {
+		/*
+		 * If we are server, we may have a repeated FINISHED of the
+		 * client here, then retransmit our CCS and FINISHED.
+		 */
+		if (dtls1_check_timeout_num(s) < 0)
+			return -1;
+
+		/* XXX - should this be calling ssl_msg_callback()? */
+
+		dtls1_retransmit_buffered_messages(s);
+
 		rr->length = 0;
+
 		return 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;
+		}
+	}
+
+	/*
+	 * We either finished a handshake or ignored the request, now try again
+	 * to obtain the (application) data we were asked for.
+	 */
 	return 1;
 }