Move SCSV handling out of cipher list parsing.

It's odd that a function like ssl_bytes_to_cipher_list secretly has side
effects all over the place. This removes the need for the TLS 1.3 code
to re-query the version range, and it removes the requirement that the
RI extension be first.

Change-Id: Ic9af549db3aaa8880f3c591b8a13ba9ae91d6a46
Reviewed-on: https://boringssl-review.googlesource.com/10220
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

ssl/handshake_client.c

@@ -621,8 +621,6 @@ static int ssl_write_client_cipher_list(SSL *ssl, CBB *out,
     if (!CBB_add_u16(&child, SSL3_CK_SCSV & 0xffff)) {
       return 0;
     }
-    /* The renegotiation extension is required to be at index zero. */
-    ssl->s3->tmp.extensions.sent |= (1u << 0);
   }
 
   if ((ssl->mode & SSL_MODE_SEND_FALLBACK_SCSV) ||

ssl/handshake_server.c

@@ -530,6 +530,26 @@ end:
   return ret;
 }
 
+int ssl_client_cipher_list_contains_cipher(
+    const struct ssl_early_callback_ctx *client_hello, uint16_t id) {
+  CBS cipher_suites;
+  CBS_init(&cipher_suites, client_hello->cipher_suites,
+           client_hello->cipher_suites_len);
+
+  while (CBS_len(&cipher_suites) > 0) {
+    uint16_t got_id;
+    if (!CBS_get_u16(&cipher_suites, &got_id)) {
+      return 0;
+    }
+
+    if (got_id == id) {
+      return 1;
+    }
+  }
+
+  return 0;
+}
+
 static int ssl3_get_client_hello(SSL *ssl) {
   int al = SSL_AD_INTERNAL_ERROR, ret = -1;
   const SSL_CIPHER *c;
@@ -600,11 +620,22 @@ static int ssl3_get_client_hello(SSL *ssl) {
     if (version > max_version) {
       version = max_version;
     }
+
     if (version < min_version) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL);
       al = SSL_AD_PROTOCOL_VERSION;
       goto f_err;
     }
+
+    /* Handle FALLBACK_SCSV. */
+    if (ssl_client_cipher_list_contains_cipher(
+            &client_hello, SSL3_CK_FALLBACK_SCSV & 0xffff) &&
+        version < max_version) {
+      al = SSL3_AD_INAPPROPRIATE_FALLBACK;
+      OPENSSL_PUT_ERROR(SSL, SSL_R_INAPPROPRIATE_FALLBACK);
+      goto f_err;
+    }
+
     ssl->version = ssl->method->version_to_wire(version);
     ssl->s3->enc_method = ssl3_get_enc_method(version);
     assert(ssl->s3->enc_method != NULL);
@@ -698,7 +729,7 @@ static int ssl3_get_client_hello(SSL *ssl) {
     goto f_err;
   }
 
-  ciphers = ssl_parse_client_cipher_list(ssl, &client_hello, max_version);
+  ciphers = ssl_parse_client_cipher_list(&client_hello);
   if (ciphers == NULL) {
     goto err;
   }

ssl/internal.h

@@ -967,9 +967,10 @@ int ssl_early_callback_get_extension(const struct ssl_early_callback_ctx *ctx,
                                      CBS *out, uint16_t extension_type);
 
 STACK_OF(SSL_CIPHER) *
