Make SSL_get_servername work in the early callback.

This avoids early callback users writing their own SNI parser and gives us a
place to surface the server name from ESNI in the future.

Update-Note: This isn't a breaking change, but users of
SSL_CTX_set_select_certificate_cb can likely drop a bit of code after this CL.

Bug: 275
Change-Id: I9685ae5cca8e0483de76229d12dac45ff8e9ec32
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/36784
Reviewed-by: Steven Valdez <[email protected]>
Commit-Queue: David Benjamin <[email protected]>

Author: davidben

include/openssl/ssl.h

@@ -3714,6 +3714,8 @@ OPENSSL_EXPORT int SSL_early_callback_ctx_extension_get(
 // high-level operation on |ssl| to be retried at a later time, which will
 // result in another call to |cb|.
 //
+// |SSL_get_servername| may be used during this callback.
+//
 // Note: The |SSL_CLIENT_HELLO| is only valid for the duration of the callback
 // and is not valid while the handshake is paused.
 OPENSSL_EXPORT void SSL_CTX_set_select_certificate_cb(

ssl/handshake_server.cc

@@ -503,6 +503,54 @@ static bool is_probably_jdk11_with_tls13(const SSL_CLIENT_HELLO *client_hello) {
   return true;
 }
 
+static bool extract_sni(SSL_HANDSHAKE *hs, uint8_t *out_alert,
+                        const SSL_CLIENT_HELLO *client_hello) {
+  SSL *const ssl = hs->ssl;
+  CBS sni;
+  if (!ssl_client_hello_get_extension(client_hello, &sni,
+                                      TLSEXT_TYPE_server_name)) {
+    // No SNI extension to parse.
+    return true;
+  }
+
+  CBS server_name_list, host_name;
+  uint8_t name_type;
+  if (!CBS_get_u16_length_prefixed(&sni, &server_name_list) ||
+      !CBS_get_u8(&server_name_list, &name_type) ||
+      // Although the server_name extension was intended to be extensible to
+      // new name types and multiple names, OpenSSL 1.0.x had a bug which meant
+      // different name types will cause an error. Further, RFC 4366 originally
+      // defined syntax inextensibly. RFC 6066 corrected this mistake, but
+      // adding new name types is no longer feasible.
+      //
+      // Act as if the extensibility does not exist to simplify parsing.
+      !CBS_get_u16_length_prefixed(&server_name_list, &host_name) ||
+      CBS_len(&server_name_list) != 0 ||
+      CBS_len(&sni) != 0) {
+    *out_alert = SSL_AD_DECODE_ERROR;
+    return false;
+  }
+
+  if (name_type != TLSEXT_NAMETYPE_host_name ||
+      CBS_len(&host_name) == 0 ||
+      CBS_len(&host_name) > TLSEXT_MAXLEN_host_name ||
+      CBS_contains_zero_byte(&host_name)) {
+    *out_alert = SSL_AD_UNRECOGNIZED_NAME;
+    return false;
+  }
+
+  // Copy the hostname as a string.
+  char *raw = nullptr;
+  if (!CBS_strdup(&host_name, &raw)) {
+    *out_alert = SSL_AD_INTERNAL_ERROR;
+    return false;
+  }
+  ssl->s3->hostname.reset(raw);
+
+  hs->should_ack_sni = true;
+  return true;
+}
+
 static enum ssl_hs_wait_t do_read_client_hello(SSL_HANDSHAKE *hs) {
   SSL *const ssl = hs->ssl;
 
@@ -526,6 +574,12 @@ static enum ssl_hs_wait_t do_read_client_hello(SSL_HANDSHAKE *hs) {
     return ssl_hs_handoff;
   }
 
+  uint8_t alert = SSL_AD_DECODE_ERROR;
+  if (!extract_sni(hs, &alert, &client_hello)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
+    return ssl_hs_error;
+  }
+
   // Run the early callback.
   if (ssl->ctx->select_certificate_cb != NULL) {
     switch (ssl->ctx->select_certificate_cb(&client_hello)) {
@@ -553,7 +607,6 @@ static enum ssl_hs_wait_t do_read_client_hello(SSL_HANDSHAKE *hs) {
     hs->apply_jdk11_workaround = true;
   }
 
-  uint8_t alert = SSL_AD_DECODE_ERROR;
   if (!negotiate_version(hs, &alert, &client_hello)) {
     ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
     return ssl_hs_error;

ssl/t1_lib.cc

@@ -633,45 +633,7 @@ static bool ext_sni_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t *out_alert,
 
 static bool ext_sni_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert,
                                       CBS *contents) {
-  SSL *const ssl = hs->ssl;
-  if (contents == NULL) {
-    return true;
-  }
-
-  CBS server_name_list, host_name;
-  uint8_t name_type;
-  if (!CBS_get_u16_length_prefixed(contents, &server_name_list) ||
-      !CBS_get_u8(&server_name_list, &name_type) ||
-      // Although the server_name extension was intended to be extensible to
-      // new name types and multiple names, OpenSSL 1.0.x had a bug which meant
-      // different name types will cause an error. Further, RFC 4366 originally
-      // defined syntax inextensibly. RFC 6066 corrected this mistake, but
-      // adding new name types is no longer feasible.
-      //
-      // Act as if the extensibility does not exist to simplify parsing.
-      !CBS_get_u16_length_prefixed(&server_name_list, &host_name) ||
-      CBS_len(&server_name_list) != 0 ||
-      CBS_len(contents) != 0) {
-    return false;
-  }
-
-  if (name_type != TLSEXT_NAMETYPE_host_name ||
-      CBS_len(&host_name) == 0 ||
-      CBS_len(&host_name) > TLSEXT_MAXLEN_host_name ||
-      CBS_contains_zero_byte(&host_name)) {
-    *out_alert = SSL_AD_UNRECOGNIZED_NAME;
-    return false;
-  }
-
-  // Copy the hostname as a string.
-  char *raw = nullptr;
-  if (!CBS_strdup(&host_name, &raw)) {
-    *out_alert = SSL_AD_INTERNAL_ERROR;
-    return false;
-  }
-  ssl->s3->hostname.reset(raw);
-
-  hs->should_ack_sni = true;
+  // SNI has already been parsed earlier in the handshake. See |extract_sni|.
   return true;
 }
 

ssl/test/test_config.cc

@@ -1102,34 +1102,15 @@ static enum ssl_select_cert_result_t SelectCertificateCallback(
   GetTestState(client_hello->ssl)->early_callback_called = true;
 
   if (!config->expect_server_name.empty()) {
-    const uint8_t *extension_data;
-    size_t extension_len;
-    CBS extension, server_name_list, host_name;
-    uint8_t name_type;
-
-    if (!SSL_early_callback_ctx_extension_get(
-            client_hello, TLSEXT_TYPE_server_name, &extension_data,
-            &extension_len)) {
-      fprintf(stderr, "Could not find server_name extension.\n");
-      return ssl_select_cert_error;
-    }
-
-    CBS_init(&extension, extension_data, extension_len);
-    if (!CBS_get_u16_length_prefixed(&extension, &server_name_list) ||
-        CBS_len(&extension) != 0 ||
-        !CBS_get_u8(&server_name_list, &name_type) ||
-        name_type != TLSEXT_NAMETYPE_host_name ||
-        !CBS_get_u16_length_prefixed(&server_name_list, &host_name) ||
-        CBS_len(&server_name_list) != 0) {
-      fprintf(stderr, "Could not decode server_name extension.\n");
+    const char *server_name =
+        SSL_get_servername(client_hello->ssl, TLSEXT_NAMETYPE_host_name);
+    if (server_name == nullptr ||
+        std::string(server_name) != config->expect_server_name) {
+      fprintf(stderr,
+              "Server name mismatch in early callback (got %s; want %s).\n",
+              server_name, config->expect_server_name.c_str());
       return ssl_select_cert_error;
     }
-
-    if (!CBS_mem_equal(&host_name,
-                       (const uint8_t *)config->expect_server_name.data(),
-                       config->expect_server_name.size())) {
-      fprintf(stderr, "Server name mismatch.\n");
-    }
   }
 
   if (config->fail_early_callback) {