Backport of: From 29d6298d0b04cfff970b993915db71ba3f580b6d Mon Sep 17 00:00:00 2001 From: Daiki Ueno 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 Upstream-Status: Backport [import from debian https://launchpad.net/ubuntu/+archive/primary/+sourcefiles/gnutls28/3.7.3-4ubuntu1.3/gnutls28_3.7.3-4ubuntu1.3.debian.tar.xz Upstream-Commit: https://gitlab.com/gnutls/gnutls/-/commit/29d6298d0b04cfff970b993915db71ba3f580b6d] CVE: CVE-2023-5981 Signed-off-by: Vijay Anusuri --- 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(-) --- a/lib/auth/rsa.c +++ b/lib/auth/rsa.c @@ -207,7 +207,7 @@ proc_rsa_client_kx(gnutls_session_t sess 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 --- a/lib/auth/rsa_psk.c +++ b/lib/auth/rsa_psk.c @@ -264,14 +264,13 @@ _gnutls_proc_rsa_psk_client_kx(gnutls_se { 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); @@ -327,71 +326,47 @@ _gnutls_proc_rsa_psk_client_kx(gnutls_se } 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). - */ - 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"); - } - } + 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(); + 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 */ --- a/lib/gnutls_int.h +++ b/lib/gnutls_int.h @@ -974,7 +974,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 */ @@ -992,7 +991,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) \ @@ -1001,7 +999,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. @@ -1126,7 +1123,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 --- a/lib/priority.c +++ b/lib/priority.c @@ -690,7 +690,6 @@ gnutls_priority_set(gnutls_session_t ses 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);