Make SNI per-connection, not per-session.
Right now we report the per-connection value during the handshake and
the per-session value after the handshake. This also trims our tickets
slightly by removing a largely unused field from SSL_SESSION.
Putting it on SSL_HANDSHAKE would be better, but sadly a number of
bindings-type APIs expose it after the handshake.
Change-Id: I6a1383f95da9b1b141b9d6adadc05ee1e458a326
Reviewed-on: https://boringssl-review.googlesource.com/20064
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 269bc8b..e917fb4 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2512,8 +2512,7 @@
// SSL_get_servername, for a server, returns the hostname supplied by the
// client or NULL if there was none. The |type| argument must be
-// |TLSEXT_NAMETYPE_host_name|. Note that the returned pointer points inside
-// |ssl| and is only valid until the next operation on |ssl|.
+// |TLSEXT_NAMETYPE_host_name|.
OPENSSL_EXPORT const char *SSL_get_servername(const SSL *ssl, const int type);
// SSL_get_servername_type, for a server, returns |TLSEXT_NAMETYPE_host_name|
@@ -4070,7 +4069,6 @@
// These are used to make removal of session-ids more efficient and to
// implement a maximum cache size.
SSL_SESSION *prev, *next;
- char *tlsext_hostname;
// RFC4507 info
uint8_t *tlsext_tick; // Session ticket
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index f196db0..10e618d 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -632,16 +632,6 @@
if (ssl->session == NULL) {
hs->new_session->cipher = hs->new_cipher;
- // On new sessions, stash the SNI value in the session.
- if (hs->hostname != NULL) {
- OPENSSL_free(hs->new_session->tlsext_hostname);
- hs->new_session->tlsext_hostname = BUF_strdup(hs->hostname.get());
- if (hs->new_session->tlsext_hostname == NULL) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
- return ssl_hs_error;
- }
- }
-
// Determine whether to request a client certificate.
hs->cert_request = !!(ssl->verify_mode & SSL_VERIFY_PEER);
// Only request a certificate if Channel ID isn't negotiated.
diff --git a/ssl/internal.h b/ssl/internal.h
index 19968ac..0e44de8 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1208,9 +1208,6 @@
uint8_t *certificate_types = nullptr;
size_t num_certificate_types = 0;
- // hostname, on the server, is the value of the SNI extension.
- UniquePtr<char> hostname;
-
// local_pubkey is the public key we are authenticating as.
UniquePtr<EVP_PKEY> local_pubkey;
@@ -1764,6 +1761,9 @@
uint8_t *alpn_selected;
size_t alpn_selected_len;
+ // hostname, on the server, is the value of the SNI extension.
+ char *hostname;
+
// For a server:
// If |tlsext_channel_id_valid| is true, then this contains the
// verified Channel ID from the client: a P256 point, (x,y), where
diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc
index fb705aa..dcf9559 100644
--- a/ssl/s3_lib.cc
+++ b/ssl/s3_lib.cc
@@ -208,6 +208,7 @@
ssl_handshake_free(ssl->s3->hs);
OPENSSL_free(ssl->s3->next_proto_negotiated);
OPENSSL_free(ssl->s3->alpn_selected);
+ OPENSSL_free(ssl->s3->hostname);
Delete(ssl->s3->aead_read_ctx);
Delete(ssl->s3->aead_write_ctx);
BUF_MEM_free(ssl->s3->pending_flight);
diff --git a/ssl/ssl_asn1.cc b/ssl/ssl_asn1.cc
index 73908db..7bcfdd7 100644
--- a/ssl/ssl_asn1.cc
+++ b/ssl/ssl_asn1.cc
@@ -119,8 +119,6 @@
// peer [3] Certificate OPTIONAL,
// sessionIDContext [4] OCTET STRING OPTIONAL,
// verifyResult [5] INTEGER OPTIONAL, -- one of X509_V_* codes
-// hostName [6] OCTET STRING OPTIONAL,
-// -- from server_name extension
// pskIdentity [8] OCTET STRING OPTIONAL,
// ticketLifetimeHint [9] INTEGER OPTIONAL, -- client-only
// ticket [10] OCTET STRING OPTIONAL, -- client-only
@@ -142,9 +140,11 @@
// }
//
// Note: historically this serialization has included other optional
-// fields. Their presence is currently treated as a parse error:
+// fields. Their presence is currently treated as a parse error, except for
+// hostName, which is ignored.
//
// keyArg [0] IMPLICIT OCTET STRING OPTIONAL,
+// hostName [6] OCTET STRING OPTIONAL,
// pskIdentityHint [7] OCTET STRING OPTIONAL,
// compressionMethod [11] OCTET STRING OPTIONAL,
// srpUsername [12] OCTET STRING OPTIONAL,
@@ -254,16 +254,6 @@
}
}
- if (in->tlsext_hostname) {
- if (!CBB_add_asn1(&session, &child, kHostNameTag) ||
- !CBB_add_asn1(&child, &child2, CBS_ASN1_OCTETSTRING) ||
- !CBB_add_bytes(&child2, (const uint8_t *)in->tlsext_hostname,
- strlen(in->tlsext_hostname))) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- return 0;
- }
- }
-
if (in->psk_identity) {
if (!CBB_add_asn1(&session, &child, kPSKIdentityTag) ||
!CBB_add_asn1(&child, &child2, CBS_ASN1_OCTETSTRING) ||
@@ -627,10 +617,19 @@
&session, ret->sid_ctx, &ret->sid_ctx_length, sizeof(ret->sid_ctx),
kSessionIDContextTag) ||
!SSL_SESSION_parse_long(&session, &ret->verify_result, kVerifyResultTag,
- X509_V_OK) ||
- !SSL_SESSION_parse_string(&session, &ret->tlsext_hostname,
- kHostNameTag) ||
- !SSL_SESSION_parse_string(&session, &ret->psk_identity,
+ X509_V_OK)) {
+ return nullptr;
+ }
+
+ // Skip the historical hostName field.
+ CBS unused_hostname;
+ if (!CBS_get_optional_asn1(&session, &unused_hostname, nullptr,
+ kHostNameTag)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION);
+ return nullptr;
+ }
+
+ if (!SSL_SESSION_parse_string(&session, &ret->psk_identity,
kPSKIdentityTag) ||
!SSL_SESSION_parse_u32(&session, &ret->tlsext_tick_lifetime_hint,
kTicketLifetimeHintTag, 0) ||
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 853994b..7a75776 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -1755,25 +1755,11 @@
return ssl->tlsext_hostname;
}
- // During the handshake, report the handshake value.
- if (ssl->s3->hs != NULL) {
- return ssl->s3->hs->hostname.get();
- }
-
- // SSL_get_servername may also be called after the handshake to look up the
- // SNI value.
- //
- // TODO(davidben): This is almost unused. Can we remove it?
- SSL_SESSION *session = SSL_get_session(ssl);
- if (session == NULL) {
- return NULL;
- }
- return session->tlsext_hostname;
+ return ssl->s3->hostname;
}
int SSL_get_servername_type(const SSL *ssl) {
- SSL_SESSION *session = SSL_get_session(ssl);
- if (session == NULL || session->tlsext_hostname == NULL) {
+ if (SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name) == NULL) {
return -1;
}
return TLSEXT_NAMETYPE_host_name;
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 32d3b35..c21c282 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -242,13 +242,6 @@
SHA256_DIGEST_LENGTH);
new_session->peer_sha256_valid = session->peer_sha256_valid;
- if (session->tlsext_hostname != NULL) {
- new_session->tlsext_hostname = BUF_strdup(session->tlsext_hostname);
- if (new_session->tlsext_hostname == NULL) {
- return nullptr;
- }
- }
-
new_session->peer_signature_algorithm = session->peer_signature_algorithm;
new_session->timeout = session->timeout;
@@ -889,7 +882,6 @@
OPENSSL_cleanse(session->session_id, sizeof(session->session_id));
sk_CRYPTO_BUFFER_pop_free(session->certs, CRYPTO_BUFFER_free);
session->x509_method->session_clear(session);
- OPENSSL_free(session->tlsext_hostname);
OPENSSL_free(session->tlsext_tick);
CRYPTO_BUFFER_free(session->signed_cert_timestamp_list);
CRYPTO_BUFFER_free(session->ocsp_response);
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index dd795a6..48e50ee 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -542,14 +542,14 @@
// filling in missing fields from |kOpenSSLSession|. This includes
// providing |peer_sha256|, so |peer| is not serialized.
static const char kCustomSession[] =
- "MIIBdgIBAQICAwMEAsAvBCAG5Q1ndq4Yfmbeo1zwLkNRKmCXGdNgWvGT3cskV0yQ"
+ "MIIBZAIBAQICAwMEAsAvBCAG5Q1ndq4Yfmbeo1zwLkNRKmCXGdNgWvGT3cskV0yQ"
"kAQwJlrlzkAWBOWiLj/jJ76D7l+UXoizP2KI2C7I2FccqMmIfFmmkUy32nIJ0mZH"
- "IWoJoQYCBFRDO46iBAICASykAwQBAqUDAgEUphAEDnd3dy5nb29nbGUuY29tqAcE"
- "BXdvcmxkqQUCAwGJwKqBpwSBpBwUQvoeOk0Kg36SYTcLEkXqKwOBfF9vE4KX0Nxe"
- "LwjcDTpsuh3qXEaZ992r1N38VDcyS6P7I6HBYN9BsNHM362zZnY27GpTw+Kwd751"
- "CLoXFPoaMOe57dbBpXoro6Pd3BTbf/Tzr88K06yEOTDKPNj3+inbMaVigtK4PLyP"
- "q+Topyzvx9USFgRvyuoxn0Hgb+R0A3j6SLRuyOdAi4gv7Y5oliynrSIEIAYGBgYG"
- "BgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGrgMEAQevAwQBBLADBAEF";
+ "IWoJoQYCBFRDO46iBAICASykAwQBAqUDAgEUqAcEBXdvcmxkqQUCAwGJwKqBpwSB"
+ "pBwUQvoeOk0Kg36SYTcLEkXqKwOBfF9vE4KX0NxeLwjcDTpsuh3qXEaZ992r1N38"
+ "VDcyS6P7I6HBYN9BsNHM362zZnY27GpTw+Kwd751CLoXFPoaMOe57dbBpXoro6Pd"
+ "3BTbf/Tzr88K06yEOTDKPNj3+inbMaVigtK4PLyPq+Topyzvx9USFgRvyuoxn0Hg"
+ "b+R0A3j6SLRuyOdAi4gv7Y5oliynrSIEIAYGBgYGBgYGBgYGBgYGBgYGBgYGBgYG"
+ "BgYGBgYGBgYGrgMEAQevAwQBBLADBAEF";
// kBoringSSLSession is a serialized SSL_SESSION generated from bssl client.
static const char kBoringSSLSession[] =
@@ -1508,10 +1508,15 @@
return true;
}
+struct ClientConfig {
+ SSL_SESSION *session = nullptr;
+ std::string servername;
+};
+
static bool ConnectClientAndServer(bssl::UniquePtr<SSL> *out_client,
bssl::UniquePtr<SSL> *out_server,
SSL_CTX *client_ctx, SSL_CTX *server_ctx,
- SSL_SESSION *session) {
+ const ClientConfig &config = ClientConfig()) {
bssl::UniquePtr<SSL> client(SSL_new(client_ctx)), server(SSL_new(server_ctx));
if (!client || !server) {
return false;
@@ -1519,7 +1524,13 @@
SSL_set_connect_state(client.get());
SSL_set_accept_state(server.get());
- SSL_set_session(client.get(), session);
+ if (config.session) {
+ SSL_set_session(client.get(), config.session);
+ }
+ if (!config.servername.empty() &&
+ !SSL_set_tlsext_host_name(client.get(), config.servername.c_str())) {
+ return false;
+ }
BIO *bio1, *bio2;
if (!BIO_new_bio_pair(&bio1, 0, &bio2, 0)) {
@@ -1573,9 +1584,9 @@
SSL_CTX_use_PrivateKey(ctx, key_.get());
}
- bool Connect() {
+ bool Connect(const ClientConfig &config = ClientConfig()) {
return ConnectClientAndServer(&client_, &server_, client_ctx_.get(),
- server_ctx_.get(), nullptr /* no session */);
+ server_ctx_.get(), config);
}
uint16_t version() const { return GetParam().version; }
@@ -1679,8 +1690,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(),
- nullptr /* no session */));
+ server_ctx.get()));
SSL_SESSION *session0 = SSL_get_session(client.get());
bssl::UniquePtr<SSL_SESSION> session1 =
@@ -2117,15 +2127,16 @@
return 1;
}
-static bssl::UniquePtr<SSL_SESSION> CreateClientSession(SSL_CTX *client_ctx,
- SSL_CTX *server_ctx) {
+static bssl::UniquePtr<SSL_SESSION> CreateClientSession(
+ SSL_CTX *client_ctx, SSL_CTX *server_ctx,
+ const ClientConfig &config = ClientConfig()) {
g_last_session = nullptr;
SSL_CTX_sess_set_new_cb(client_ctx, SaveLastSession);
// Connect client and server to get a session.
bssl::UniquePtr<SSL> client, server;
if (!ConnectClientAndServer(&client, &server, client_ctx, server_ctx,
- nullptr /* no session */)) {
+ config)) {
fprintf(stderr, "Failed to connect client and server.\n");
return nullptr;
}
@@ -2145,8 +2156,10 @@
static void ExpectSessionReused(SSL_CTX *client_ctx, SSL_CTX *server_ctx,
SSL_SESSION *session, bool want_reused) {
bssl::UniquePtr<SSL> client, server;
- EXPECT_TRUE(ConnectClientAndServer(&client, &server, client_ctx, server_ctx,
- session));
+ ClientConfig config;
+ config.session = session;
+ EXPECT_TRUE(
+ ConnectClientAndServer(&client, &server, client_ctx, server_ctx, config));
EXPECT_EQ(SSL_session_reused(client.get()), SSL_session_reused(server.get()));
@@ -2161,8 +2174,10 @@
SSL_CTX_sess_set_new_cb(client_ctx, SaveLastSession);
bssl::UniquePtr<SSL> client, server;
- if (!ConnectClientAndServer(&client, &server, client_ctx,
- server_ctx, session)) {
+ ClientConfig config;
+ config.session = session;
+ if (!ConnectClientAndServer(&client, &server, client_ctx, server_ctx,
+ config)) {
fprintf(stderr, "Failed to connect client and server.\n");
return nullptr;
}
@@ -2640,7 +2655,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(), nullptr));
+ server_ctx.get()));
EXPECT_EQ(TLS1_2_VERSION, SSL_version(client.get()));
}
@@ -3047,6 +3062,46 @@
}
}
+TEST_P(SSLVersionTest, GetServerName) {
+ // No extensions in SSL 3.0.
+ if (version() == SSL3_VERSION) {
+ return;
+ }
+
+ ClientConfig config;
+ config.servername = "host1";
+
+ SSL_CTX_set_tlsext_servername_callback(
+ server_ctx_.get(), [](SSL *ssl, int *out_alert, void *arg) -> int {
+ // During the handshake, |SSL_get_servername| must match |config|.
+ ClientConfig *config_p = reinterpret_cast<ClientConfig *>(arg);
+ EXPECT_STREQ(config_p->servername.c_str(),
+ SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name));
+ return SSL_TLSEXT_ERR_OK;
+ });
+ SSL_CTX_set_tlsext_servername_arg(server_ctx_.get(), &config);
+
+ ASSERT_TRUE(Connect(config));
+ // After the handshake, it must also be available.
+ EXPECT_STREQ(config.servername.c_str(),
+ SSL_get_servername(server_.get(), TLSEXT_NAMETYPE_host_name));
+
+ // Establish a session under host1.
+ SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
+ SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH);
+ bssl::UniquePtr<SSL_SESSION> session =
+ CreateClientSession(client_ctx_.get(), server_ctx_.get(), config);
+
+ // If the client resumes a session with a different name, |SSL_get_servername|
+ // must return the new name.
+ ASSERT_TRUE(session);
+ config.session = session.get();
+ config.servername = "host2";
+ ASSERT_TRUE(Connect(config));
+ EXPECT_STREQ(config.servername.c_str(),
+ SSL_get_servername(server_.get(), TLSEXT_NAMETYPE_host_name));
+}
+
TEST(SSLTest, AddChainCertHack) {
// Ensure that we don't accidently break the hack that we have in place to
// keep curl and serf happy when they use an |X509| even after transfering
@@ -3145,8 +3200,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(),
- nullptr /* no session */));
+ server_ctx.get()));
}
TEST(SSLTest, ClientCABuffers) {
@@ -3208,8 +3262,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(),
- nullptr /* no session */));
+ server_ctx.get()));
EXPECT_TRUE(cert_cb_called);
}
@@ -3487,16 +3540,14 @@
// way to enable SSL 3.0.
bssl::UniquePtr<SSL> client, server;
EXPECT_FALSE(ConnectClientAndServer(&client, &server, tls_ctx.get(),
- ssl3_ctx.get(),
- nullptr /* no session */));
+ ssl3_ctx.get()));
uint32_t err = ERR_get_error();
EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(err));
EXPECT_EQ(SSL_R_NO_SUPPORTED_VERSIONS_ENABLED, ERR_GET_REASON(err));
// Likewise for SSLv3_method clients.
EXPECT_FALSE(ConnectClientAndServer(&client, &server, ssl3_ctx.get(),
- tls_ctx.get(),
- nullptr /* no session */));
+ tls_ctx.get()));
err = ERR_get_error();
EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(err));
EXPECT_EQ(SSL_R_NO_SUPPORTED_VERSIONS_ENABLED, ERR_GET_REASON(err));
@@ -3560,8 +3611,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(),
- nullptr /* no session */));
+ server_ctx.get()));
const std::vector<uint8_t> record = {1, 2, 3, 4, 5};
std::vector<uint8_t> prefix(
@@ -3604,8 +3654,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(),
- nullptr /* no session */));
+ server_ctx.get()));
const std::vector<uint8_t> plaintext = {1, 2, 3, 4, 5};
std::vector<uint8_t> record = plaintext;
@@ -3643,8 +3692,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(),
- nullptr /* no session */));
+ server_ctx.get()));
const std::vector<uint8_t> plaintext = {1, 2, 3, 4, 5};
std::vector<uint8_t> record = plaintext;
@@ -3683,8 +3731,7 @@
bssl::UniquePtr<SSL> client, server;
ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
- server_ctx.get(),
- nullptr /* no session */));
+ server_ctx.get()));
std::vector<uint8_t> record = {1, 2, 3, 4, 5};
std::vector<uint8_t> prefix(
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 481c9f8..ec70d27 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -619,31 +619,14 @@
static int ext_sni_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t *out_alert,
CBS *contents) {
- SSL *const ssl = hs->ssl;
- if (contents == NULL) {
- return 1;
- }
-
- if (CBS_len(contents) != 0) {
- return 0;
- }
-
- assert(ssl->tlsext_hostname != NULL);
-
- if (ssl->session == NULL) {
- OPENSSL_free(hs->new_session->tlsext_hostname);
- hs->new_session->tlsext_hostname = BUF_strdup(ssl->tlsext_hostname);
- if (!hs->new_session->tlsext_hostname) {
- *out_alert = SSL_AD_INTERNAL_ERROR;
- return 0;
- }
- }
-
- return 1;
+ // The server may acknowledge SNI with an empty extension. We check the syntax
+ // but otherwise ignore this signal.
+ return contents == NULL || CBS_len(contents) == 0;
}
static int ext_sni_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert,
CBS *contents) {
+ SSL *const ssl = hs->ssl;
if (contents == NULL) {
return 1;
}
@@ -674,12 +657,10 @@
}
// Copy the hostname as a string.
- char *hostname_raw = nullptr;
- if (!CBS_strdup(&host_name, &hostname_raw)) {
+ if (!CBS_strdup(&host_name, &ssl->s3->hostname)) {
*out_alert = SSL_AD_INTERNAL_ERROR;
return 0;
}
- hs->hostname.reset(hostname_raw);
hs->should_ack_sni = true;
return 1;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 65c2f11..cd6baa4 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -413,15 +413,6 @@
// Record connection properties in the new session.
hs->new_session->cipher = hs->new_cipher;
- if (hs->hostname != NULL) {
- OPENSSL_free(hs->new_session->tlsext_hostname);
- hs->new_session->tlsext_hostname = BUF_strdup(hs->hostname.get());
- if (hs->new_session->tlsext_hostname == NULL) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
- return ssl_hs_error;
- }
- }
-
// Store the initial negotiated ALPN in the session.
if (ssl->s3->alpn_selected != NULL) {
hs->new_session->early_alpn = (uint8_t *)BUF_memdup(
