Do not distinguish NULL and empty PSK identity hints.

Plain PSK omits the ServerKeyExchange when there is no hint and includes
it otherwise (it should have always sent it), while other PSK ciphers
like ECDHE_PSK cannot omit the hint. Having different capabilities here
is odd and RFC 4279 5.2 suggests that all PSK ciphers are capable of
"[not] provid[ing] an identity hint".

Interpret this to mean no identity hint and empty identity hint are the
same state. Annoyingly, this gives a plain PSK implementation two
options for spelling an empty hint. The spec isn't clear and this is not
really a battle worth fighting, so I've left both acceptable and added a
test for this case.

See also https://android-review.googlesource.com/c/275217/. This is also
consistent with Android's PskKeyManager API, our only consumer anyway.

https://developer.android.com/reference/android/net/PskKeyManager.html

Change-Id: I8a8e6cc1f7dd1b8b202cdaf3d4f151bebfb4a25b
Reviewed-on: https://boringssl-review.googlesource.com/11087
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

6 files changed, 51 insertions, 5 deletions

diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index b8153f5a..238906bf 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -1160,8 +1160,13 @@ static int ssl3_get_server_key_exchange(SSL *ssl) {
       goto f_err;
     }
 
-    /* Save the identity hint as a C string. */
-    if (!CBS_strdup(&psk_identity_hint, &ssl->s3->hs->peer_psk_identity_hint)) {
+    /* Save non-empty identity hints as a C string. Empty identity hints we
+     * treat as missing. Plain PSK makes it possible to send either no hint
+     * (omit ServerKeyExchange) or an empty hint, while ECDHE_PSK can only spell
+     * empty hint. Having different capabilities is odd, so we interpret empty
+     * and missing as identical. */
+    if (CBS_len(&psk_identity_hint) != 0 &&
+        !CBS_strdup(&psk_identity_hint, &ssl->s3->hs->peer_psk_identity_hint)) {
       al = SSL_AD_INTERNAL_ERROR;
       OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
       goto f_err;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 49cfe278..1e1f7527 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2443,7 +2443,11 @@ int SSL_use_psk_identity_hint(SSL *ssl, const char *identity_hint) {
   OPENSSL_free(ssl->psk_identity_hint);
   ssl->psk_identity_hint = NULL;
 
-  if (identity_hint != NULL) {
+  /* Treat the empty hint as not supplying one. Plain PSK makes it possible to
+   * send either no hint (omit ServerKeyExchange) or an empty hint, while
+   * ECDHE_PSK can only spell empty hint. Having different capabilities is odd,
+   * so we interpret empty and missing as identical. */
+  if (identity_hint != NULL && identity_hint[0] != '\0') {
     ssl->psk_identity_hint = BUF_strdup(identity_hint);
     if (ssl->psk_identity_hint == NULL) {
       return 0;
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index a5bea16b..1fe8ce0c 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -552,7 +552,13 @@ static unsigned PskClientCallback(SSL *ssl, const char *hint,
                                   uint8_t *out_psk, unsigned max_psk_len) {
   const TestConfig *config = GetTestConfig(ssl);
 
-  if (strcmp(hint ? hint : "", config->psk_identity.c_str()) != 0) {
+  if (config->psk_identity.empty()) {
+    if (hint != nullptr) {
+      fprintf(stderr, "Server PSK hint was non-null.\n");
+      return 0;
+    }
+  } else if (hint == nullptr ||
+             strcmp(hint, config->psk_identity.c_str()) != 0) {
     fprintf(stderr, "Server PSK hint did not match.\n");
     return 0;
   }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index eef1dbe8..d2f29fa4 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1071,6 +1071,11 @@ type ProtocolBugs struct {
 	// SendCompressionMethods, if not nil, is the compression method list to
 	// send in the ClientHello.
 	SendCompressionMethods []byte
+
+	// AlwaysSendPreSharedKeyIdentityHint, if true, causes the server to
+	// always send a ServerKeyExchange for PSK ciphers, even if the identity
+	// hint is empty.
+	AlwaysSendPreSharedKeyIdentityHint bool
 }
 
 func (c *Config) serverInit() {
diff --git a/ssl/test/runner/key_agreement.go b/ssl/test/runner/key_agreement.go
index ebfb93dc..6327c134 100644
--- a/ssl/test/runner/key_agreement.go
+++ b/ssl/test/runner/key_agreement.go
@@ -915,7 +915,7 @@ func (ka *pskKeyAgreement) generateServerKeyExchange(config *Config, cert *Certi
 
 	if baseSkx != nil {
 		bytes = append(bytes, baseSkx.key...)
-	} else if config.PreSharedKeyIdentity == "" {
+	} else if config.PreSharedKeyIdentity == "" && !config.Bugs.AlwaysSendPreSharedKeyIdentityHint {
 		// ServerKeyExchange is optional if the identity hint is empty
 		// and there would otherwise be no ServerKeyExchange.
 		return nil, nil
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 8e107aaa..feef62cf 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2567,6 +2567,32 @@ func addCipherSuiteTests() {
 		},
 	})
 
+	// Test empty ECDHE_PSK identity hints work as expected.
+	testCases = append(testCases, testCase{
+		name: "EmptyECDHEPSKHint",
+		config: Config{
+			MaxVersion:   VersionTLS12,
+			CipherSuites: []uint16{TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA},
+			PreSharedKey: []byte("secret"),
+		},
+		flags: []string{"-psk", "secret"},
+	})
+
+	// Test empty PSK identity hints work as expected, even if an explicit
+	// ServerKeyExchange is sent.
+	testCases = append(testCases, testCase{
+		name: "ExplicitEmptyPSKHint",
+		config: Config{
+			MaxVersion:   VersionTLS12,
+			CipherSuites: []uint16{TLS_PSK_WITH_AES_128_CBC_SHA},
+			PreSharedKey: []byte("secret"),
+			Bugs: ProtocolBugs{
+				AlwaysSendPreSharedKeyIdentityHint: true,
+			},
+		},
+		flags: []string{"-psk", "secret"},
+	})
+
 	// versionSpecificCiphersTest specifies a test for the TLS 1.0 and TLS
 	// 1.1 specific cipher suite settings. A server is setup with the given
 	// cipher lists and then a connection is made for each member of