Fix up ClientHello parser errors

Bob pointed out that the previous CL didn't quiiiite move the errors
around right. I looked at the ssl_parse_client_hello_with_trailing_data
calls but not the SSL_parse_client_hello calls. As a result, we doubled
up some errors.

I'd also missed that we already have SSL_R_CLIENTHELLO_PARSE_FAILED.
That said, which error to use is a little interesting. Some codepaths
used to use SSL_R_DECODE_ERROR and some used
SSL_R_CLIENTHELLO_PARSE_FAILED. Further complicating things is that some
ClientHello error paths are unreachable because only the first time a
ClientHello is parsed matters. But we have the second ClientHello in HRR
and inner ClientHellos to content with. (A TLS connection can have up to
four ClientHellos now!)

I've erred towards picking the more specific one, given this whole mess.

Update-Note: The error when the server cannot parse the ClientHello is
now a bit more specific. This might be visible to server-specific
logging, but will not change what is sent over the wire.

Change-Id: I64a4305968616a9f414d3c95fb4ffbd1cfdc4ecc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76147
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Commit-Queue: Bob Beck <[email protected]>

ssl/extensions.cc

@@ -132,7 +132,7 @@ bool ssl_parse_client_hello_with_trailing_data(const SSL *ssl, CBS *cbs,
       !CBS_get_bytes(cbs, &random, SSL3_RANDOM_SIZE) ||
       !CBS_get_u8_length_prefixed(cbs, &session_id) ||
       CBS_len(&session_id) > SSL_MAX_SSL_SESSION_ID_LENGTH) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
     return false;
   }
 
@@ -144,7 +144,7 @@ bool ssl_parse_client_hello_with_trailing_data(const SSL *ssl, CBS *cbs,
   if (SSL_is_dtls(out->ssl)) {
     CBS cookie;
     if (!CBS_get_u8_length_prefixed(cbs, &cookie)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+      OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
       return false;
     }
     out->dtls_cookie = CBS_data(&cookie);
@@ -159,7 +159,7 @@ bool ssl_parse_client_hello_with_trailing_data(const SSL *ssl, CBS *cbs,
       CBS_len(&cipher_suites) < 2 || (CBS_len(&cipher_suites) & 1) != 0 ||
       !CBS_get_u8_length_prefixed(cbs, &compression_methods) ||
       CBS_len(&compression_methods) < 1) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+      OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
     return false;
   }
 
@@ -178,7 +178,7 @@ bool ssl_parse_client_hello_with_trailing_data(const SSL *ssl, CBS *cbs,
     CBS extensions;
     if (!CBS_get_u16_length_prefixed(cbs, &extensions) ||
         !tls1_check_duplicate_extensions(&extensions)) {
-      OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+      OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
       return false;
     }
     out->extensions = CBS_data(&extensions);
@@ -4560,7 +4560,7 @@ int SSL_parse_client_hello(const SSL *ssl, SSL_CLIENT_HELLO *out,
     return 0;
   }
   if (CBS_len(&cbs) != 0) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+    OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
     return 0;
   }
   return 1;

ssl/handshake.cc

@@ -88,7 +88,6 @@ bool SSL_HANDSHAKE::GetClientHello(SSLMessage *out_msg,
 
   if (!SSL_parse_client_hello(ssl, out_client_hello, CBS_data(&out_msg->body),
                               CBS_len(&out_msg->body))) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     return false;
   }

ssl/handshake_server.cc

@@ -534,7 +534,6 @@ static enum ssl_hs_wait_t do_read_client_hello(SSL_HANDSHAKE *hs) {
   SSL_CLIENT_HELLO client_hello;
   if (!SSL_parse_client_hello(ssl, &client_hello, CBS_data(&msg.body),
                               CBS_len(&msg.body))) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     return ssl_hs_error;
   }

ssl/ssl_test.cc

@@ -10006,14 +10006,16 @@ TEST(SSLTest, ParseClientHello) {
     trailing_data.push_back(0);
     EXPECT_FALSE(SSL_parse_client_hello(
         ssl.get(), &client_hello, trailing_data.data(), trailing_data.size()));
-    EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_SSL, SSL_R_DECODE_ERROR));
+    EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_SSL,
+                            SSL_R_CLIENTHELLO_PARSE_FAILED));
     ERR_clear_error();
 
     // Other invalid inputs.
     static const uint8_t kInvalid[] = {'n', 'o', 'p', 'e'};
     EXPECT_FALSE(SSL_parse_client_hello(ssl.get(), &client_hello, kInvalid,
                                         sizeof(kInvalid)));
-    EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_SSL, SSL_R_DECODE_ERROR));
+    EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_SSL,
+                            SSL_R_CLIENTHELLO_PARSE_FAILED));
     ERR_clear_error();
   }
 }

ssl/test/runner/runner.go

@@ -8756,7 +8756,7 @@ func addExtensionTests() {
 					resumeSession: true,
 					shouldFail:    true,
 					// The maximum session ID length is 32.
-					expectedError: ":DECODE_ERROR:",
+					expectedError: ":CLIENTHELLO_PARSE_FAILED:",
 				})
 			}
 
@@ -15271,6 +15271,11 @@ func addTrailingMessageDataTests() {
 			t.test.expectedLocalError = ""
 		}
 
+		if t.messageType == typeClientHello {
+			// We have a different error for ClientHello parsing.
+			t.test.expectedError = ":CLIENTHELLO_PARSE_FAILED:"
+		}
+
 		if t.messageType == typeFinished {
 			// Bad Finished messages read as the verify data having
 			// the wrong length.
@@ -15425,7 +15430,7 @@ func addTLS13HandshakeTests() {
 			},
 		},
 		shouldFail:         true,
-		expectedError:      ":DECODE_ERROR:",
+		expectedError:      ":CLIENTHELLO_PARSE_FAILED:",
 		expectedLocalError: "remote error: error decoding message",
 	})
 	testCases = append(testCases, testCase{
@@ -15438,7 +15443,7 @@ func addTLS13HandshakeTests() {
 			},
 		},
 		shouldFail:         true,
-		expectedError:      ":DECODE_ERROR:",
+		expectedError:      ":CLIENTHELLO_PARSE_FAILED:",
 		expectedLocalError: "remote error: error decoding message",
 	})
 

ssl/tls13_server.cc

@@ -772,7 +772,6 @@ static enum ssl_hs_wait_t do_read_second_client_hello(SSL_HANDSHAKE *hs) {
   SSL_CLIENT_HELLO client_hello;
   if (!SSL_parse_client_hello(ssl, &client_hello, CBS_data(&msg.body),
                               CBS_len(&msg.body))) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     return ssl_hs_error;
   }