-    ssl_parse_client_cipher_list(SSL *ssl,
+    ssl_parse_client_cipher_list(const struct ssl_early_callback_ctx *ctx);
-                                 const struct ssl_early_callback_ctx *ctx,
+
-                                 uint16_t max_version);
+int ssl_client_cipher_list_contains_cipher(
+    const struct ssl_early_callback_ctx *client_hello, uint16_t id);
 
 
 /* Underdocumented functions.

ssl/ssl_lib.c

@@ -1630,14 +1630,10 @@ int SSL_set_cipher_list(SSL *ssl, const char *str) {
 }
 
 STACK_OF(SSL_CIPHER) *
-    ssl_parse_client_cipher_list(SSL *ssl,
+    ssl_parse_client_cipher_list(const struct ssl_early_callback_ctx *ctx) {
-                                 const struct ssl_early_callback_ctx *ctx,
-                                 uint16_t max_version) {
   CBS cipher_suites;
   CBS_init(&cipher_suites, ctx->cipher_suites, ctx->cipher_suites_len);
 
-  ssl->s3->send_connection_binding = 0;
-
   STACK_OF(SSL_CIPHER) *sk = sk_SSL_CIPHER_new_null();
   if (sk == NULL) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
@@ -1652,28 +1648,6 @@ STACK_OF(SSL_CIPHER) *
       goto err;
     }
 
-    /* Check for SCSV. */
-    if (ssl->s3 && cipher_suite == (SSL3_CK_SCSV & 0xffff)) {
-      /* SCSV is fatal if renegotiating. */
-      if (ssl->s3->initial_handshake_complete) {
-        OPENSSL_PUT_ERROR(SSL, SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING);
-        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
-        goto err;
-      }
-      ssl->s3->send_connection_binding = 1;
-      continue;
-    }
-
-    /* Check for FALLBACK_SCSV. */
-    if (ssl->s3 && cipher_suite == (SSL3_CK_FALLBACK_SCSV & 0xffff)) {
-      if (ssl3_protocol_version(ssl) < max_version) {
-        OPENSSL_PUT_ERROR(SSL, SSL_R_INAPPROPRIATE_FALLBACK);
-        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL3_AD_INAPPROPRIATE_FALLBACK);
-        goto err;
-      }
-      continue;
-    }
-
     const SSL_CIPHER *c = SSL_get_cipher_by_value(cipher_suite);
     if (c != NULL && !sk_SSL_CIPHER_push(sk, c)) {
       OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);

ssl/t1_lib.c

@@ -893,25 +893,11 @@ static int ext_ri_parse_clienthello(SSL *ssl, uint8_t *out_alert,
     return 1;
   }
 
-  CBS fake_contents;
-  static const uint8_t kFakeExtension[] = {0};
-
   if (contents == NULL) {
-    if (ssl->s3->send_connection_binding) {
+    return 1;
-      /* The renegotiation SCSV was received so pretend that we received a
-       * renegotiation extension. */
-      CBS_init(&fake_contents, kFakeExtension, sizeof(kFakeExtension));
-      contents = &fake_contents;
-      /* We require that the renegotiation extension is at index zero of
-       * kExtensions. */
-      ssl->s3->tmp.extensions.received |= (1u << 0);
-    } else {
-      return 1;
-    }
   }
 
   CBS renegotiated_connection;
-
   if (!CBS_get_u8_length_prefixed(contents, &renegotiated_connection) ||
       CBS_len(contents) != 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_RENEGOTIATION_ENCODING_ERR);
