Slightly simplify V2ClientHello sniffing.
Rather than sniff for ClientHello, just fall through to standard logic
once weird cases are resolved.
This means that garbage will now read as WRONG_VERSION rather than
UNKNOWN_PROTOCOL, but the rules here were slightly odd anyway. This also
means we'll now accept empty records before the ClientHello (up to the
empty record limit), and process records of the wrong type with the
usual codepath during the handshake.
This shouldn't be any more risk as it just makes the ClientHello more
consistent with the rest of the protocol. A TLS implementation that
doesn't parse V2ClientHello would do the same unless it still
special-cased the first record. All newly-exposed states are reachable
by fragmenting ClientHello by one byte and then sending the record in
question.
BUG=468889
Change-Id: Ib701ae5d8adb663e158c391639b232a9d9cd1c6e
Reviewed-on: https://boringssl-review.googlesource.com/5712
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index e45251b..e2ae719 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -617,8 +617,8 @@
int ret;
const uint8_t *p;
- /* Read the first 8 bytes. To recognize a ClientHello or V2ClientHello only
- * needs the first 6 bytes, but 8 is needed to recognize CONNECT below. */
+ /* Read the first 8 bytes. To recognize a V2ClientHello only needs the first 4
+ * bytes, but 8 is needed to recognize CONNECT below. */
ret = ssl3_read_sniff_buffer(s, INITIAL_SNIFF_BUFFER_SIZE);
if (ret <= 0) {
return ret;
@@ -641,38 +641,33 @@
return -1;
}
- /* Determine if this is a ClientHello or V2ClientHello. */
+ /* Determine if this is a V2ClientHello. */
if ((p[0] & 0x80) && p[2] == SSL2_MT_CLIENT_HELLO &&
p[3] >= SSL3_VERSION_MAJOR) {
/* This is a V2ClientHello. */
s->state = SSL3_ST_SR_V2_CLIENT_HELLO;
return 1;
}
- if (p[0] == SSL3_RT_HANDSHAKE && p[1] >= SSL3_VERSION_MAJOR &&
- p[5] == SSL3_MT_CLIENT_HELLO) {
- /* This is a ClientHello. Initialize the record layer with the already
- * consumed data and continue the handshake. */
- if (!ssl3_setup_read_buffer(s)) {
- return -1;
- }
- assert(s->rstate == SSL_ST_READ_HEADER);
- /* There cannot have already been data in the record layer. */
- assert(s->s3->rbuf.left == 0);
- memcpy(s->s3->rbuf.buf, p, s->s3->sniff_buffer_len);
- s->s3->rbuf.offset = 0;
- s->s3->rbuf.left = s->s3->sniff_buffer_len;
- s->packet_length = 0;
- BUF_MEM_free(s->s3->sniff_buffer);
- s->s3->sniff_buffer = NULL;
- s->s3->sniff_buffer_len = 0;
-
- s->state = SSL3_ST_SR_CLNT_HELLO_A;
- return 1;
+ /* Fall through to the standard logic. Initialize the record layer with the
+ * already consumed data and continue the handshake. */
+ if (!ssl3_setup_read_buffer(s)) {
+ return -1;
}
+ assert(s->rstate == SSL_ST_READ_HEADER);
+ /* There cannot have already been data in the record layer. */
+ assert(s->s3->rbuf.left == 0);
+ memcpy(s->s3->rbuf.buf, p, s->s3->sniff_buffer_len);
+ s->s3->rbuf.offset = 0;
+ s->s3->rbuf.left = s->s3->sniff_buffer_len;
+ s->packet_length = 0;
- OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_PROTOCOL);
- return -1;
+ BUF_MEM_free(s->s3->sniff_buffer);
+ s->s3->sniff_buffer = NULL;
+ s->s3->sniff_buffer_len = 0;
+
+ s->state = SSL3_ST_SR_CLNT_HELLO_A;
+ return 1;
}
int ssl3_get_v2_client_hello(SSL *s) {
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index b837ad0..be3a2b9 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -155,6 +155,10 @@
#include "../crypto/internal.h"
+/* |SSL_R_UKNOWN_PROTOCOL| is no longer emitted, but continue to define it
+ * to avoid downstream churn. */
+OPENSSL_DECLARE_ERROR_REASON(SSL, SSL_R_UKNOWN_PROTOCOL)
+
/* Some error codes are special. Ensure the make_errors.go script never
* regresses this. */
OPENSSL_COMPILE_ASSERT(SSL_R_TLSV1_ALERT_NO_RENEGOTIATION ==
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 2c01f15..49ada2a 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1180,7 +1180,7 @@
name: "Garbage",
sendPrefix: "blah",
shouldFail: true,
- expectedError: ":UNKNOWN_PROTOCOL:",
+ expectedError: ":WRONG_VERSION_NUMBER:",
},
{
name: "SkipCipherVersionCheck",
