ext/pre_shared_key: cleanups in error handlingtmp-psk-parsing-fixes

This addresses a memory leak found via oss-fuzz. It also
sets the right index on the selected PSK, and returns the
right server error code on incorrect key file.

Addresses:
  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=7465

Signed-off-by: Nikos Mavrogiannopoulos <nmav@gnutls.org>

2 files changed, 39 insertions, 17 deletions

diff --git a/lib/ext/pre_shared_key.c b/lib/ext/pre_shared_key.c
index d4ea982cbb..d91359883e 100644
--- a/lib/ext/pre_shared_key.c
+++ b/lib/ext/pre_shared_key.c
@@ -257,8 +257,8 @@ static int server_recv_params(gnutls_session_t session,
 	uint8_t binder_value[MAX_HASH_SIZE];
 	int psk_index = -1;
 	gnutls_datum_t binder_recvd = { NULL, 0 };
-	gnutls_datum_t key;
-	unsigned hash_size;
+	gnutls_datum_t key = {NULL, 0};
+	unsigned hash_size, ctr;
 	psk_ext_parser_st psk_parser;
 	struct psk_st psk;
 	psk_auth_info_t info;
@@ -270,6 +270,7 @@ static int server_recv_params(gnutls_session_t session,
 		return gnutls_assert_val(ret);
 	}
 
+	ctr = 0;
 	while ((ret = _gnutls13_psk_ext_parser_next_psk(&psk_parser, &psk)) >= 0) {
 		if (psk.ob_ticket_age == 0) {
 			/* _gnutls_psk_pwd_find_entry() expects 0-terminated identities */
@@ -280,10 +281,14 @@ static int server_recv_params(gnutls_session_t session,
 				identity_str[psk.identity.size] = 0;
 
 				ret = _gnutls_psk_pwd_find_entry(session, identity_str, &key);
-				if (ret == 0)
-					psk_index = ret;
+				if (ret < 0)
+					return gnutls_assert_val(ret);
+
+				psk_index = ctr;
+				break;
 			}
 		}
+		ctr++;
 	}
 
 	if (psk_index < 0)
@@ -291,12 +296,17 @@ static int server_recv_params(gnutls_session_t session,
 
 	ret = _gnutls13_psk_ext_parser_find_binder(&psk_parser, psk_index,
 						   &binder_recvd);
-	if (ret < 0)
-		return gnutls_assert_val(ret);
+	if (ret < 0) {
+		gnutls_assert();
+		goto fail;
+	}
 
 	/* Get full ClientHello */
-	if (!_gnutls_ext_get_full_client_hello(session, &full_client_hello))
-		return 0;
+	if (!_gnutls_ext_get_full_client_hello(session, &full_client_hello)) {
+		ret = GNUTLS_E_INTERNAL_ERROR;
+		gnutls_assert();
+		goto fail;
+	}
 
 	/* Compute the binder value for this PSK */
 	prf = pskcred->binder_algo;
@@ -304,13 +314,16 @@ static int server_recv_params(gnutls_session_t session,
 	ret = compute_psk_binder(GNUTLS_SERVER, prf, hash_size, hash_size, 0, 0,
 				 &key, &full_client_hello,
 				 binder_value);
-	if (ret < 0)
-		return gnutls_assert_val(ret);
+	if (ret < 0) {
+		gnutls_assert();
+		goto fail;
+	}
 
 	if (_gnutls_mac_get_algo_len(prf) != binder_recvd.size ||
 	    safe_memcmp(binder_value, binder_recvd.data, binder_recvd.size)) {
-		gnutls_free(key.data);
-		return gnutls_assert_val(GNUTLS_E_RECEIVED_ILLEGAL_PARAMETER);
+		gnutls_assert();
+		ret = GNUTLS_E_RECEIVED_ILLEGAL_PARAMETER;
+		goto fail;
 	}
 
 	if (session->internals.hsk_flags & HSK_PSK_KE_MODE_DHE_PSK)
@@ -323,12 +336,17 @@ static int server_recv_params(gnutls_session_t session,
 	/* save the username in psk_auth_info to make it available
 	 * using gnutls_psk_server_get_username() */
 	if (psk.ob_ticket_age == 0) {
-		if (psk.identity.size >= sizeof(info->username))
-			return gnutls_assert_val(GNUTLS_E_RECEIVED_ILLEGAL_PARAMETER);
+		if (psk.identity.size >= sizeof(info->username)) {
+			gnutls_assert();
+			ret = GNUTLS_E_RECEIVED_ILLEGAL_PARAMETER;
+			goto fail;
+		}
 
 		ret = _gnutls_auth_info_set(session, GNUTLS_CRD_PSK, sizeof(psk_auth_info_st), 1);
-		if (ret < 0)
-			return gnutls_assert_val(ret);
+		if (ret < 0) {
+			gnutls_assert();
+			goto fail;
+		}
 
 		info = _gnutls_get_auth_info(session, GNUTLS_CRD_PSK);
 		assert(info != NULL);
@@ -348,6 +366,10 @@ static int server_recv_params(gnutls_session_t session,
 	session->key.proto.tls13.binder_prf = prf;
 
 	return 0;
+
+ fail:
+	gnutls_free(key.data);
+	return ret;
 }
 
 /*
diff --git a/tests/psk-file.c b/tests/psk-file.c
index a6df3f0467..a73031193f 100644
--- a/tests/psk-file.c
+++ b/tests/psk-file.c
@@ -389,7 +389,7 @@ void doit(void)
 	run_test2("NORMAL:-VERS-ALL:+VERS-TLS1.3:+DHE-PSK:-GROUP-ALL", "NORMAL:-VERS-ALL:+VERS-TLS1.3:+DHE-PSK:+PSK:-GROUP-ALL", "jas", &key, 0, 0, GNUTLS_E_FATAL_ALERT_RECEIVED, GNUTLS_E_NO_COMMON_KEY_SHARE);
 
 	/* if user invalid we continue without PSK */
-	run_test2("NORMAL:-VERS-ALL:+VERS-TLS1.3:+PSK:+DHE-PSK", NULL, "non-hex", &key, 0, 0, GNUTLS_E_FATAL_ALERT_RECEIVED, GNUTLS_E_INSUFFICIENT_CREDENTIALS);
+	run_test2("NORMAL:-VERS-ALL:+VERS-TLS1.3:+PSK:+DHE-PSK", NULL, "non-hex", &key, 0, 0, GNUTLS_E_FATAL_ALERT_RECEIVED, GNUTLS_E_KEYFILE_ERROR);
 	run_test2("NORMAL:-VERS-ALL:+VERS-TLS1.3:+PSK:+DHE-PSK", NULL, "unknown", &key, 0, 0, GNUTLS_E_FATAL_ALERT_RECEIVED, GNUTLS_E_RECEIVED_ILLEGAL_PARAMETER);
 	run_test2("NORMAL:-VERS-ALL:+VERS-TLS1.3:+PSK:+DHE-PSK", NULL, "jas", &wrong_key, 0, 0, GNUTLS_E_FATAL_ALERT_RECEIVED, GNUTLS_E_RECEIVED_ILLEGAL_PARAMETER);
 }