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;
