_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 <nmav@redhat.com>

7 files changed, 259 insertions, 58 deletions

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 <http://www.gnu.org/licenses/>
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+/* This program tests the ciphersuite negotiation for various key exchange
+ * methods and options under TLS1.3. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <gnutls/gnutls.h>
+#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<sizeof(tests)/sizeof(tests[0]);i++) {
+		try(&tests[i]);
+	}
+
+	gnutls_global_deinit();
+}