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>
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index b8153f5..238906b 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -1160,8 +1160,13 @@
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 49cfe27..1e1f752 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2443,7 +2443,11 @@
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 a5bea16..1fe8ce0 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -552,7 +552,13 @@
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 eef1dbe..d2f29fa 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1071,6 +1071,11 @@
// 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 ebfb93d..6327c13 100644
--- a/ssl/test/runner/key_agreement.go
+++ b/ssl/test/runner/key_agreement.go
@@ -915,7 +915,7 @@
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 8e107aa..feef62c 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2567,6 +2567,32 @@
},
})
+ // 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