From 29d6298d0b04cfff970b993915db71ba3f580b6d Mon Sep 17 00:00:00 2001                                                                                                                 
From: Daiki Ueno <ueno@gnu.org>
Date: Mon, 23 Oct 2023 09:26:57 +0900
Subject: [PATCH] auth/rsa_psk: side-step potential side-channel

This removes branching that depends on secret data, porting changes
for regular RSA key exchange from
4804febddc2ed958e5ae774de2a8f85edeeff538 and
80a6ce8ddb02477cd724cd5b2944791aaddb702a.  This also removes the
allow_wrong_pms as it was used sorely to control debug output
depending on the branching.

Signed-off-by: Daiki Ueno <ueno@gnu.org>

Conflict::rsa_psk.c

---
 lib/auth/rsa.c     |  2 +-
 lib/auth/rsa_psk.c | 90 ++++++++++++++++++----------------------------
 lib/gnutls_int.h   |  4 ---
 lib/priority.c     |  1 -
 4 files changed, 35 insertions(+), 62 deletions(-)

diff --git a/lib/auth/rsa.c b/lib/auth/rsa.c
index 492ec11..dd0fea1 100644
--- a/lib/auth/rsa.c
+++ b/lib/auth/rsa.c
@@ -206,7 +206,7 @@ proc_rsa_client_kx(gnutls_session_t session, uint8_t * data, size_t _data_size)
 				     session->key.key.size);
 	/* After this point, any conditional on failure that cause differences
 	 * in execution may create a timing or cache access pattern side
-	 * channel that can be used as an oracle, so treat very carefully */
+	 * channel that can be used as an oracle, so tread carefully */
 
 	/* Error handling logic:
 	 * In case decryption fails then don't inform the peer. Just use the
diff --git a/lib/auth/rsa_psk.c b/lib/auth/rsa_psk.c
index c1e9ac4..289ecbd 100644
--- a/lib/auth/rsa_psk.c
+++ b/lib/auth/rsa_psk.c
@@ -255,14 +255,13 @@ _gnutls_proc_rsa_psk_client_kx(gnutls_session_t session, uint8_t * data,
 {
 	gnutls_datum_t username;
 	psk_auth_info_t info;
-	gnutls_datum_t plaintext;
 	gnutls_datum_t ciphertext;
 	gnutls_datum_t pwd_psk = { NULL, 0 };
 	int ret, dsize;
-	int randomize_key = 0;
 	ssize_t data_size = _data_size;
 	gnutls_psk_server_credentials_t cred;
 	gnutls_datum_t premaster_secret = { NULL, 0 };
+	volatile uint8_t ver_maj, ver_min;
 
 	cred = (gnutls_psk_server_credentials_t)
 	    _gnutls_get_cred(session, GNUTLS_CRD_PSK);
@@ -320,68 +319,47 @@ _gnutls_proc_rsa_psk_client_kx(gnutls_session_t session, uint8_t * data,
 	}
 	ciphertext.size = dsize;
 
-	ret =
-	    gnutls_privkey_decrypt_data(session->internals.selected_key, 0,
-					&ciphertext, &plaintext);
-	if (ret < 0 || plaintext.size != GNUTLS_MASTER_SIZE) {
-		/* In case decryption fails then don't inform
-		 * the peer. Just use a random key. (in order to avoid
-		 * attack against pkcs-1 formatting).
-		 */
+	ver_maj = _gnutls_get_adv_version_major(session);
+	ver_min = _gnutls_get_adv_version_minor(session);
+
+	premaster_secret.data = gnutls_malloc(GNUTLS_MASTER_SIZE);
+	if (premaster_secret.data == NULL) {
 		gnutls_assert();
-		_gnutls_debug_log
-		    ("auth_rsa_psk: Possible PKCS #1 format attack\n");
-		if (ret >= 0) {
-			gnutls_free(plaintext.data);
-		}
-		randomize_key = 1;
-	} else {
-		/* If the secret was properly formatted, then
-		 * check the version number.
-		 */
-		if (_gnutls_get_adv_version_major(session) != plaintext.data[0]
-		    || (session->internals.allow_wrong_pms == 0
-			&& _gnutls_get_adv_version_minor(session) !=
-			plaintext.data[1])) {
-			/* No error is returned here, if the version number check
-			 * fails. We proceed normally.
-			 * That is to defend against the attack described in the paper
-			 * "Attacking RSA-based sessions in SSL/TLS" by Vlastimil Klima,
-			 * Ondej Pokorny and Tomas Rosa.
-			 */
-			gnutls_assert();
-			_gnutls_debug_log
-			    ("auth_rsa: Possible PKCS #1 version check format attack\n");
-		}
+		return GNUTLS_E_MEMORY_ERROR;
 	}
