From ae4765396f19f5aa8aeb6565707e8e5ada4f3e6d Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 26 Apr 2017 11:28:20 +0100 Subject: Add a ciphersuite config sanity check for servers Ensure that there are ciphersuites enabled for the maximum supported version we will accept in a ClientHello. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3334) --- ssl/ssl_locl.h | 3 +- ssl/statem/extensions.c | 2 +- ssl/statem/extensions_clnt.c | 2 +- ssl/statem/statem_lib.c | 40 ++++++++- ssl/t1_lib.c | 2 +- test/recipes/70-test_sslmessages.t | 1 + test/ssl-tests/02-protocol-version.conf | 32 +++---- test/ssl-tests/23-srp.conf | 4 + test/ssl-tests/23-srp.conf.in | 154 ++++++++++++++++---------------- test/ssl-tests/protocol_version.pm | 6 +- 10 files changed, 145 insertions(+), 101 deletions(-) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index f4860ea1fd..d51772fa69 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2169,8 +2169,7 @@ __owur int ssl_check_version_downgrade(SSL *s); __owur int ssl_set_version_bound(int method_version, int version, int *bound); __owur int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello); __owur int ssl_choose_client_version(SSL *s, int version); -int ssl_get_client_min_max_version(const SSL *s, int *min_version, - int *max_version); +int ssl_get_min_max_version(const SSL *s, int *min_version, int *max_version); __owur long tls1_default_timeout(void); __owur int dtls1_do_write(SSL *s, int type); diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 8550dfe22e..cb420e8a0c 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -639,7 +639,7 @@ int tls_construct_extensions(SSL *s, WPACKET *pkt, unsigned int context, } if ((context & EXT_CLIENT_HELLO) != 0) { - reason = ssl_get_client_min_max_version(s, &min_version, &max_version); + reason = ssl_get_min_max_version(s, &min_version, &max_version); if (reason != 0) { SSLerr(SSL_F_TLS_CONSTRUCT_EXTENSIONS, reason); goto err; diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index c6cd0ce8a3..5f33e2ceb8 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -464,7 +464,7 @@ int tls_construct_ctos_supported_versions(SSL *s, WPACKET *pkt, return 0; } - reason = ssl_get_client_min_max_version(s, &min_version, &max_version); + reason = ssl_get_min_max_version(s, &min_version, &max_version); if (reason != 0) { SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_VERSIONS, reason); return 0; diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 36c96e5697..ab72788224 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -78,6 +78,39 @@ int tls_setup_handshake(SSL *s) return 0; if (s->server) { + STACK_OF(SSL_CIPHER) *ciphers = SSL_get_ciphers(s); + int i, ver_min, ver_max, ok = 0; + + /* + * Sanity check that the maximum version we accept has ciphers + * enabled. For clients we do this check during construction of the + * ClientHello. + */ + if (ssl_get_min_max_version(s, &ver_min, &ver_max) != 0) { + SSLerr(SSL_F_TLS_SETUP_HANDSHAKE, ERR_R_INTERNAL_ERROR); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); + return 0; + } + for (i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { + const SSL_CIPHER *c = sk_SSL_CIPHER_value(ciphers, i); + + if (SSL_IS_DTLS(s)) { + if (DTLS_VERSION_GE(ver_max, c->min_dtls) && + DTLS_VERSION_LE(ver_max, c->max_dtls)) + ok = 1; + } else if (ver_max >= c->min_tls && ver_max <= c->max_tls) { + ok = 1; + } + if (ok) + break; + } + if (!ok) { + SSLerr(SSL_F_TLS_SETUP_HANDSHAKE, SSL_R_NO_CIPHERS_AVAILABLE); + ERR_add_error_data(1, "No ciphers enabled for max supported " + "SSL/TLS version"); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); + return 0; + } if (SSL_IS_FIRST_HANDSHAKE(s)) { s->ctx->stats.sess_accept++; } else if (!s->s3->send_connection_binding && @@ -1714,7 +1747,7 @@ int ssl_choose_client_version(SSL *s, int version) } /* - * ssl_get_client_min_max_version - get minimum and maximum client version + * ssl_get_min_max_version - get minimum and maximum protocol version * @s: The SSL connection * @min_version: The minimum supported version * @max_version: The maximum supported version @@ -1732,8 +1765,7 @@ int ssl_choose_client_version(SSL *s, int version) * Returns 0 on success or an SSL error reason number on failure. On failure * min_version and max_version will also be set to 0. */ -int ssl_get_client_min_max_version(const SSL *s, int *min_version, - int *max_version) +int ssl_get_min_max_version(const SSL *s, int *min_version, int *max_version) { int version; int hole; @@ -1827,7 +1859,7 @@ int ssl_set_client_hello_version(SSL *s) { int ver_min, ver_max, ret; - ret = ssl_get_client_min_max_version(s, &ver_min, &ver_max); + ret = ssl_get_min_max_version(s, &ver_min, &ver_max); if (ret != 0) return ret; diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 83e493eb7c..6ff3363300 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1013,7 +1013,7 @@ void ssl_set_client_disabled(SSL *s) s->s3->tmp.mask_a = 0; s->s3->tmp.mask_k = 0; ssl_set_sig_mask(&s->s3->tmp.mask_a, s, SSL_SECOP_SIGALG_MASK); - ssl_get_client_min_max_version(s, &s->s3->tmp.min_ver, &s->s3->tmp.max_ver); + ssl_get_min_max_version(s, &s->s3->tmp.min_ver, &s->s3->tmp.max_ver); #ifndef OPENSSL_NO_PSK /* with PSK there must be client callback set */ if (!s->psk_client_callback) { diff --git a/test/recipes/70-test_sslmessages.t b/test/recipes/70-test_sslmessages.t index 790b3aeda2..a6278dc630 100644 --- a/test/recipes/70-test_sslmessages.t +++ b/test/recipes/70-test_sslmessages.t @@ -396,6 +396,7 @@ SKIP: { skip "No EC support in this OpenSSL build", 1 if disabled("ec"); $proxy->clear(); $proxy->clientflags("-no_tls1_3"); + $proxy->serverflags("-no_tls1_3"); $proxy->ciphers("ECDHE-RSA-AES128-SHA"); $proxy->start(); checkhandshake($proxy, checkhandshake::EC_HANDSHAKE, diff --git a/test/ssl-tests/02-protocol-version.conf b/test/ssl-tests/02-protocol-version.conf index d5e0779156..41fa8ca17a 100644 --- a/test/ssl-tests/02-protocol-version.conf +++ b/test/ssl-tests/02-protocol-version.conf @@ -700,7 +700,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-0] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -850,7 +850,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-6] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -1314,7 +1314,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-24] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -1339,7 +1339,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-25] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -4759,7 +4759,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-156] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -4915,7 +4915,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-162] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -5397,7 +5397,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-180] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -5423,7 +5423,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-181] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -17393,7 +17393,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-624] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -17549,7 +17549,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-630] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -18031,7 +18031,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-648] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -18057,7 +18057,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-649] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -18082,7 +18082,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-650] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -18232,7 +18232,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-656] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -18696,7 +18696,7 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-674] -ExpectedResult = InternalError +ExpectedResult = ClientFail # =========================================================== @@ -18721,6 +18721,6 @@ VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [test-675] -ExpectedResult = InternalError +ExpectedResult = ClientFail diff --git a/test/ssl-tests/23-srp.conf b/test/ssl-tests/23-srp.conf index 6ae49e6814..610a0bb08a 100644 --- a/test/ssl-tests/23-srp.conf +++ b/test/ssl-tests/23-srp.conf @@ -18,6 +18,7 @@ client = 0-srp-client [0-srp-server] Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem CipherString = SRP +MaxProtocol = TLSv1.2 PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [0-srp-client] @@ -52,6 +53,7 @@ client = 1-srp-bad-password-client [1-srp-bad-password-server] Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem CipherString = SRP +MaxProtocol = TLSv1.2 PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [1-srp-bad-password-client] @@ -86,6 +88,7 @@ client = 2-srp-auth-client [2-srp-auth-server] Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem CipherString = aSRP +MaxProtocol = TLSv1.2 PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [2-srp-auth-client] @@ -120,6 +123,7 @@ client = 3-srp-auth-bad-password-client [3-srp-auth-bad-password-server] Certificate = ${ENV::TEST_CERTS_DIR}/servercert.pem CipherString = aSRP +MaxProtocol = TLSv1.2 PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [3-srp-auth-bad-password-client] diff --git a/test/ssl-tests/23-srp.conf.in b/test/ssl-tests/23-srp.conf.in index b7601fc3e5..dcbd9f4ff9 100644 --- a/test/ssl-tests/23-srp.conf.in +++ b/test/ssl-tests/23-srp.conf.in @@ -15,89 +15,93 @@ package ssltests; our @tests = ( { - name => "srp", - server => { - "CipherString" => "SRP", - extra => { - "SRPUser" => "user", - "SRPPassword" => "password", - }, + name => "srp", + server => { + "CipherString" => "SRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "password", + }, + }, + client => { + "CipherString" => "SRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "password", + }, + }, + test => { + "ExpectedResult" => "Success" }, - client => { - "CipherString" => "SRP", - "MaxProtocol" => "TLSv1.2", - extra => { - "SRPUser" => "user", - "SRPPassword" => "password", - }, - }, - test => { - "ExpectedResult" => "Success" - }, }, { - name => "srp-bad-password", - server => { - "CipherString" => "SRP", - extra => { - "SRPUser" => "user", - "SRPPassword" => "password", - }, + name => "srp-bad-password", + server => { + "CipherString" => "SRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "password", + }, + }, + client => { + "CipherString" => "SRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "passw0rd", + }, + }, + test => { + # Server fails first with bad client Finished. + "ExpectedResult" => "ServerFail" }, - client => { - "CipherString" => "SRP", - "MaxProtocol" => "TLSv1.2", - extra => { - "SRPUser" => "user", - "SRPPassword" => "passw0rd", - }, - }, - test => { - # Server fails first with bad client Finished. - "ExpectedResult" => "ServerFail" - }, }, { - name => "srp-auth", - server => { - "CipherString" => "aSRP", - extra => { - "SRPUser" => "user", - "SRPPassword" => "password", - }, + name => "srp-auth", + server => { + "CipherString" => "aSRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "password", + }, + }, + client => { + "CipherString" => "aSRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "password", + }, + }, + test => { + "ExpectedResult" => "Success" }, - client => { - "CipherString" => "aSRP", - "MaxProtocol" => "TLSv1.2", - extra => { - "SRPUser" => "user", - "SRPPassword" => "password", - }, - }, - test => { - "ExpectedResult" => "Success" - }, }, { - name => "srp-auth-bad-password", - server => { - "CipherString" => "aSRP", - extra => { - "SRPUser" => "user", - "SRPPassword" => "password", - }, + name => "srp-auth-bad-password", + server => { + "CipherString" => "aSRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "password", + }, + }, + client => { + "CipherString" => "aSRP", + "MaxProtocol" => "TLSv1.2", + extra => { + "SRPUser" => "user", + "SRPPassword" => "passw0rd", + }, + }, + test => { + # Server fails first with bad client Finished. + "ExpectedResult" => "ServerFail" }, - client => { - "CipherString" => "aSRP", - "MaxProtocol" => "TLSv1.2", - extra => { - "SRPUser" => "user", - "SRPPassword" => "passw0rd", - }, - }, - test => { - # Server fails first with bad client Finished. - "ExpectedResult" => "ServerFail" - }, }, -); \ No newline at end of file +); diff --git a/test/ssl-tests/protocol_version.pm b/test/ssl-tests/protocol_version.pm index 7c28bcf0f6..ef92275257 100644 --- a/test/ssl-tests/protocol_version.pm +++ b/test/ssl-tests/protocol_version.pm @@ -242,7 +242,11 @@ sub expected_result { $c_max = min $c_max, $max_enabled; $s_max = min $s_max, $max_enabled; - if ($c_min > $c_max) { + if ($c_min > $c_max && $s_min > $s_max) { + # Client will fail to send a hello and server will fail to start. The + # client failed first so this is reported as ClientFail. + return ("ClientFail", undef); + } elsif ($c_min > $c_max) { # Client should fail to even send a hello. # This results in an internal error since the server will be # waiting for input that never arrives. -- cgit v1.2.3