From 38a7315060ec4ca49799b2a7ea83e8678e3acd20 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 26 Apr 2017 11:28:20 +0100 Subject: [PATCH] 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: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/3316) --- 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/23-srp.conf | 4 + test/ssl-tests/23-srp.conf.in | 154 +++++++++++++++-------------- 8 files changed, 124 insertions(+), 84 deletions(-) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 8eb6ff52c9..15065c7d98 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2194,8 +2194,7 @@ __owur int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, DOWNGRADE *dgrd); __owur int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al); -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 7ec7128172..c8ed722ab1 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -674,7 +674,7 @@ int tls_construct_extensions(SSL *s, WPACKET *pkt, unsigned int context, } if ((context & SSL_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 7d2a4b04a5..898992d99b 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 01804458fa..36d55343b8 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 && @@ -1781,7 +1814,7 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) } /* - * 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 @@ -1799,8 +1832,7 @@ int ssl_choose_client_version(SSL *s, int version, int checkdgrd, int *al) * 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; @@ -1894,7 +1926,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 5007f7ebb3..0e1a97e175 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/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 +); -- GitLab