From 93de5e5c11ae343b35ad06f029f220e03200953f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 17 Apr 2015 22:33:25 -0400 Subject: [PATCH] Reject empty cipher suite lists early. See upstream's 3ae91cfb327c9ed689b9aaf7bca01a3f5a0657cb. I misread that code and thought it was allowing empty cipher suites when there *is* a session ID, but it was allowing them when there isn't. Which doesn't make much sense because it'll get rejected later anyway. (Verified by toying with handshake_client.go.) Change-Id: Ia870a1518bca36fce6f3018892254f53ab49f460 Reviewed-on: https://boringssl-review.googlesource.com/4401 Reviewed-by: Adam Langley --- ssl/s3_srvr.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 5a547c06..a25775f3 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -1093,6 +1093,8 @@ int ssl3_get_client_hello(SSL *s) { } if (!CBS_get_u16_length_prefixed(&client_hello, &cipher_suites) || + CBS_len(&cipher_suites) == 0 || + CBS_len(&cipher_suites) % 2 != 0 || !CBS_get_u8_length_prefixed(&client_hello, &compression_methods) || CBS_len(&compression_methods) == 0) { al = SSL_AD_DECODE_ERROR; @@ -1100,25 +1102,13 @@ int ssl3_get_client_hello(SSL *s) { goto f_err; } - /* TODO(davidben): Per spec, cipher_suites can never be empty (specified at - * the ClientHello structure level). This logic allows it to be empty if - * resuming a session. Can we always require non-empty? If a client sends - * empty cipher_suites because it's resuming a session, it could always fail - * to resume a session, so it's unlikely to actually work. */ - if (CBS_len(&cipher_suites) == 0 && CBS_len(&session_id) != 0) { - /* We need a cipher if we are not resuming a session. */ - al = SSL_AD_ILLEGAL_PARAMETER; - OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_NO_CIPHERS_SPECIFIED); - goto f_err; - } - ciphers = ssl_bytes_to_cipher_list(s, &cipher_suites); if (ciphers == NULL) { goto err; } /* If it is a hit, check that the cipher is in the list. */ - if (s->hit && CBS_len(&cipher_suites) > 0) { + if (s->hit) { size_t j; int found_cipher = 0; uint32_t id = s->session->cipher->id;