Make QUIC enforce ALPN when using TLS handshake gfe-relnote: enforce ALPN when using TLS, protected by disabled quic_tls flag PiperOrigin-RevId: 261159061 Change-Id: I9ccdd221e92beae2b83677e692e3c6d084351731
diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc index 735fd3b..2e015c8 100644 --- a/quic/core/tls_client_handshaker.cc +++ b/quic/core/tls_client_handshaker.cc
@@ -15,6 +15,8 @@ namespace quic { +std::string* quic_alpn_override_on_client_for_tests = nullptr; + TlsClientHandshaker::ProofVerifierCallbackImpl::ProofVerifierCallbackImpl( TlsClientHandshaker* parent) : parent_(parent) {} @@ -77,29 +79,36 @@ } std::string alpn_string = AlpnForVersion(session()->connection()->version()); + if (quic_alpn_override_on_client_for_tests != nullptr) { + alpn_string = *quic_alpn_override_on_client_for_tests; + } if (alpn_string.length() > std::numeric_limits<uint8_t>::max()) { QUIC_BUG << "ALPN too long: '" << alpn_string << "'"; - CloseConnection(QUIC_HANDSHAKE_FAILED, "ALPN too long"); + CloseConnection(QUIC_HANDSHAKE_FAILED, + "Client configured ALPN is too long"); return false; } const uint8_t alpn_length = alpn_string.length(); - // SSL_set_alpn_protos expects a sequence of one-byte-length-prefixed strings - // so we copy alpn_string to a new buffer that has the length in alpn[0]. - uint8_t alpn[std::numeric_limits<uint8_t>::max() + 1]; - alpn[0] = alpn_length; - memcpy(reinterpret_cast<char*>(alpn + 1), alpn_string.data(), alpn_length); - if (SSL_set_alpn_protos(ssl(), alpn, - static_cast<unsigned>(alpn_length) + 1) != 0) { - QUIC_BUG << "Failed to set ALPN: '" << alpn_string << "'"; - CloseConnection(QUIC_HANDSHAKE_FAILED, "Failed to set ALPN"); - return false; + if (alpn_length > 0) { + // SSL_set_alpn_protos expects a sequence of one-byte-length-prefixed + // strings so we copy alpn_string to a new buffer that has the length + // in alpn[0]. + uint8_t alpn[std::numeric_limits<uint8_t>::max() + 1]; + alpn[0] = alpn_length; + memcpy(reinterpret_cast<char*>(alpn + 1), alpn_string.data(), alpn_length); + if (SSL_set_alpn_protos(ssl(), alpn, + static_cast<unsigned>(alpn_length) + 1) != 0) { + QUIC_BUG << "Failed to set ALPN: '" << alpn_string << "'"; + CloseConnection(QUIC_HANDSHAKE_FAILED, "Client failed to set ALPN"); + return false; + } } QUIC_DLOG(INFO) << "Client using ALPN: '" << alpn_string << "'"; // Set the Transport Parameters to send in the ClientHello if (!SetTransportParameters()) { CloseConnection(QUIC_HANDSHAKE_FAILED, - "Failed to set Transport Parameters"); + "Client failed to set Transport Parameters"); return false; } @@ -206,7 +215,8 @@ return; } if (state_ == STATE_IDLE) { - CloseConnection(QUIC_HANDSHAKE_FAILED, "TLS handshake failed"); + CloseConnection(QUIC_HANDSHAKE_FAILED, + "Client observed TLS handshake idle failure"); return; } if (state_ == STATE_HANDSHAKE_COMPLETE) { @@ -236,7 +246,8 @@ // TODO(nharper): Surface error details from the error queue when ssl_error // is SSL_ERROR_SSL. QUIC_LOG(WARNING) << "SSL_do_handshake failed; closing connection"; - CloseConnection(QUIC_HANDSHAKE_FAILED, "TLS handshake failed"); + CloseConnection(QUIC_HANDSHAKE_FAILED, + "Client observed TLS handshake failure"); } } @@ -261,25 +272,31 @@ const uint8_t* alpn_data = nullptr; unsigned alpn_length = 0; SSL_get0_alpn_selected(ssl(), &alpn_data, &alpn_length); - // TODO(b/130164908) Act on ALPN. - if (alpn_length != 0) { - std::string received_alpn_string(reinterpret_cast<const char*>(alpn_data), - alpn_length); - std::string sent_alpn_string = - AlpnForVersion(session()->connection()->version()); - if (received_alpn_string != sent_alpn_string) { - QUIC_LOG(ERROR) << "Client: received mismatched ALPN '" - << received_alpn_string << "', expected '" - << sent_alpn_string << "'"; - CloseConnection(QUIC_HANDSHAKE_FAILED, "Mismatched ALPN"); - return; - } - QUIC_DLOG(INFO) << "Client: server selected ALPN: '" << received_alpn_string - << "'"; - } else { - QUIC_DLOG(INFO) << "Client: server did not select ALPN"; + + if (alpn_length == 0) { + QUIC_DLOG(ERROR) << "Client: server did not select ALPN"; + // TODO(b/130164908) this should send no_application_protocol + // instead of QUIC_HANDSHAKE_FAILED. + CloseConnection(QUIC_HANDSHAKE_FAILED, "Server did not select ALPN"); + return; } + std::string received_alpn_string(reinterpret_cast<const char*>(alpn_data), + alpn_length); + std::string sent_alpn_string = + AlpnForVersion(session()->connection()->version()); + if (received_alpn_string != sent_alpn_string) { + QUIC_LOG(ERROR) << "Client: received mismatched ALPN '" + << received_alpn_string << "', expected '" + << sent_alpn_string << "'"; + // TODO(b/130164908) this should send no_application_protocol + // instead of QUIC_HANDSHAKE_FAILED. + CloseConnection(QUIC_HANDSHAKE_FAILED, "Client received mismatched ALPN"); + return; + } + QUIC_DLOG(INFO) << "Client: server selected ALPN: '" << received_alpn_string + << "'"; + session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); session()->NeuterUnencryptedData(); encryption_established_ = true;
diff --git a/quic/core/tls_client_handshaker.h b/quic/core/tls_client_handshaker.h index c22fbe9..d8d8499 100644 --- a/quic/core/tls_client_handshaker.h +++ b/quic/core/tls_client_handshaker.h
@@ -121,6 +121,10 @@ TlsClientConnection tls_connection_; }; +// Allows tests to override the ALPN used by clients. +// DO NOT use outside of tests. +extern std::string* quic_alpn_override_on_client_for_tests; + } // namespace quic #endif // QUICHE_QUIC_CORE_TLS_CLIENT_HANDSHAKER_H_
diff --git a/quic/core/tls_handshaker_test.cc b/quic/core/tls_handshaker_test.cc index f15a266..67dd47f 100644 --- a/quic/core/tls_handshaker_test.cc +++ b/quic/core/tls_handshaker_test.cc
@@ -430,6 +430,42 @@ EXPECT_FALSE(server_stream_->handshake_confirmed()); } +TEST_F(TlsHandshakerTest, ClientNotSendingALPN) { + static std::string kTestClientNoAlpn = ""; + quic_alpn_override_on_client_for_tests = &kTestClientNoAlpn; + EXPECT_CALL(*client_conn_, CloseConnection(QUIC_HANDSHAKE_FAILED, + "Server did not select ALPN", _)); + EXPECT_CALL(*server_conn_, + CloseConnection(QUIC_HANDSHAKE_FAILED, + "Server did not receive a known ALPN", _)); + client_stream_->CryptoConnect(); + ExchangeHandshakeMessages(client_stream_, server_stream_); + + EXPECT_FALSE(client_stream_->handshake_confirmed()); + EXPECT_FALSE(client_stream_->encryption_established()); + EXPECT_FALSE(server_stream_->handshake_confirmed()); + EXPECT_FALSE(server_stream_->encryption_established()); + quic_alpn_override_on_client_for_tests = nullptr; +} + +TEST_F(TlsHandshakerTest, ClientSendingBadALPN) { + static std::string kTestBadClientAlpn = "bad-client-alpn"; + quic_alpn_override_on_client_for_tests = &kTestBadClientAlpn; + EXPECT_CALL(*client_conn_, CloseConnection(QUIC_HANDSHAKE_FAILED, + "Server did not select ALPN", _)); + EXPECT_CALL(*server_conn_, + CloseConnection(QUIC_HANDSHAKE_FAILED, + "Server did not receive a known ALPN", _)); + client_stream_->CryptoConnect(); + ExchangeHandshakeMessages(client_stream_, server_stream_); + + EXPECT_FALSE(client_stream_->handshake_confirmed()); + EXPECT_FALSE(client_stream_->encryption_established()); + EXPECT_FALSE(server_stream_->handshake_confirmed()); + EXPECT_FALSE(server_stream_->encryption_established()); + quic_alpn_override_on_client_for_tests = nullptr; +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/tls_server_handshaker.cc b/quic/core/tls_server_handshaker.cc index 0e7d9e5..0f2c0d2 100644 --- a/quic/core/tls_server_handshaker.cc +++ b/quic/core/tls_server_handshaker.cc
@@ -61,7 +61,7 @@ if (!SetTransportParameters()) { CloseConnection(QUIC_HANDSHAKE_FAILED, - "Failed to set Transport Parameters"); + "Server failed to set Transport Parameters"); } } @@ -170,7 +170,8 @@ QUIC_LOG(WARNING) << "SSL_do_handshake failed; SSL_get_error returns " << ssl_error << ", state_ = " << state_; ERR_print_errors_fp(stderr); - CloseConnection(QUIC_HANDSHAKE_FAILED, "TLS handshake failed"); + CloseConnection(QUIC_HANDSHAKE_FAILED, + "Server observed TLS handshake failure"); } } @@ -239,6 +240,16 @@ } void TlsServerHandshaker::FinishHandshake() { + if (!valid_alpn_received_) { + QUIC_DLOG(ERROR) + << "Server: handshake finished without receiving a known ALPN"; + // TODO(b/130164908) this should send no_application_protocol + // instead of QUIC_HANDSHAKE_FAILED. + CloseConnection(QUIC_HANDSHAKE_FAILED, + "Server did not receive a known ALPN"); + return; + } + QUIC_LOG(INFO) << "Server: handshake finished"; state_ = STATE_HANDSHAKE_COMPLETE; @@ -328,26 +339,48 @@ const uint8_t* in, unsigned in_len) { // |in| contains a sequence of 1-byte-length-prefixed values. - // We currently simply return the first provided ALPN value. - // TODO(b/130164908) Act on ALPN. + *out_len = 0; + *out = nullptr; if (in_len == 0) { - *out_len = 0; - *out = nullptr; - QUIC_DLOG(INFO) << "No ALPN provided"; - return SSL_TLSEXT_ERR_OK; + QUIC_DLOG(ERROR) << "No ALPN provided by client"; + return SSL_TLSEXT_ERR_NOACK; } - const uint8_t first_alpn_length = in[0]; - if (static_cast<unsigned>(first_alpn_length) > in_len - 1) { - QUIC_LOG(ERROR) << "Failed to parse ALPN"; - return SSL_TLSEXT_ERR_ALERT_FATAL; + + std::string expected_alpn_string = + AlpnForVersion(session()->connection()->version()); + + CBS all_alpns; + CBS_init(&all_alpns, in, in_len); + + while (CBS_len(&all_alpns) > 0) { + CBS alpn; + if (!CBS_get_u8_length_prefixed(&all_alpns, &alpn)) { + QUIC_DLOG(ERROR) << "Failed to parse ALPN length"; + return SSL_TLSEXT_ERR_NOACK; + } + const size_t alpn_length = CBS_len(&alpn); + if (alpn_length > + static_cast<size_t>(std::numeric_limits<uint8_t>::max())) { + QUIC_BUG << "Parsed impossible ALPN length " << alpn_length; + return SSL_TLSEXT_ERR_NOACK; + } + if (alpn_length == 0) { + QUIC_DLOG(ERROR) << "Received invalid zero-length ALPN"; + return SSL_TLSEXT_ERR_NOACK; + } + std::string alpn_string(reinterpret_cast<const char*>(CBS_data(&alpn)), + alpn_length); + if (alpn_string == expected_alpn_string) { + QUIC_DLOG(INFO) << "Server selecting ALPN '" << alpn_string << "'"; + *out_len = static_cast<uint8_t>(alpn_length); + *out = CBS_data(&alpn); + valid_alpn_received_ = true; + return SSL_TLSEXT_ERR_OK; + } } - *out_len = first_alpn_length; - *out = in + 1; - QUIC_DLOG(INFO) << "Server selecting ALPN '" - << QuicStringPiece(reinterpret_cast<const char*>(*out), - *out_len) - << "'"; - return SSL_TLSEXT_ERR_OK; + + QUIC_DLOG(ERROR) << "No known ALPN provided by client"; + return SSL_TLSEXT_ERR_NOACK; } } // namespace quic
diff --git a/quic/core/tls_server_handshaker.h b/quic/core/tls_server_handshaker.h index 71fe6dd..5ce699f 100644 --- a/quic/core/tls_server_handshaker.h +++ b/quic/core/tls_server_handshaker.h
@@ -123,6 +123,7 @@ bool encryption_established_ = false; bool handshake_confirmed_ = false; + bool valid_alpn_received_ = false; QuicReferenceCountedPointer<QuicCryptoNegotiatedParameters> crypto_negotiated_params_; TlsServerConnection tls_connection_;