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>

7 files changed, 91 insertions, 13 deletions

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 @@ static bool CheckHandshakeProperties(SSL *ssl, bool is_resume,
     }
   }
 
-  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 @@ type Config struct {
 	// 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 @@ func (hs *clientHandshakeState) sendFinished(out []byte, isResume bool) error {
 	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 @@ func (hs *serverHandshakeState) processClientExtensions(serverExtensions *server
 
 	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 @@ func addStateMachineCoverageTests(config stateMachineTestConfig) {
 			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 @@ func addStateMachineCoverageTests(config stateMachineTestConfig) {
 			},
 		})
 
+		// 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 @@ const Flag<TestConfig> *FindFlag(const char *name) {
         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 @@ static int NextProtoSelectCallback(SSL *ssl, uint8_t **out, uint8_t *outlen,
                                    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 NextProtoSelectCallback(SSL *ssl, uint8_t **out, uint8_t *outlen,
 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 @@ bssl::UniquePtr<SSL_CTX> TestConfig::SetupCtx(SSL_CTX *old_ctx) const {
 
   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 @@ struct TestConfig {
   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;