From: jsing <jsing@openbsd.org>
Date: Fri, 24 Aug 2018 17:44:22 +0000 (+0000)
Subject: Pull up the parsing of a ClientHello.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=5a15326f4fac974563e5a799f0a34a31f8de6b54;p=openbsd

Pull up the parsing of a ClientHello.

Parse up until the extensions (if any), then proceed with processing,
rather than gradually parsing while processing. This makes the code
cleaner, requires messages to be valid before processing and makes way
for upcoming changes.

ok inoguchi@ tb@
---

diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c
index 745fd6d83ad..b9b2c58705b 100644
--- a/lib/libssl/ssl_srvr.c
+++ b/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.43 2018/08/24 17:30:32 jsing Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.44 2018/08/24 17:44:22 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -846,13 +846,26 @@ ssl3_get_client_hello(SSL *s)
 
 	CBS_init(&cbs, s->internal->init_msg, n);
 
+	/* Parse client hello up until the extensions (if any). */
+	if (!CBS_get_u16(&cbs, &client_version))
+		goto truncated;
+	if (!CBS_get_bytes(&cbs, &client_random, SSL3_RANDOM_SIZE))
+		goto truncated;
+	if (!CBS_get_u8_length_prefixed(&cbs, &session_id))
+		goto truncated;
+	if (SSL_IS_DTLS(s)) {
+		if (!CBS_get_u8_length_prefixed(&cbs, &cookie))
+			goto truncated;
+	}
+	if (!CBS_get_u16_length_prefixed(&cbs, &cipher_suites))
+		goto truncated;
+	if (!CBS_get_u8_length_prefixed(&cbs, &compression_methods))
+		goto truncated;
+
 	/*
 	 * Use version from inside client hello, not from record header.
 	 * (may differ: see RFC 2246, Appendix E, second paragraph)
 	 */
-	if (!CBS_get_u16(&cbs, &client_version))
-		goto truncated;
-
 	if (ssl_max_shared_version(s, client_version, &shared_version) != 1) {
 		SSLerror(s, SSL_R_WRONG_VERSION_NUMBER);
 		if ((s->client_version >> 8) == SSL3_VERSION_MAJOR &&
@@ -877,19 +890,12 @@ ssl3_get_client_hello(SSL *s)
 	}
 	s->method = method;
 
-	if (!CBS_get_bytes(&cbs, &client_random, SSL3_RANDOM_SIZE))
-		goto truncated;
-	if (!CBS_get_u8_length_prefixed(&cbs, &session_id))
-		goto truncated;
-
 	/*
-	 * If we require cookies (DTLS) and this ClientHello doesn't
-	 * contain one, just return since we do not want to
-	 * allocate any memory yet. So check cookie length...
+	 * If we require cookies (DTLS) and this ClientHello does not contain
+	 * one, just return since we do not want to allocate any memory yet.
+	 * So check cookie length...
 	 */
 	if (SSL_IS_DTLS(s)) {
-		if (!CBS_get_u8_length_prefixed(&cbs, &cookie))
-			goto truncated;
 		if (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) {
 			if (CBS_len(&cookie) == 0)
 				return (1);
@@ -979,9 +985,6 @@ ssl3_get_client_hello(SSL *s)
 		}
 	}
 
-	if (!CBS_get_u16_length_prefixed(&cbs, &cipher_suites))
-		goto truncated;
-
 	/* XXX - This logic seems wrong... */
 	if (CBS_len(&cipher_suites) == 0 && CBS_len(&session_id) != 0) {
 		/* we need a cipher if we are not resuming a session */
@@ -1020,9 +1023,6 @@ ssl3_get_client_hello(SSL *s)
 		}
 	}
 
-	if (!CBS_get_u8_length_prefixed(&cbs, &compression_methods))
-		goto truncated;
-
 	comp_null = 0;
 	while (CBS_len(&compression_methods) > 0) {
 		if (!CBS_get_u8(&compression_methods, &comp_method))