Test various empty string cases with NPN callbacks NPN is a little odd, owing to being a three-step process. The client offers NPN, then the server accepts NPN and offers a list of protocols, then the client picks a protocol. The server is permitted to accept NPN but then offer zero supported protocols. This worked, but was not tested or clearly documented. In the last step, the client *must* pick a protocol, but it is permitted to pick the empty string. The semantics of this are not explicitly stated in the draft, but one can infer it means we aren't picking a protocol. This also worked but was not tested or clearly documented. Change-Id: I26d7089f4902834ff68a97467fc826e957d5fdf8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69027 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Bob Beck <bbe@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 04c191f..6a92a28 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -3265,30 +3265,49 @@ // and deprecated in favor of it. // SSL_CTX_set_next_protos_advertised_cb sets a callback that is called when a -// TLS server needs a list of supported protocols for Next Protocol -// Negotiation. The returned list must be in wire format. The list is returned -// by setting |*out| to point to it and |*out_len| to its length. This memory -// will not be modified, but one should assume that |ssl| keeps a reference to -// it. +// TLS server needs a list of supported protocols for Next Protocol Negotiation. // -// The callback should return |SSL_TLSEXT_ERR_OK| if it wishes to advertise. -// Otherwise, no such extension will be included in the ServerHello. +// If the callback wishes to advertise NPN to the client, it should return +// |SSL_TLSEXT_ERR_OK| and then set |*out| and |*out_len| to describe to a +// buffer containing a (possibly empty) list of supported protocols in wire +// format. That is, each protocol is prefixed with a 1-byte length, then +// concatenated. From there, the client will select a protocol, possibly one not +// on the server's list. The caller can use |SSL_get0_next_proto_negotiated| +// after the handshake completes to query the final protocol. +// +// The returned buffer must remain valid and unmodified for at least the +// duration of the |SSL| operation (e.g. |SSL_do_handshake|) that triggered the +// callback. +// +// If the caller wishes not to advertise NPN, it should return +// |SSL_TLSEXT_ERR_NOACK|. No NPN extension will be included in the ServerHello, +// and the TLS server will behave as if it does not implement NPN. OPENSSL_EXPORT void SSL_CTX_set_next_protos_advertised_cb( SSL_CTX *ctx, int (*cb)(SSL *ssl, const uint8_t **out, unsigned *out_len, void *arg), void *arg); // SSL_CTX_set_next_proto_select_cb sets a callback that is called when a client -// needs to select a protocol from the server's provided list. |*out| must be -// set to point to the selected protocol (which may be within |in|). The length -// of the protocol name must be written into |*out_len|. The server's advertised -// protocols are provided in |in| and |in_len|. The callback can assume that -// |in| is syntactically valid. +// needs to select a protocol from the server's provided list, passed in wire +// format in |in_len| bytes from |in|. The callback can assume that |in| is +// syntactically valid. // -// The client must select a protocol. It is fatal to the connection if this -// callback returns a value other than |SSL_TLSEXT_ERR_OK|. +// On success, the callback should return |SSL_TLSEXT_ERR_OK| and set |*out| and +// |*out_len| to describe a buffer containing the selected protocol, or an +// empty buffer to select no protocol. The returned buffer may point within +// |in|, or it may point to some other buffer that remains valid and unmodified +// for at least the duration of the |SSL| operation (e.g. |SSL_do_handshake|) +// that triggered the callback. // -// Configuring this callback enables NPN on a client. +// Returning any other value indicates a fatal error and will terminate the TLS +// connection. To proceed without selecting a protocol, the callback must return +// |SSL_TLSEXT_ERR_OK| and set |*out| and |*out_len| to an empty buffer. (E.g. +// NULL and zero, respectively.) +// +// Configuring this callback enables NPN on a client. Although the callback can +// then decline to negotiate a protocol, merely configuring the callback causes +// the client to offer NPN in the ClientHello. Callers thus should not configure +// this callback in TLS client contexts that are not intended to use NPN. OPENSSL_EXPORT void SSL_CTX_set_next_proto_select_cb( SSL_CTX *ctx, int (*cb)(SSL *ssl, uint8_t **out, uint8_t *out_len, const uint8_t *in, unsigned in_len, void *arg), @@ -3296,7 +3315,7 @@ // SSL_get0_next_proto_negotiated sets |*out_data| and |*out_len| to point to // the client's requested protocol for this connection. If the client didn't -// request any protocol, then |*out_data| is set to NULL. +// request any protocol, then |*out_len| is set to zero. // // Note that the client can request any protocol it chooses. The value returned // from this function need not be a member of the list of supported protocols
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 38ab01c..5bf9af4 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc
@@ -524,7 +524,7 @@ } } - if (!config->expect_next_proto.empty()) { + if (!config->expect_next_proto.empty() || config->expect_no_next_proto) { const uint8_t *next_proto; unsigned next_proto_len; SSL_get0_next_proto_negotiated(ssl, &next_proto, &next_proto_len);
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 082efce..1681c2b 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -455,6 +455,14 @@ // NextProtos is a list of supported, application level protocols. NextProtos []string + // NoFallbackNextProto, if true, causes the client to decline to pick an NPN + // protocol, instead of picking an opportunistic, fallback protocol. + NoFallbackNextProto bool + + // NegotiateNPNWithNoProtos, if true, causes the server to negotiate NPN + // despite having no protocols configured. + NegotiateNPNWithNoProtos bool + // ApplicationSettings is a set of application settings to use which each // application protocol. ApplicationSettings map[string][]byte
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 0c338d7..7172b3d 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -2265,6 +2265,10 @@ if hs.serverHello.extensions.nextProtoNeg { nextProto := new(nextProtoMsg) proto, fallback := mutualProtocol(c.config.NextProtos, hs.serverHello.extensions.nextProtos) + if fallback && c.config.NoFallbackNextProto { + proto = "" + fallback = false + } nextProto.proto = proto c.clientProtocol = proto c.clientProtocolFallback = fallback
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index ceaf029..7459b5b 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go
@@ -1664,11 +1664,7 @@ if c.vers < VersionTLS13 || config.Bugs.NegotiateNPNAtAllVersions { if len(hs.clientHello.alpnProtocols) == 0 || c.config.Bugs.NegotiateALPNAndNPN { - // Although sending an empty NPN extension is reasonable, Firefox has - // had a bug around this. Best to send nothing at all if - // config.NextProtos is empty. See - // https://code.google.com/p/go/issues/detail?id=5445. - if hs.clientHello.nextProtoNeg && len(config.NextProtos) > 0 { + if hs.clientHello.nextProtoNeg && (len(config.NextProtos) > 0 || config.NegotiateNPNWithNoProtos) { serverExtensions.nextProtoNeg = true serverExtensions.nextProtos = config.NextProtos serverExtensions.npnAfterAlpn = config.Bugs.SwapNPNAndALPN
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 8a31afa..39b7611 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -5683,7 +5683,7 @@ expectedError: halfHelloRequestError, }) - // NPN on client and server; results in post-handshake message. + // NPN on client and server; results in post-ChangeCipherSpec message. tests = append(tests, testCase{ name: "NPN-Client", config: Config{ @@ -5715,6 +5715,73 @@ }, }) + // The client may select no protocol after seeing the server list. + tests = append(tests, testCase{ + name: "NPN-Client-ClientSelectEmpty", + config: Config{ + MaxVersion: VersionTLS12, + NextProtos: []string{"foo"}, + }, + flags: []string{"-select-empty-next-proto"}, + resumeSession: true, + expectations: connectionExpectations{ + noNextProto: true, + nextProtoType: npn, + }, + }) + tests = append(tests, testCase{ + testType: serverTest, + name: "NPN-Server-ClientSelectEmpty", + config: Config{ + MaxVersion: VersionTLS12, + NextProtos: []string{"no-match"}, + NoFallbackNextProto: true, + }, + flags: []string{ + "-advertise-npn", "\x03foo\x03bar\x03baz", + "-expect-no-next-proto", + }, + resumeSession: true, + expectations: connectionExpectations{ + noNextProto: true, + nextProtoType: npn, + }, + }) + + // The server may negotiate NPN, despite offering no protocols. In this + // case, the server must still be prepared for the client to select a + // fallback protocol. + tests = append(tests, testCase{ + name: "NPN-Client-ServerAdvertiseEmpty", + config: Config{ + MaxVersion: VersionTLS12, + NegotiateNPNWithNoProtos: true, + }, + flags: []string{"-select-next-proto", "foo"}, + resumeSession: true, + expectations: connectionExpectations{ + nextProto: "foo", + nextProtoType: npn, + }, + }) + tests = append(tests, testCase{ + testType: serverTest, + name: "NPN-Server-ServerAdvertiseEmpty", + config: Config{ + MaxVersion: VersionTLS12, + NextProtos: []string{"foo"}, + }, + flags: []string{ + "-advertise-empty-npn", + "-expect-next-proto", "foo", + }, + resumeSession: true, + expectations: connectionExpectations{ + nextProto: "foo", + nextProtoType: npn, + }, + }) + // Client does False Start and negotiates NPN. tests = append(tests, testCase{ name: "FalseStart",
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index b84fd08..24b1bdd 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -293,9 +293,13 @@ BoolFlag("-require-any-client-certificate", &TestConfig::require_any_client_certificate), StringFlag("-advertise-npn", &TestConfig::advertise_npn), + BoolFlag("-advertise-empty-npn", &TestConfig::advertise_empty_npn), StringFlag("-expect-next-proto", &TestConfig::expect_next_proto), + BoolFlag("-expect-no-next-proto", &TestConfig::expect_no_next_proto), BoolFlag("-false-start", &TestConfig::false_start), StringFlag("-select-next-proto", &TestConfig::select_next_proto), + BoolFlag("-select-empty-next-proto", + &TestConfig::select_empty_next_proto), BoolFlag("-async", &TestConfig::async), BoolFlag("-write-different-record-sizes", &TestConfig::write_different_record_sizes), @@ -713,10 +717,6 @@ const uint8_t *in, unsigned inlen, void *arg) { const TestConfig *config = GetTestConfig(ssl); - if (config->select_next_proto.empty()) { - return SSL_TLSEXT_ERR_NOACK; - } - *out = (uint8_t *)config->select_next_proto.data(); *outlen = config->select_next_proto.size(); return SSL_TLSEXT_ERR_OK; @@ -725,7 +725,7 @@ static int NextProtosAdvertisedCallback(SSL *ssl, const uint8_t **out, unsigned int *out_len, void *arg) { const TestConfig *config = GetTestConfig(ssl); - if (config->advertise_npn.empty()) { + if (config->advertise_npn.empty() && !config->advertise_empty_npn) { return SSL_TLSEXT_ERR_NOACK; } @@ -1721,7 +1721,7 @@ SSL_CTX_set_next_protos_advertised_cb(ssl_ctx.get(), NextProtosAdvertisedCallback, NULL); - if (!select_next_proto.empty()) { + if (!select_next_proto.empty() || select_empty_next_proto) { SSL_CTX_set_next_proto_select_cb(ssl_ctx.get(), NextProtoSelectCallback, NULL); }
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 89cb2be..89656d1 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -67,9 +67,12 @@ std::string expect_certificate_types; bool require_any_client_certificate = false; std::string advertise_npn; + bool advertise_empty_npn = false; std::string expect_next_proto; + bool expect_no_next_proto = false; bool false_start = false; std::string select_next_proto; + bool select_empty_next_proto = false; bool async = false; bool write_different_record_sizes = false; bool cbc_record_splitting = false;