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/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index 27ad65c..ab6b15f 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata
@@ -133,6 +133,7 @@ SSL,196,PSK_NO_CLIENT_CB SSL,197,PSK_NO_SERVER_CB SSL,298,QUIC_INTERNAL_ERROR +SSL,305,QUIC_TRANSPORT_PARAMETERS_MISCONFIGURED SSL,198,READ_TIMEOUT_EXPIRED SSL,199,RECORD_LENGTH_MISMATCH SSL,200,RECORD_TOO_LARGE
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 23489dd..66243e2 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -3067,38 +3067,6 @@ OPENSSL_EXPORT const char *SSL_get_psk_identity(const SSL *ssl); -// QUIC transport parameters. -// -// draft-ietf-quic-tls defines a new TLS extension quic_transport_parameters -// used by QUIC for each endpoint to unilaterally declare its supported -// transport parameters. draft-ietf-quic-transport (section 7.4) defines the -// contents of that extension (a TransportParameters struct) and describes how -// to handle it and its semantic meaning. -// -// BoringSSL handles this extension as an opaque byte string. The caller is -// responsible for serializing and parsing it. - -// SSL_set_quic_transport_params configures |ssl| to send |params| (of length -// |params_len|) in the quic_transport_parameters extension in either the -// ClientHello or EncryptedExtensions handshake message. This extension will -// only be sent if the TLS version is at least 1.3, and for a server, only if -// the client sent the extension. The buffer pointed to by |params| only need be -// valid for the duration of the call to this function. This function returns 1 -// on success and 0 on failure. -OPENSSL_EXPORT int SSL_set_quic_transport_params(SSL *ssl, - const uint8_t *params, - size_t params_len); - -// SSL_get_peer_quic_transport_params provides the caller with the value of the -// quic_transport_parameters extension sent by the peer. A pointer to the buffer -// containing the TransportParameters will be put in |*out_params|, and its -// length in |*params_len|. This buffer will be valid for the lifetime of the -// |SSL|. If no params were received from the peer, |*out_params_len| will be 0. -OPENSSL_EXPORT void SSL_get_peer_quic_transport_params(const SSL *ssl, - const uint8_t **out_params, - size_t *out_params_len); - - // Delegated credentials. // // *** EXPERIMENTAL — PRONE TO CHANGE *** @@ -3172,6 +3140,12 @@ // // Note: 0-RTT support is incomplete and does not currently handle QUIC // transport parameters and server SETTINGS frame. +// +// QUIC implementations must additionally configure transport parameters with +// |SSL_set_quic_transport_params|. |SSL_get_peer_quic_transport_params| may be +// used to query the value received from the peer. BoringSSL handles this +// extension as an opaque byte string. The caller is responsible for serializing +// and parsing them. See draft-ietf-quic-transport (section 7.3) for details. // ssl_encryption_level_t represents a specific QUIC encryption level used to // transmit handshake messages. @@ -3300,6 +3274,24 @@ OPENSSL_EXPORT int SSL_set_quic_method(SSL *ssl, const SSL_QUIC_METHOD *quic_method); +// SSL_set_quic_transport_params configures |ssl| to send |params| (of length +// |params_len|) in the quic_transport_parameters extension in either the +// ClientHello or EncryptedExtensions handshake message. It is an error to set +// transport parameters if |ssl| is not configured for QUIC. The buffer pointed +// to by |params| only need be valid for the duration of the call to this +// function. This function returns 1 on success and 0 on failure. +OPENSSL_EXPORT int SSL_set_quic_transport_params(SSL *ssl, + const uint8_t *params, + size_t params_len); + +// SSL_get_peer_quic_transport_params provides the caller with the value of the +// quic_transport_parameters extension sent by the peer. A pointer to the buffer +// containing the TransportParameters will be put in |*out_params|, and its +// length in |*params_len|. This buffer will be valid for the lifetime of the +// |SSL|. If no params were received from the peer, |*out_params_len| will be 0. +OPENSSL_EXPORT void SSL_get_peer_quic_transport_params( + const SSL *ssl, const uint8_t **out_params, size_t *out_params_len); + // Early data. // @@ -5117,6 +5109,7 @@ #define SSL_R_KEY_USAGE_BIT_INCORRECT 302 #define SSL_R_INCONSISTENT_CLIENT_HELLO 303 #define SSL_R_CIPHER_MISMATCH_ON_EARLY_DATA 304 +#define SSL_R_QUIC_TRANSPORT_PARAMETERS_MISCONFIGURED 305 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
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.