From 3495f992b8b4cd50f1136edcc2f66b53e701980d Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Fri, 9 Mar 2018 12:12:56 +0100 Subject: _gnutls_supported_ecc_recv_params: take into account precedence That is, when %SERVER_PRECEDENCE is given in the priority string make sure that the negotiated curve of DH group respects the server's priorities. That's very relevant under TLS1.3 as ciphersuite negotiation itself, where %SERVER_PRECEDENCE applied, does contain only the cipher algorithm and MAC unlike TLS1.2 which included key exchange as well. Resolves #378 Signed-off-by: Nikos Mavrogiannopoulos --- lib/algorithms/ciphersuites.c | 2 +- lib/ext/ecc.c | 78 ++++++++++++++++++----- lib/ext/key_share.c | 67 +++++++++----------- lib/gnutls_int.h | 5 +- tests/Makefile.am | 2 + tests/cipher-neg-common.c | 20 +++++- tests/tls13-cipher-neg.c | 143 ++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 259 insertions(+), 58 deletions(-) create mode 100644 tests/tls13-cipher-neg.c diff --git a/lib/algorithms/ciphersuites.c b/lib/algorithms/ciphersuites.c index a541925029..063363b5bf 100644 --- a/lib/algorithms/ciphersuites.c +++ b/lib/algorithms/ciphersuites.c @@ -1458,7 +1458,7 @@ _gnutls_figure_common_ciphersuite(gnutls_session_t session, * we should assume that SECP256R1 is supported; that is required * by RFC4492, probably to allow SSLv2 hellos negotiate elliptic curve * ciphersuites */ - if (session->internals.cand_ec_group == NULL && + if (!version->tls13_sem && session->internals.cand_ec_group == NULL && !_gnutls_hello_ext_is_present(session, GNUTLS_EXTENSION_SUPPORTED_ECC)) { session->internals.cand_ec_group = _gnutls_id_to_group(DEFAULT_EC_GROUP); } diff --git a/lib/ext/ecc.c b/lib/ext/ecc.c index 58cf3d86b2..50cd862211 100644 --- a/lib/ext/ecc.c +++ b/lib/ext/ecc.c @@ -115,7 +115,7 @@ static int _gnutls_supported_ecc_recv_params(gnutls_session_t session, const uint8_t * data, size_t _data_size) { - int ret, i; + int i; ssize_t data_size = _data_size; uint16_t len; const uint8_t *p = data; @@ -123,6 +123,9 @@ _gnutls_supported_ecc_recv_params(gnutls_session_t session, unsigned have_ffdhe = 0; unsigned tls_id; unsigned min_dh; + unsigned j; + int serv_ec_idx, serv_dh_idx; /* index in server's priority listing */ + int cli_ec_pos, cli_dh_pos; /* position in listing sent by client */ if (session->security_parameters.entity == GNUTLS_CLIENT) { /* A client shouldn't receive this extension in TLS1.2. It is @@ -148,11 +151,15 @@ _gnutls_supported_ecc_recv_params(gnutls_session_t session, /* we figure what is the minimum DH allowed for this session, if any */ min_dh = get_min_dh(session); - /* This is being processed prior to a ciphersuite being selected */ + serv_ec_idx = serv_dh_idx = -1; + cli_ec_pos = cli_dh_pos = -1; + + /* This extension is being processed prior to a ciphersuite being selected, + * so we cannot rely on ciphersuite information. */ for (i = 0; i < len; i += 2) { - if (have_ffdhe == 0 && p[i] == 0x01) { + if (have_ffdhe == 0 && p[i] == 0x01) have_ffdhe = 1; - } + tls_id = _gnutls_read_uint16(&p[i]); group = _gnutls_tls_id_to_group(tls_id); @@ -160,25 +167,62 @@ _gnutls_supported_ecc_recv_params(gnutls_session_t session, if (group == NULL) continue; - /* if a DH group and less than expected ignore */ if (min_dh > 0 && group->prime && group->prime->size*8 < min_dh) continue; - /* Check if we support this group */ - if ((ret = - _gnutls_session_supports_group(session, - group->id)) - < 0) { - group = NULL; - continue; - } else { - if (group->pk == GNUTLS_PK_DH && session->internals.cand_dh_group == NULL) - session->internals.cand_dh_group = group; - else if (group->pk != GNUTLS_PK_DH && session->internals.cand_ec_group == NULL) - session->internals.cand_ec_group = group; + /* we simulate _gnutls_session_supports_group, but we prioritize if + * %SERVER_PRECEDENCE is given */ + for (j = 0; j < session->internals.priorities->groups.size; j++) { + if (session->internals.priorities->groups.entry[j]->id == group->id) { + if (session->internals.priorities->server_precedence) { + if (group->pk == GNUTLS_PK_DH) { + if (serv_dh_idx != -1 && (int)j > serv_dh_idx) + break; + serv_dh_idx = j; + cli_dh_pos = i; + } else { + if (serv_ec_idx != -1 && (int)j > serv_ec_idx) + break; + serv_ec_idx = j; + cli_ec_pos = i; + } + } else { + if (group->pk == GNUTLS_PK_DH) { + if (cli_dh_pos != -1) + break; + cli_dh_pos = i; + serv_dh_idx = j; + } else { + if (cli_ec_pos != -1) + break; + cli_ec_pos = i; + serv_ec_idx = j; + } + } + break; + } } } + /* serv_dh/ec_pos contain the index of the groups we want to use. + */ + if (serv_dh_idx != -1) { + session->internals.cand_dh_group = session->internals.priorities->groups.entry[serv_dh_idx]; + session->internals.cand_group = session->internals.cand_dh_group; + } + + if (serv_ec_idx != -1) { + session->internals.cand_ec_group = session->internals.priorities->groups.entry[serv_ec_idx]; + if (session->internals.cand_group == NULL || + (session->internals.priorities->server_precedence && serv_ec_idx < serv_dh_idx) || + (!session->internals.priorities->server_precedence && cli_ec_pos < cli_dh_pos)) { + session->internals.cand_group = session->internals.cand_ec_group; + } + } + + if (session->internals.cand_group) + _gnutls_handshake_log("EXT[%p]: Selected group %s\n", session, session->internals.cand_group->name); + if (have_ffdhe) session->internals.hsk_flags |= HSK_HAVE_FFDHE; } diff --git a/lib/ext/key_share.c b/lib/ext/key_share.c index f110e10268..f9403df838 100644 --- a/lib/ext/key_share.c +++ b/lib/ext/key_share.c @@ -489,9 +489,10 @@ key_share_recv_params(gnutls_session_t session, int ret; ssize_t data_size = _data_size; ssize_t size; - unsigned gid, used_share = 0; + unsigned gid; const version_entry_st *ver; - const gnutls_group_entry_st *group, *sgroup = NULL; + const gnutls_group_entry_st *group; + unsigned used_share = 0; if (session->security_parameters.entity == GNUTLS_SERVER) { ver = get_version(session); @@ -523,46 +524,38 @@ key_share_recv_params(gnutls_session_t session, if (group != NULL) _gnutls_handshake_log("EXT[%p]: Received key share for %s\n", session, group->name); - if (group != NULL) { - if (group == session->internals.cand_ec_group) - sgroup = group; - else if (group == session->internals.cand_dh_group) - sgroup = group; - } + if (group != NULL && group == session->internals.cand_group) { + _gnutls_session_group_set(session, group); - if (sgroup == NULL) { - data += size; - continue; - } + ret = server_use_key_share(session, group, data, size); + if (ret < 0) + return gnutls_assert_val(ret); - _gnutls_session_group_set(session, sgroup); + used_share = 1; + break; - ret = server_use_key_share(session, sgroup, data, size); - if (ret < 0) { - return gnutls_assert_val(ret); } - used_share = 1; - break; + data += size; + continue; } - if (used_share == 0) { - /* we utilize GNUTLS_E_NO_COMMON_KEY_SHARE for: - * 1. signal for hello-retry-request in the handshake - * layer during first client hello parsing (server side - here). - * This does not result to error code being - * propagated to app layer. - * 2. Propagate to application error code that no - * common key share was found after an HRR was - * received (client side) - * 3. Propagate to application error code that no - * common key share was found after an HRR was - * sent (server side). - * In cases (2,3) the error is translated to illegal - * parameter alert. - */ + /* we utilize GNUTLS_E_NO_COMMON_KEY_SHARE for: + * 1. signal for hello-retry-request in the handshake + * layer during first client hello parsing (server side - here). + * This does not result to error code being + * propagated to app layer. + * 2. Propagate to application error code that no + * common key share was found after an HRR was + * received (client side) + * 3. Propagate to application error code that no + * common key share was found after an HRR was + * sent (server side). + * In cases (2,3) the error is translated to illegal + * parameter alert. + */ + if (used_share == 0) return gnutls_assert_val(GNUTLS_E_NO_COMMON_KEY_SHARE); - } } else { /* Client */ ver = get_version(session); @@ -712,10 +705,8 @@ key_share_send_params(gnutls_session_t session, return gnutls_assert_val(0); if (_gnutls_ext_get_msg(session) == GNUTLS_EXT_FLAG_HRR) { - if (session->internals.cand_ec_group != NULL) - group = session->internals.cand_ec_group; - else - group = session->internals.cand_dh_group; + group = session->internals.cand_group; + if (group == NULL) return gnutls_assert_val(GNUTLS_E_NO_COMMON_KEY_SHARE); diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h index 1d75c4a09f..e926b3d0fe 100644 --- a/lib/gnutls_int.h +++ b/lib/gnutls_int.h @@ -1272,9 +1272,12 @@ typedef struct { * receive size */ unsigned max_recv_size; - /* candidate groups to be selected for security params groups */ + /* candidate groups to be selected for security params groups, they are + * prioritized in isolation under TLS1.2 */ const gnutls_group_entry_st *cand_ec_group; const gnutls_group_entry_st *cand_dh_group; + /* used under TLS1.3+ */ + const gnutls_group_entry_st *cand_group; /* the ciphersuite received in HRR */ uint8_t hrr_cs[2]; diff --git a/tests/Makefile.am b/tests/Makefile.am index 18814bf3f5..5abd976ff8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -118,6 +118,8 @@ ctests += tls13/ocsp-client ctests += tls13/change_cipher_spec +ctests += tls13-cipher-neg + ctests += mini-record-2 simple gnutls_hmac_fast set_pkcs12_cred cert certuniqueid tls-neg-ext-key \ mpi certificate_set_x509_crl dn parse_ca x509-dn x509-dn-decode record-sizes \ hostname-check cve-2008-4989 pkcs12_s2k chainverify record-sizes-range \ diff --git a/tests/cipher-neg-common.c b/tests/cipher-neg-common.c index 1ddbad612d..a855147359 100644 --- a/tests/cipher-neg-common.c +++ b/tests/cipher-neg-common.c @@ -23,6 +23,7 @@ typedef struct test_case_st { const char *name; int cipher; + int group; const char *client_prio; const char *server_prio; unsigned not_on_fips; @@ -79,11 +80,28 @@ static void try(test_case_st *test) } if (cret != test->cipher) { - fail("%s: negotiated cipher diffs the expected (%s, %s)!\n", + fail("%s: negotiated cipher differs with the expected (%s, %s)!\n", test->name, gnutls_cipher_get_name(cret), gnutls_cipher_get_name(test->cipher)); } + if (test->group) { + sret = gnutls_group_get(client); + cret = gnutls_group_get(server); + + if (sret != cret) { + fail("%s: client negotiated different group than server (%s, %s)!\n", + test->name, gnutls_group_get_name(cret), + gnutls_group_get_name(sret)); + } + + if (cret != test->group) { + fail("%s: negotiated group differs with the expected (%s, %s)!\n", + test->name, gnutls_group_get_name(cret), + gnutls_group_get_name(test->group)); + } + } + gnutls_deinit(server); gnutls_deinit(client); gnutls_certificate_free_credentials(s_cert_cred); diff --git a/tests/tls13-cipher-neg.c b/tests/tls13-cipher-neg.c new file mode 100644 index 0000000000..ea9df13142 --- /dev/null +++ b/tests/tls13-cipher-neg.c @@ -0,0 +1,143 @@ +/* + * Copyright (C) 2017-2018 Red Hat, Inc. + * + * Author: Nikos Mavrogiannopoulos + * + * This file is part of GnuTLS. + * + * GnuTLS is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * GnuTLS is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see + */ + +#ifdef HAVE_CONFIG_H +#include +#endif + +/* This program tests the ciphersuite negotiation for various key exchange + * methods and options under TLS1.3. */ + +#include +#include +#include +#include +#include +#include "utils.h" +#include "cert-common.h" +#include "eagain-common.h" + +#include "cipher-neg-common.c" + +/* We remove the ECDHE and DHE key exchanges as they impose additional + * rules in the sorting of groups. + */ +#define SPRIO "NORMAL:-VERS-TLS-ALL:+VERS-TLS1.3:-ECDHE-RSA:-ECDHE-ECDSA:-DHE-RSA:-RSA:-DHE-DSS" +#define CPRIO "NORMAL:-VERS-TLS-ALL:+VERS-TLS1.3:+VERS-TLS1.2:-ECDHE-RSA:-ECDHE-ECDSA:-DHE-RSA:-RSA:-DHE-DSS" + +test_case_st tests[] = { + { + .name = "server TLS 1.3: AES-128-GCM with SECP256R1 (server)", + .cipher = GNUTLS_CIPHER_AES_128_GCM, + .group = GNUTLS_GROUP_SECP256R1, + .server_prio = SPRIO":-CIPHER-ALL:+AES-128-GCM:+CIPHER-ALL:%SERVER_PRECEDENCE:-GROUP-ALL:+GROUP-SECP256R1:+GROUP-ALL", + .client_prio = CPRIO":+AES-128-GCM:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-SECP384R1:+GROUP-SECP521R1:+GROUP-SECP256R1" + }, + { + .name = "both TLS 1.3: AES-128-GCM with X25519 (server)", + .cipher = GNUTLS_CIPHER_AES_128_GCM, + .group = GNUTLS_GROUP_X25519, + .server_prio = SPRIO":-CIPHER-ALL:+AES-128-GCM:+CIPHER-ALL:%SERVER_PRECEDENCE:-GROUP-ALL:+GROUP-X25519:+GROUP-ALL", + .client_prio = CPRIO":+AES-128-GCM:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-SECP384R1:+GROUP-SECP521R1:+GROUP-SECP256R1:+GROUP-ALL" + }, + { + .name = "client TLS 1.3: AES-128-GCM with SECP256R1 (client)", + .cipher = GNUTLS_CIPHER_AES_128_GCM, + .group = GNUTLS_GROUP_SECP256R1, + .server_prio = SPRIO":+AES-128-GCM:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-SECP384R1:+GROUP-SECP521R1:+GROUP-SECP256R1", + .client_prio = CPRIO":-CIPHER-ALL:+AES-128-GCM:+CIPHER-ALL:-VERS-ALL:+VERS-TLS1.3:-GROUP-ALL:+GROUP-SECP256R1:+GROUP-ALL" + }, + { + .name = "both TLS 1.3: AES-128-GCM with X25519 (client)", + .cipher = GNUTLS_CIPHER_AES_128_GCM, + .group = GNUTLS_GROUP_X25519, + .server_prio = SPRIO":+AES-128-GCM:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-SECP384R1:+GROUP-SECP521R1:+GROUP-SECP256R1:+GROUP-ALL", + .client_prio = CPRIO":-CIPHER-ALL:+AES-128-GCM:+CIPHER-ALL:-VERS-ALL:+VERS-TLS1.3:-GROUP-ALL:+GROUP-X25519:+GROUP-ALL" + }, + { + .name = "server TLS 1.3: AES-128-CCM and FFDHE2048 (server)", + .cipher = GNUTLS_CIPHER_AES_128_CCM, + .group = GNUTLS_GROUP_FFDHE2048, + .server_prio = SPRIO":-CIPHER-ALL:+AES-128-CCM:+CIPHER-ALL:%SERVER_PRECEDENCE:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-ALL", + .client_prio = CPRIO":+AES-128-CCM" + }, + { + .name = "both TLS 1.3: AES-128-CCM and FFDHE 2048 (server)", + .cipher = GNUTLS_CIPHER_AES_128_CCM, + .group = GNUTLS_GROUP_FFDHE2048, + .server_prio = SPRIO":-CIPHER-ALL:+AES-128-CCM:+CIPHER-ALL:%SERVER_PRECEDENCE:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-ALL", + .client_prio = CPRIO":+AES-128-CCM:+VERS-TLS1.3" + }, + { + .name = "client TLS 1.3: AES-128-CCM and FFDHE 2048 (client)", + .cipher = GNUTLS_CIPHER_AES_128_CCM, + .group = GNUTLS_GROUP_FFDHE2048, + .server_prio = SPRIO":+AES-128-CCM", + .client_prio = CPRIO":-CIPHER-ALL:+AES-128-CCM:+CIPHER-ALL:-VERS-ALL:+VERS-TLS1.3:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-ALL" + }, + { + .name = "both TLS 1.3: AES-128-CCM and FFDHE 2048 (client)", + .cipher = GNUTLS_CIPHER_AES_128_CCM, + .group = GNUTLS_GROUP_FFDHE2048, + .server_prio = SPRIO":+AES-128-CCM:+VERS-TLS1.3", + .client_prio = CPRIO":-CIPHER-ALL:+AES-128-CCM:+CIPHER-ALL:-VERS-ALL:+VERS-TLS1.3:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-ALL" + }, + { + .name = "server TLS 1.3: CHACHA20-POLY (server)", + .cipher = GNUTLS_CIPHER_CHACHA20_POLY1305, + .not_on_fips = 1, + .server_prio = SPRIO":-CIPHER-ALL:+CHACHA20-POLY1305:+CIPHER-ALL:%SERVER_PRECEDENCE", + .client_prio = CPRIO":+CHACHA20-POLY1305" + }, + { + .name = "both TLS 1.3: CHACHA20-POLY (server)", + .cipher = GNUTLS_CIPHER_CHACHA20_POLY1305, + .not_on_fips = 1, + .server_prio = SPRIO":-CIPHER-ALL:+CHACHA20-POLY1305:+CIPHER-ALL:%SERVER_PRECEDENCE", + .client_prio = CPRIO":+CHACHA20-POLY1305:+VERS-TLS1.3" + }, + { + .name = "client TLS 1.3: CHACHA20-POLY (client)", + .cipher = GNUTLS_CIPHER_CHACHA20_POLY1305, + .not_on_fips = 1, + .server_prio = SPRIO":+CHACHA20-POLY1305", + .client_prio = CPRIO":-CIPHER-ALL:+CHACHA20-POLY1305:+CIPHER-ALL" + }, + { + .name = "both TLS 1.3: CHACHA20-POLY (client)", + .cipher = GNUTLS_CIPHER_CHACHA20_POLY1305, + .not_on_fips = 1, + .server_prio = SPRIO":+CHACHA20-POLY1305", + .client_prio = CPRIO":-CIPHER-ALL:+CHACHA20-POLY1305:+CIPHER-ALL" + } +}; + +void doit(void) +{ + unsigned i; + global_init(); + + for (i=0;i