@@ -2236,9 +2222,6 @@ static int ext_supported_groups_add_serverhello(SSL *ssl, CBB *out) {
 /* kExtensions contains all the supported extensions. */
 static const struct tls_extension kExtensions[] = {
   {
-    /* The renegotiation extension must always be at index zero because the
-     * |received| and |sent| bitsets need to be tweaked when the "extension" is
-     * sent as an SCSV. */
     TLSEXT_TYPE_renegotiate,
     NULL,
     ext_ri_add_clienthello,
@@ -2513,8 +2496,7 @@ err:
 static int ssl_scan_clienthello_tlsext(
     SSL *ssl, const struct ssl_early_callback_ctx *client_hello,
     int *out_alert) {
-  size_t i;
+  for (size_t i = 0; i < kNumExtensions; i++) {
-  for (i = 0; i < kNumExtensions; i++) {
     if (kExtensions[i].init != NULL) {
       kExtensions[i].init(ssl);
     }
@@ -2522,10 +2504,6 @@ static int ssl_scan_clienthello_tlsext(
 
   ssl->s3->tmp.extensions.received = 0;
   ssl->s3->tmp.custom_extensions.received = 0;
-  /* The renegotiation extension must always be at index zero because the
-   * |received| and |sent| bitsets need to be tweaked when the "extension" is
-   * sent as an SCSV. */
-  assert(kExtensions[0].value == TLSEXT_TYPE_renegotiate);
 
   CBS extensions;
   CBS_init(&extensions, client_hello->extensions, client_hello->extensions_len);
@@ -2568,17 +2546,32 @@ static int ssl_scan_clienthello_tlsext(
     }
   }
 
-  for (i = 0; i < kNumExtensions; i++) {
+  for (size_t i = 0; i < kNumExtensions; i++) {
-    if (!(ssl->s3->tmp.extensions.received & (1u << i))) {
+    if (ssl->s3->tmp.extensions.received & (1u << i)) {
-      /* Extension wasn't observed so call the callback with a NULL
+      continue;
-       * parameter. */
+    }
-      uint8_t alert = SSL_AD_DECODE_ERROR;
+
-      if (!kExtensions[i].parse_clienthello(ssl, &alert, NULL)) {
+    CBS *contents = NULL, fake_contents;
-        OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION);
+    static const uint8_t kFakeRenegotiateExtension[] = {0};
-        ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
+    if (kExtensions[i].value == TLSEXT_TYPE_renegotiate &&
-        *out_alert = alert;
+        ssl_client_cipher_list_contains_cipher(client_hello,
-        return 0;
+                                               SSL3_CK_SCSV & 0xffff)) {
-      }
+      /* The renegotiation SCSV was received so pretend that we received a
+       * renegotiation extension. */
+      CBS_init(&fake_contents, kFakeRenegotiateExtension,
+               sizeof(kFakeRenegotiateExtension));
+      contents = &fake_contents;
+      ssl->s3->tmp.extensions.received |= (1u << i);
+    }
+
+    /* Extension wasn't observed so call the callback with a NULL
+     * parameter. */
+    uint8_t alert = SSL_AD_DECODE_ERROR;
+    if (!kExtensions[i].parse_clienthello(ssl, &alert, contents)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION);
+      ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
+      *out_alert = alert;
+      return 0;
     }
   }
 
@@ -2640,8 +2633,10 @@ static int ssl_scan_serverhello_tlsext(SSL *ssl, CBS *cbs, int *out_alert) {
       continue;
     }
 
-    if (!(ssl->s3->tmp.extensions.sent & (1u << ext_index))) {
+    if (!(ssl->s3->tmp.extensions.sent & (1u << ext_index)) &&
-      /* If the extension was never sent then it is illegal. */
+        type != TLSEXT_TYPE_renegotiate) {
+      /* If the extension was never sent then it is illegal, except for the
+       * renegotiation extension which, in SSL 3.0, is signaled via SCSV. */
       OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION);
       ERR_add_error_dataf("extension :%u", (unsigned)type);
       *out_alert = SSL_AD_UNSUPPORTED_EXTENSION;

ssl/tls13_server.c

@@ -166,14 +166,7 @@ static enum ssl_hs_wait_t do_process_client_hello(SSL *ssl, SSL_HANDSHAKE *hs) {
     }
   }
 
-  uint16_t min_version, max_version;
+  STACK_OF(SSL_CIPHER) *ciphers = ssl_parse_client_cipher_list(&client_hello);
-  if (!ssl_get_version_range(ssl, &min_version, &max_version)) {
-    ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
-    return ssl_hs_error;
-  }
-
-  STACK_OF(SSL_CIPHER) *ciphers =
-      ssl_parse_client_cipher_list(ssl, &client_hello, max_version);
   if (ciphers == NULL) {
     return ssl_hs_error;
   }