From 3e51757de2bf9beef7d249f22d255e4dd9ddb012 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 11 Aug 2016 11:52:23 -0400 Subject: [PATCH] Enforce the server ALPN protocol was advertised. The server should not be allowed select a protocol that wasn't advertised. Callers tend to not really notice and act as if some default were chosen which is unlikely to work very well. Change-Id: Ib6388db72f05386f854d275bab762ca79e8174e6 Reviewed-on: https://boringssl-review.googlesource.com/10284 Reviewed-by: Adam Langley Commit-Queue: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/err/ssl.errordata | 1 + include/openssl/ssl.h | 1 + ssl/t1_lib.c | 26 ++++++++++++++++++++++++++ ssl/test/runner/common.go | 4 ++-- ssl/test/runner/runner.go | 16 ++++++++++++++++ 5 files changed, 46 insertions(+), 2 deletions(-) diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index 5800d41d..7753f17a 100644 --- a/crypto/err/ssl.errordata +++ b/crypto/err/ssl.errordata @@ -59,6 +59,7 @@ SSL,154,HANDSHAKE_FAILURE_ON_CLIENT_HELLO SSL,155,HTTPS_PROXY_REQUEST SSL,156,HTTP_REQUEST SSL,157,INAPPROPRIATE_FALLBACK +SSL,259,INVALID_ALPN_PROTOCOL SSL,158,INVALID_COMMAND SSL,256,INVALID_COMPRESSION_LIST SSL,159,INVALID_MESSAGE diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 931b100f..fe6bac2e 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4803,6 +4803,7 @@ OPENSSL_EXPORT int SSL_set_ssl_method(SSL *s, const SSL_METHOD *method); #define SSL_R_INVALID_COMPRESSION_LIST 256 #define SSL_R_DUPLICATE_EXTENSION 257 #define SSL_R_MISSING_KEY_SHARE 258 +#define SSL_R_INVALID_ALPN_PROTOCOL 259 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020 diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 62f3824d..5e790a48 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1517,6 +1517,32 @@ static int ext_alpn_parse_serverhello(SSL *ssl, uint8_t *out_alert, return 0; } + /* Check that the protcol name is one of the ones we advertised. */ + int protocol_ok = 0; + CBS client_protocol_name_list, client_protocol_name; + CBS_init(&client_protocol_name_list, ssl->alpn_client_proto_list, + ssl->alpn_client_proto_list_len); + while (CBS_len(&client_protocol_name_list) > 0) { + if (!CBS_get_u8_length_prefixed(&client_protocol_name_list, + &client_protocol_name)) { + *out_alert = SSL_AD_INTERNAL_ERROR; + return 0; + } + + if (CBS_len(&client_protocol_name) == CBS_len(&protocol_name) && + memcmp(CBS_data(&client_protocol_name), CBS_data(&protocol_name), + CBS_len(&protocol_name)) == 0) { + protocol_ok = 1; + break; + } + } + + if (!protocol_ok) { + OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_ALPN_PROTOCOL); + *out_alert = SSL_AD_ILLEGAL_PARAMETER; + return 0; + } + if (!CBS_stow(&protocol_name, &ssl->s3->alpn_selected, &ssl->s3->alpn_selected_len)) { *out_alert = SSL_AD_INTERNAL_ERROR; diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index b6befe42..f3d57c7b 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -875,8 +875,8 @@ type ProtocolBugs struct { NegotiateALPNAndNPN bool // SendALPN, if non-empty, causes the server to send the specified - // string in the ALPN extension regardless of whether the client - // advertised it. + // string in the ALPN extension regardless of the content or presence of + // the client offer. SendALPN string // SendEmptySessionTicket, if true, causes the server to send an empty diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 4cb22b1c..45d3e139 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -4316,6 +4316,22 @@ func addExtensionTests() { expectedNextProtoType: alpn, resumeSession: resumeSession, }) + testCases = append(testCases, testCase{ + testType: clientTest, + name: "ALPNClient-Mismatch-" + ver.name, + config: Config{ + MaxVersion: ver.version, + Bugs: ProtocolBugs{ + SendALPN: "baz", + }, + }, + flags: []string{ + "-advertise-alpn", "\x03foo\x03bar", + }, + shouldFail: true, + expectedError: ":INVALID_ALPN_PROTOCOL:", + expectedLocalError: "remote error: illegal parameter", + }) testCases = append(testCases, testCase{ testType: serverTest, name: "ALPNServer-" + ver.name,