Require QUIC method with Transport Parameters and vice versa
Bug: 296, 322
Change-Id: I297f53674ee7177f61d75695f47b5caeed78bd17
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40384
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 3f7bbfc..96d67bc 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -5106,13 +5106,34 @@
transport_->client()->AllowOutOfOrderWrites();
transport_->server()->AllowOutOfOrderWrites();
}
+ static const uint8_t transport_params[] = {0};
+ if (!SSL_set_quic_transport_params(client_.get(), transport_params,
+ sizeof(transport_params)) ||
+ !SSL_set_quic_transport_params(server_.get(), transport_params,
+ sizeof(transport_params))) {
+ return false;
+ }
return true;
}
+ enum class ExpectedError {
+ kNoError,
+ kClientError,
+ kServerError,
+ };
+
// CompleteHandshakesForQUIC runs |SSL_do_handshake| on |client_| and
// |server_| until each completes once. It returns true on success and false
// on failure.
bool CompleteHandshakesForQUIC() {
+ return RunQUICHandshakesAndExpectError(ExpectedError::kNoError);
+ }
+
+ // Runs |SSL_do_handshake| on |client_| and |server_| until each completes
+ // once. If |expect_client_error| is true, it will return true only if the
+ // client handshake failed. Otherwise, it returns true if both handshakes
+ // succeed and false otherwise.
+ bool RunQUICHandshakesAndExpectError(ExpectedError expected_error) {
bool client_done = false, server_done = false;
while (!client_done || !server_done) {
if (!client_done) {
@@ -5125,6 +5146,9 @@
if (client_ret == 1) {
client_done = true;
} else if (client_ret != -1 || client_err != SSL_ERROR_WANT_READ) {
+ if (expected_error == ExpectedError::kClientError) {
+ return true;
+ }
ADD_FAILURE() << "Unexpected client output: " << client_ret << " "
<< client_err;
return false;
@@ -5141,13 +5165,16 @@
if (server_ret == 1) {
server_done = true;
} else if (server_ret != -1 || server_err != SSL_ERROR_WANT_READ) {
+ if (expected_error == ExpectedError::kServerError) {
+ return true;
+ }
ADD_FAILURE() << "Unexpected server output: " << server_ret << " "
<< server_err;
return false;
}
}
}
- return true;
+ return expected_error == ExpectedError::kNoError;
}
bssl::UniquePtr<SSL_SESSION> CreateClientSessionForQUIC() {
@@ -5866,6 +5893,26 @@
EXPECT_FALSE(SSL_session_reused(server.get()));
}
+TEST_F(QUICMethodTest, ClientRejectsMissingTransportParams) {
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
+ 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(SSL_set_quic_transport_params(server_.get(), nullptr, 0));
+ ASSERT_TRUE(RunQUICHandshakesAndExpectError(ExpectedError::kServerError));
+}
+
+TEST_F(QUICMethodTest, ServerRejectsMissingTransportParams) {
+ const SSL_QUIC_METHOD quic_method = DefaultQUICMethod();
+ 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(SSL_set_quic_transport_params(client_.get(), nullptr, 0));
+ ASSERT_TRUE(RunQUICHandshakesAndExpectError(ExpectedError::kClientError));
+}
+
extern "C" {
int BORINGSSL_enum_c_type_test(void);
}
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index d04c874..dad0fcf 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -2547,10 +2547,17 @@
static bool ext_quic_transport_params_add_clienthello(SSL_HANDSHAKE *hs,
CBB *out) {
- if (hs->config->quic_transport_params.empty() ||
- hs->max_version <= TLS1_2_VERSION) {
+ if (hs->config->quic_transport_params.empty() && !hs->ssl->quic_method) {
return true;
}
+ if (hs->config->quic_transport_params.empty() || !hs->ssl->quic_method) {
+ // QUIC Transport Parameters must be sent over QUIC, and they must not be
+ // sent over non-QUIC transports. If transport params are set, then
+ // SSL(_CTX)_set_quic_method must also be called.
+ OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_TRANSPORT_PARAMETERS_MISCONFIGURED);
+ return false;
+ }
+ assert(hs->min_version > TLS1_2_VERSION);
CBB contents;
if (!CBB_add_u16(out, TLSEXT_TYPE_quic_transport_parameters) ||
@@ -2568,13 +2575,19 @@
CBS *contents) {
SSL *const ssl = hs->ssl;
if (contents == nullptr) {
- return true;
+ if (!ssl->quic_method) {
+ return true;
+ }
+ assert(ssl->quic_method);
+ *out_alert = SSL_AD_MISSING_EXTENSION;
+ return false;
}
- // QUIC requires TLS 1.3.
- if (ssl_protocol_version(ssl) < TLS1_3_VERSION) {
+ if (!ssl->quic_method) {
*out_alert = SSL_AD_UNSUPPORTED_EXTENSION;
return false;
}
+ // QUIC requires TLS 1.3.
+ assert(ssl_protocol_version(ssl) == TLS1_3_VERSION);
return ssl->s3->peer_quic_transport_params.CopyFrom(*contents);
}
@@ -2583,8 +2596,22 @@
uint8_t *out_alert,
CBS *contents) {
SSL *const ssl = hs->ssl;
- if (!contents || !ssl->quic_method) {
- return true;
+ if (!contents) {
+ if (!ssl->quic_method) {
+ if (hs->config->quic_transport_params.empty()) {
+ return true;
+ }
+ // QUIC transport parameters must not be set if |ssl| is not configured
+ // for QUIC.
+ OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_TRANSPORT_PARAMETERS_MISCONFIGURED);
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ }
+ *out_alert = SSL_AD_MISSING_EXTENSION;
+ return false;
+ }
+ if (!ssl->quic_method) {
+ *out_alert = SSL_AD_UNSUPPORTED_EXTENSION;
+ return false;
}
assert(ssl_protocol_version(ssl) == TLS1_3_VERSION);
return ssl->s3->peer_quic_transport_params.CopyFrom(*contents);
@@ -2592,8 +2619,11 @@
static bool ext_quic_transport_params_add_serverhello(SSL_HANDSHAKE *hs,
CBB *out) {
+ assert(hs->ssl->quic_method != nullptr);
if (hs->config->quic_transport_params.empty()) {
- return true;
+ // Transport parameters must be set when using QUIC.
+ OPENSSL_PUT_ERROR(SSL, SSL_R_QUIC_TRANSPORT_PARAMETERS_MISCONFIGURED);
+ return false;
}
CBB contents;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 762caff..6a00a64 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -636,6 +636,9 @@
// exportTrafficSecrets, if true, configures the test to export the TLS 1.3
// traffic secrets and confirms that they match.
exportTrafficSecrets bool
+ // skipTransportParamsConfig, if true, will skip automatic configuration of
+ // sending QUIC transport parameters when protocol == quic.
+ skipTransportParamsConfig bool
}
var testCases []testCase
@@ -1244,6 +1247,20 @@
flags = append(flags, "-dtls")
} else if test.protocol == quic {
flags = append(flags, "-quic")
+ if !test.skipTransportParamsConfig {
+ test.config.QUICTransportParams = []byte{1, 2}
+ if test.resumeConfig != nil {
+ test.resumeConfig.QUICTransportParams = []byte{1, 2}
+ }
+ test.expectedQUICTransportParams = []byte{3, 4}
+ flags = append(flags,
+ []string{
+ "-quic-transport-params",
+ base64.StdEncoding.EncodeToString([]byte{3, 4}),
+ "-expect-quic-transport-params",
+ base64.StdEncoding.EncodeToString([]byte{1, 2}),
+ }...)
+ }
}
var resumeCount int
@@ -7160,6 +7177,23 @@
base64.StdEncoding.EncodeToString([]byte{1, 2}),
},
expectedQUICTransportParams: []byte{3, 4},
+ skipTransportParamsConfig: true,
+ })
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ protocol: quic,
+ name: "QUICTransportParams-Client-RejectMissing-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ },
+ flags: []string{
+ "-quic-transport-params",
+ base64.StdEncoding.EncodeToString([]byte{3, 4}),
+ },
+ shouldFail: true,
+ expectedError: ":MISSING_EXTENSION:",
+ skipTransportParamsConfig: true,
})
// Server sends params
testCases = append(testCases, testCase{
@@ -7178,65 +7212,59 @@
base64.StdEncoding.EncodeToString([]byte{1, 2}),
},
expectedQUICTransportParams: []byte{3, 4},
+ skipTransportParamsConfig: true,
})
- } else {
testCases = append(testCases, testCase{
- testType: clientTest,
- name: "QUICTransportParams-Client-NotSent-" + ver.name,
+ testType: serverTest,
+ protocol: quic,
+ name: "QUICTransportParams-Server-RejectMissing-" + ver.name,
config: Config{
MinVersion: ver.version,
MaxVersion: ver.version,
},
flags: []string{
- "-max-version",
- strconv.Itoa(int(ver.version)),
"-quic-transport-params",
base64.StdEncoding.EncodeToString([]byte{3, 4}),
},
- })
- testCases = append(testCases, testCase{
- testType: clientTest,
- name: "QUICTransportParams-Client-Rejected-" + ver.name,
- config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- QUICTransportParams: []byte{1, 2},
- },
- flags: []string{
- "-quic-transport-params",
- base64.StdEncoding.EncodeToString([]byte{3, 4}),
- },
- shouldFail: true,
- expectedError: ":ERROR_PARSING_EXTENSION:",
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "QUICTransportParams-Server-Rejected-" + ver.name,
- config: Config{
- MinVersion: ver.version,
- MaxVersion: ver.version,
- QUICTransportParams: []byte{1, 2},
- },
- flags: []string{
- "-expect-quic-transport-params",
- base64.StdEncoding.EncodeToString([]byte{1, 2}),
- },
- shouldFail: true,
- expectedError: "QUIC transport params mismatch",
- })
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "QUICTransportParams-OldServerIgnores-" + ver.name,
- config: Config{
- MaxVersion: VersionTLS13,
- QUICTransportParams: []byte{1, 2},
- },
- flags: []string{
- "-min-version", ver.shimFlag(tls),
- "-max-version", ver.shimFlag(tls),
- },
+ expectedQUICTransportParams: []byte{3, 4},
+ shouldFail: true,
+ expectedError: ":MISSING_EXTENSION:",
+ skipTransportParamsConfig: true,
})
}
+ testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "QUICTransportParams-Client-NotSentInTLS-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ },
+ flags: []string{
+ "-max-version",
+ strconv.Itoa(int(ver.version)),
+ "-quic-transport-params",
+ base64.StdEncoding.EncodeToString([]byte{3, 4}),
+ },
+ shouldFail: true,
+ expectedError: ":QUIC_TRANSPORT_PARAMETERS_MISCONFIGURED:",
+ skipTransportParamsConfig: true,
+ })
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "QUICTransportParams-Server-RejectedInTLS-" + ver.name,
+ config: Config{
+ MinVersion: ver.version,
+ MaxVersion: ver.version,
+ QUICTransportParams: []byte{1, 2},
+ },
+ flags: []string{
+ "-expect-quic-transport-params",
+ base64.StdEncoding.EncodeToString([]byte{1, 2}),
+ },
+ shouldFail: true,
+ expectedLocalError: "remote error: unsupported extension",
+ skipTransportParamsConfig: true,
+ })
// Test ticket behavior.