+	premaster_secret.size = GNUTLS_MASTER_SIZE;
 
-	if (randomize_key != 0) {
-		premaster_secret.size = GNUTLS_MASTER_SIZE;
-		premaster_secret.data = gnutls_malloc(premaster_secret.size);
-		if (premaster_secret.data == NULL) {
-			gnutls_assert();
-			return GNUTLS_E_MEMORY_ERROR;
-		}
-
-		/* we do not need strong random numbers here.
-		 */
-		ret = gnutls_rnd(GNUTLS_RND_NONCE, premaster_secret.data,
-				 premaster_secret.size);
-		if (ret < 0) {
-			gnutls_assert();
-			goto cleanup;
-		}
-	} else {
-		premaster_secret.data = plaintext.data;
-		premaster_secret.size = plaintext.size;
+	/* Fallback value when decryption fails. Needs to be unpredictable. */
+	ret = gnutls_rnd(GNUTLS_RND_NONCE, premaster_secret.data,
+			 premaster_secret.size);
+	if (ret < 0) {
+		gnutls_assert();
+		goto cleanup;
 	}
 
+	gnutls_privkey_decrypt_data2(session->internals.selected_key, 0,
+				     &ciphertext, premaster_secret.data,
+				     premaster_secret.size);
+	/* After this point, any conditional on failure that cause differences
+	 * in execution may create a timing or cache access pattern side
+	 * channel that can be used as an oracle, so tread carefully */
+
+	/* Error handling logic:
+	 * In case decryption fails then don't inform the peer. Just use the
+	 * random key previously generated. (in order to avoid attack against
+	 * pkcs-1 formatting).
+	 *
+	 * If we get version mismatches no error is returned either. We
+	 * proceed normally. This is to defend against the attack described
+	 * in the paper "Attacking RSA-based sessions in SSL/TLS" by
+	 * Vlastimil Klima, Ondej Pokorny and Tomas Rosa.
+	 */
+
 	/* This is here to avoid the version check attack
 	 * discussed above.
 	 */
-
-	premaster_secret.data[0] = _gnutls_get_adv_version_major(session);
-	premaster_secret.data[1] = _gnutls_get_adv_version_minor(session);
+	premaster_secret.data[0] = ver_maj;
+	premaster_secret.data[1] = ver_min;
 
 	/* find the key of this username
 	 */
diff --git a/lib/gnutls_int.h b/lib/gnutls_int.h
index 969454b..034059a 100644
--- a/lib/gnutls_int.h
+++ b/lib/gnutls_int.h
@@ -977,7 +977,6 @@ struct gnutls_priority_st {
 	bool _no_etm;
 	bool _no_ext_master_secret;
 	bool _allow_key_usage_violation;
-	bool _allow_wrong_pms;
 	bool _dumbfw;
 	unsigned int _dh_prime_bits;	/* old (deprecated) variable */
 
@@ -995,7 +994,6 @@ struct gnutls_priority_st {
 	      (x)->no_etm = 1; \
 	      (x)->no_ext_master_secret = 1; \
 	      (x)->allow_key_usage_violation = 1; \
-	      (x)->allow_wrong_pms = 1; \
 	      (x)->dumbfw = 1
 
 # define ENABLE_PRIO_COMPAT(x) \
@@ -1004,7 +1002,6 @@ struct gnutls_priority_st {
 	      (x)->_no_etm = 1; \
 	      (x)->_no_ext_master_secret = 1; \
 	      (x)->_allow_key_usage_violation = 1; \
-	      (x)->_allow_wrong_pms = 1; \
 	      (x)->_dumbfw = 1
 
 /* DH and RSA parameters types.
@@ -1129,7 +1126,6 @@ typedef struct {
 	bool no_etm;
 	bool no_ext_master_secret;
 	bool allow_key_usage_violation;
-	bool allow_wrong_pms;
 	bool dumbfw;
 
 	/* old (deprecated) variable. This is used for both srp_prime_bits
diff --git a/lib/priority.c b/lib/priority.c
index 154929e..8dd9c42 100644
--- a/lib/priority.c
+++ b/lib/priority.c
@@ -725,7 +725,6 @@ int gnutls_priority_set(gnutls_session_t session, gnutls_priority_t priority)
 	COPY_TO_INTERNALS(no_etm);
 	COPY_TO_INTERNALS(no_ext_master_secret);
 	COPY_TO_INTERNALS(allow_key_usage_violation);
-	COPY_TO_INTERNALS(allow_wrong_pms);
 	COPY_TO_INTERNALS(dumbfw);
 	COPY_TO_INTERNALS(dh_prime_bits);
 
-- 
2.33.0