From b5b993b2295be98e23fa8bb570b2c38c5bf8aaf3 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 20 Aug 2018 15:12:39 +0100 Subject: [PATCH] Use the same min-max version range on the client consistently We need to ensure that the min-max version range we use when constructing the ClientHello is the same range we use when we validate the version selected by the ServerHello. Otherwise this may appear as a fallback or downgrade. Fixes #6964 Reviewed-by: Viktor Dukhovni (Merged from https://github.com/openssl/openssl/pull/7013) --- ssl/ssl_locl.h | 2 +- ssl/statem/extensions.c | 2 +- ssl/statem/extensions_clnt.c | 2 +- ssl/statem/statem_lib.c | 134 ++++++++++++++++++++--------------- ssl/t1_lib.c | 2 +- 5 files changed, 79 insertions(+), 63 deletions(-) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 362ae1cbe5..e8819e7a28 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2384,7 +2384,7 @@ __owur int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello, __owur int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions); __owur int ssl_get_min_max_version(const SSL *s, int *min_version, - int *max_version); + int *max_version, int *real_max); __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 12c712e8e6..307e6b9d6f 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -810,7 +810,7 @@ int tls_construct_extensions(SSL *s, WPACKET *pkt, unsigned int context, } if ((context & SSL_EXT_CLIENT_HELLO) != 0) { - reason = ssl_get_min_max_version(s, &min_version, &max_version); + reason = ssl_get_min_max_version(s, &min_version, &max_version, NULL); if (reason != 0) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_EXTENSIONS, reason); diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 2d5b60a737..4b5e6fe2b8 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -507,7 +507,7 @@ EXT_RETURN tls_construct_ctos_supported_versions(SSL *s, WPACKET *pkt, { int currv, min_version, max_version, reason; - reason = ssl_get_min_max_version(s, &min_version, &max_version); + reason = ssl_get_min_max_version(s, &min_version, &max_version, NULL); if (reason != 0) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_VERSIONS, reason); diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 38121b7fd2..795202aadc 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -105,7 +105,7 @@ int tls_setup_handshake(SSL *s) * enabled. For clients we do this check during construction of the * ClientHello. */ - if (ssl_get_min_max_version(s, &ver_min, &ver_max) != 0) { + if (ssl_get_min_max_version(s, &ver_min, &ver_max, NULL) != 0) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_SETUP_HANDSHAKE, ERR_R_INTERNAL_ERROR); return 0; @@ -1835,8 +1835,7 @@ int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions) { const version_info *vent; const version_info *table; - int highver = 0; - int origv; + int ret, ver_min, ver_max, real_max, origv; origv = s->version; s->version = version; @@ -1883,65 +1882,62 @@ int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions) break; } - for (vent = table; vent->version != 0; ++vent) { - const SSL_METHOD *method; - int err; - - if (vent->cmeth == NULL) - continue; - - if (highver != 0 && s->version != vent->version) - continue; - - if (highver == 0 && (s->mode & SSL_MODE_SEND_FALLBACK_SCSV) != 0) - highver = vent->version; + ret = ssl_get_min_max_version(s, &ver_min, &ver_max, &real_max); + if (ret != 0) { + s->version = origv; + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, ret); + return 0; + } + if (SSL_IS_DTLS(s) ? DTLS_VERSION_LT(s->version, ver_min) + : s->version < ver_min) { + s->version = origv; + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, SSL_R_UNSUPPORTED_PROTOCOL); + return 0; + } else if (SSL_IS_DTLS(s) ? DTLS_VERSION_GT(s->version, ver_max) + : s->version > ver_max) { + s->version = origv; + SSLfatal(s, SSL_AD_PROTOCOL_VERSION, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, SSL_R_UNSUPPORTED_PROTOCOL); + return 0; + } - method = vent->cmeth(); - err = ssl_method_error(s, method); - if (err != 0) { - if (s->version == vent->version) { - s->version = origv; - SSLfatal(s, SSL_AD_PROTOCOL_VERSION, - SSL_F_SSL_CHOOSE_CLIENT_VERSION, err); - return 0; - } + if ((s->mode & SSL_MODE_SEND_FALLBACK_SCSV) == 0) + real_max = ver_max; - continue; + /* Check for downgrades */ + if (s->version == TLS1_2_VERSION && real_max > s->version) { + if (memcmp(tls12downgrade, + s->s3->server_random + SSL3_RANDOM_SIZE + - sizeof(tls12downgrade), + sizeof(tls12downgrade)) == 0) { + s->version = origv; + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, + SSL_R_INAPPROPRIATE_FALLBACK); + return 0; + } + } else if (!SSL_IS_DTLS(s) + && s->version < TLS1_2_VERSION + && real_max > s->version) { + if (memcmp(tls11downgrade, + s->s3->server_random + SSL3_RANDOM_SIZE + - sizeof(tls11downgrade), + sizeof(tls11downgrade)) == 0) { + s->version = origv; + SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, + SSL_F_SSL_CHOOSE_CLIENT_VERSION, + SSL_R_INAPPROPRIATE_FALLBACK); + return 0; } - if (highver == 0) - highver = vent->version; + } - if (s->version != vent->version) + for (vent = table; vent->version != 0; ++vent) { + if (vent->cmeth == NULL || s->version != vent->version) continue; - /* Check for downgrades */ - if (s->version == TLS1_2_VERSION && highver > s->version) { - if (memcmp(tls12downgrade, - s->s3->server_random + SSL3_RANDOM_SIZE - - sizeof(tls12downgrade), - sizeof(tls12downgrade)) == 0) { - s->version = origv; - SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, - SSL_F_SSL_CHOOSE_CLIENT_VERSION, - SSL_R_INAPPROPRIATE_FALLBACK); - return 0; - } - } else if (!SSL_IS_DTLS(s) - && s->version < TLS1_2_VERSION - && highver > s->version) { - if (memcmp(tls11downgrade, - s->s3->server_random + SSL3_RANDOM_SIZE - - sizeof(tls11downgrade), - sizeof(tls11downgrade)) == 0) { - s->version = origv; - SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, - SSL_F_SSL_CHOOSE_CLIENT_VERSION, - SSL_R_INAPPROPRIATE_FALLBACK); - return 0; - } - } - - s->method = method; + s->method = vent->cmeth(); return 1; } @@ -1956,6 +1952,9 @@ int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions) * @s: The SSL connection * @min_version: The minimum supported version * @max_version: The maximum supported version + * @real_max: The highest version below the lowest compile time version hole + * where that hole lies above at least one run-time enabled + * protocol. * * Work out what version we should be using for the initial ClientHello if the * version is initially (D)TLS_ANY_VERSION. We apply any explicit SSL_OP_NO_xxx @@ -1970,9 +1969,10 @@ int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions) * 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_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 *real_max) { - int version; + int version, tmp_real_max; int hole; const SSL_METHOD *single = NULL; const SSL_METHOD *method; @@ -1989,6 +1989,12 @@ int ssl_get_min_max_version(const SSL *s, int *min_version, int *max_version) * ssl_method_error(s, s->method) */ *min_version = *max_version = s->version; + /* + * Providing a real_max only makes sense where we're using a version + * flexible method. + */ + if (!ossl_assert(real_max == NULL)) + return ERR_R_INTERNAL_ERROR; return 0; case TLS_ANY_VERSION: table = tls_version_table; @@ -2021,6 +2027,9 @@ int ssl_get_min_max_version(const SSL *s, int *min_version, int *max_version) */ *min_version = version = 0; hole = 1; + if (real_max != NULL) + *real_max = 0; + tmp_real_max = 0; for (vent = table; vent->version != 0; ++vent) { /* * A table entry with a NULL client method is still a hole in the @@ -2028,15 +2037,22 @@ int ssl_get_min_max_version(const SSL *s, int *min_version, int *max_version) */ if (vent->cmeth == NULL) { hole = 1; + tmp_real_max = 0; continue; } method = vent->cmeth(); + + if (hole == 1 && tmp_real_max == 0) + tmp_real_max = vent->version; + if (ssl_method_error(s, method) != 0) { hole = 1; } else if (!hole) { single = NULL; *min_version = method->version; } else { + if (real_max != NULL && tmp_real_max != 0) + *real_max = tmp_real_max; version = (single = method)->version; *min_version = version; hole = 0; @@ -2071,7 +2087,7 @@ int ssl_set_client_hello_version(SSL *s) if (!SSL_IS_FIRST_HANDSHAKE(s)) return 0; - ret = ssl_get_min_max_version(s, &ver_min, &ver_max); + ret = ssl_get_min_max_version(s, &ver_min, &ver_max, NULL); if (ret != 0) return ret; diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index df27ba6b7b..ca05a3a55a 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1103,7 +1103,7 @@ int ssl_set_client_disabled(SSL *s) s->s3->tmp.mask_k = 0; ssl_set_sig_mask(&s->s3->tmp.mask_a, s, SSL_SECOP_SIGALG_MASK); if (ssl_get_min_max_version(s, &s->s3->tmp.min_ver, - &s->s3->tmp.max_ver) != 0) + &s->s3->tmp.max_ver, NULL) != 0) return 0; #ifndef OPENSSL_NO_PSK /* with PSK there must be client callback set */ -- 2.25.1