Add is_quic bit to SSL_SESSION
This bit is used to prevent cross-protocol resumption between QUIC and
TLS-over-TCP.
Bug: 221
Change-Id: I8ab5341f00ae96c0a5f7ac3999f61edc7cbeca1c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40444
Commit-Queue: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index e64e456..d77a971 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -406,7 +406,8 @@
(ssl->session->session_id_length == 0 &&
ssl->session->ticket.empty()) ||
ssl->session->not_resumable ||
- !ssl_session_is_time_valid(ssl, ssl->session.get())) {
+ !ssl_session_is_time_valid(ssl, ssl->session.get()) ||
+ (ssl->quic_method != nullptr) != ssl->session->is_quic) {
ssl_set_session(ssl, NULL);
}
}
diff --git a/ssl/internal.h b/ssl/internal.h
index ac3a844..04bf7a4 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -3548,6 +3548,9 @@
// is_server is whether this session was created by a server.
bool is_server : 1;
+ // is_quic indicates whether this session was created using QUIC.
+ bool is_quic : 1;
+
private:
~ssl_session_st();
friend void SSL_SESSION_free(SSL_SESSION *);
diff --git a/ssl/ssl_asn1.cc b/ssl/ssl_asn1.cc
index ea02cf4..98ea4fe 100644
--- a/ssl/ssl_asn1.cc
+++ b/ssl/ssl_asn1.cc
@@ -129,6 +129,7 @@
// ticketMaxEarlyData [24] INTEGER OPTIONAL,
// authTimeout [25] INTEGER OPTIONAL, -- defaults to timeout
// earlyALPN [26] OCTET STRING OPTIONAL,
+// isQuic [27] BOOLEAN OPTIONAL,
// }
//
// Note: historically this serialization has included other optional
@@ -188,6 +189,8 @@
CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 25;
static const unsigned kEarlyALPNTag =
CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 26;
+static const unsigned kIsQuicTag =
+ CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 27;
static int SSL_SESSION_to_bytes_full(const SSL_SESSION *in, CBB *cbb,
int for_ticket) {
@@ -388,6 +391,15 @@
}
}
+ if (in->is_quic) {
+ if (!CBB_add_asn1(&session, &child, kIsQuicTag) ||
+ !CBB_add_asn1_bool(&child, true)) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+ return 0;
+ }
+ }
+
+
return CBB_flush(cbb);
}
@@ -718,6 +730,7 @@
ret->is_server = is_server;
+ int is_quic;
if (!SSL_SESSION_parse_u16(&session, &ret->peer_signature_algorithm,
kPeerSignatureAlgorithmTag, 0) ||
!SSL_SESSION_parse_u32(&session, &ret->ticket_max_early_data,
@@ -726,10 +739,13 @@
ret->timeout) ||
!SSL_SESSION_parse_octet_string(&session, &ret->early_alpn,
kEarlyALPNTag) ||
+ !CBS_get_optional_asn1_bool(&session, &is_quic, kIsQuicTag,
+ /*default_value=*/false) ||
CBS_len(&session) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION);
return nullptr;
}
+ ret->is_quic = is_quic;
if (!x509_method->session_cache_objects(ret.get())) {
OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION);
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 6635703..8819239 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -197,6 +197,7 @@
new_session->is_server = session->is_server;
new_session->ssl_version = session->ssl_version;
+ new_session->is_quic = session->is_quic;
new_session->sid_ctx_length = session->sid_ctx_length;
OPENSSL_memcpy(new_session->sid_ctx, session->sid_ctx, session->sid_ctx_length);
@@ -357,6 +358,7 @@
session->is_server = is_server;
session->ssl_version = ssl->version;
+ session->is_quic = ssl->quic_method != nullptr;
// Fill in the time from the |SSL_CTX|'s clock.
struct OPENSSL_timeval now;
@@ -639,7 +641,10 @@
((sk_CRYPTO_BUFFER_num(session->certs.get()) == 0 &&
!session->peer_sha256_valid) ||
session->peer_sha256_valid ==
- hs->config->retain_only_sha256_of_client_certs);
+ hs->config->retain_only_sha256_of_client_certs) &&
+ // Only resume if the underlying transport protocol hasn't changed.
+ // This is to prevent cross-protocol resumption between QUIC and TCP.
+ (hs->ssl->quic_method != nullptr) == session->is_quic;
}
// ssl_lookup_session looks up |session_id| in the session cache and sets
@@ -853,7 +858,8 @@
peer_sha256_valid(false),
not_resumable(false),
ticket_age_add_valid(false),
- is_server(false) {
+ is_server(false),
+ is_quic(false) {
CRYPTO_new_ex_data(&ex_data);
time = ::time(nullptr);
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 9ccab18..3f7bbfc 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -5772,6 +5772,100 @@
ExpectReceivedTransportParamsEqual(server_.get(), kClientParams);
}
+TEST_F(QUICMethodTest, ForbidCrossProtocolResumptionClient) {
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
+
+ g_last_session = nullptr;
+
+ SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
+ SSL_CTX_sess_set_new_cb(client_ctx_.get(), SaveLastSession);
+ ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
+ ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
+
+ ASSERT_TRUE(CreateClientAndServer());
+ ASSERT_TRUE(CompleteHandshakesForQUIC());
+
+ ExpectHandshakeSuccess();
+ EXPECT_FALSE(SSL_session_reused(client_.get()));
+ EXPECT_FALSE(SSL_session_reused(server_.get()));
+
+ // The server sent NewSessionTicket messages in the handshake.
+ EXPECT_FALSE(g_last_session);
+ ASSERT_TRUE(ProvideHandshakeData(client_.get()));
+ EXPECT_EQ(SSL_process_quic_post_handshake(client_.get()), 1);
+ EXPECT_TRUE(g_last_session);
+
+ // Pretend that g_last_session came from a TLS-over-TCP connection.
+ g_last_session.get()->is_quic = false;
+
+ // Create a second connection and verify that resumption does not occur with
+ // a session from a non-QUIC connection. This tests that the client does not
+ // offer over QUIC a session believed to be received over TCP. The server
+ // believes this is a QUIC session, so if the client offered the session, the
+ // server would have resumed it.
+ ASSERT_TRUE(CreateClientAndServer());
+ bssl::UniquePtr<SSL_SESSION> session = std::move(g_last_session);
+ SSL_set_session(client_.get(), session.get());
+
+ ASSERT_TRUE(CompleteHandshakesForQUIC());
+ ExpectHandshakeSuccess();
+ EXPECT_FALSE(SSL_session_reused(client_.get()));
+ EXPECT_FALSE(SSL_session_reused(server_.get()));
+}
+
+TEST_F(QUICMethodTest, ForbidCrossProtocolResumptionServer) {
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
+
+ g_last_session = nullptr;
+
+ SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH);
+ SSL_CTX_sess_set_new_cb(client_ctx_.get(), SaveLastSession);
+ ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method));
+ ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method));
+
+ ASSERT_TRUE(CreateClientAndServer());
+ ASSERT_TRUE(CompleteHandshakesForQUIC());
+
+ ExpectHandshakeSuccess();
+ EXPECT_FALSE(SSL_session_reused(client_.get()));
+ EXPECT_FALSE(SSL_session_reused(server_.get()));
+
+ // The server sent NewSessionTicket messages in the handshake.
+ EXPECT_FALSE(g_last_session);
+ ASSERT_TRUE(ProvideHandshakeData(client_.get()));
+ EXPECT_EQ(SSL_process_quic_post_handshake(client_.get()), 1);
+ EXPECT_TRUE(g_last_session);
+
+ // Attempt a resumption with g_last_session using TLS_method.
+ bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(client_ctx);
+
+ ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), nullptr));
+
+ bssl::UniquePtr<SSL> client(SSL_new(client_ctx.get())),
+ server(SSL_new(server_ctx_.get()));
+ ASSERT_TRUE(client);
+ ASSERT_TRUE(server);
+ SSL_set_connect_state(client.get());
+ SSL_set_accept_state(server.get());
+
+ // The TLS-over-TCP client will refuse to resume with a quic session, so
+ // mark is_quic = false to bypass the client check to test the server check.
+ g_last_session.get()->is_quic = false;
+ SSL_set_session(client.get(), g_last_session.get());
+
+ BIO *bio1, *bio2;
+ ASSERT_TRUE(BIO_new_bio_pair(&bio1, 0, &bio2, 0));
+
+ // SSL_set_bio takes ownership.
+ SSL_set_bio(client.get(), bio1, bio1);
+ SSL_set_bio(server.get(), bio2, bio2);
+ ASSERT_TRUE(CompleteHandshakes(client.get(), server.get()));
+
+ EXPECT_FALSE(SSL_session_reused(client.get()));
+ EXPECT_FALSE(SSL_session_reused(server.get()));
+}
+
extern "C" {
int BORINGSSL_enum_c_type_test(void);
}
