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 --- 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 @@ -205,11 +205,11 @@ proc_rsa_client_kx(gnutls_session_t sess gnutls_privkey_decrypt_data2(session->internals.selected_key, 0, &ciphertext, session->key.key.data, 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 * random key previously generated. (in order to avoid attack against * pkcs-1 formatting). --- a/lib/auth/rsa_psk.c +++ b/lib/auth/rsa_psk.c @@ -262,18 +262,17 @@ static int _gnutls_proc_rsa_psk_client_kx(gnutls_session_t session, uint8_t * data, size_t _data_size) { 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); if (cred == NULL) { @@ -327,75 +326,53 @@ _gnutls_proc_rsa_psk_client_kx(gnutls_se gnutls_assert(); return GNUTLS_E_UNEXPECTED_PACKET_LENGTH; } 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) { - 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; + gnutls_assert(); + return GNUTLS_E_MEMORY_ERROR; } + premaster_secret.size = GNUTLS_MASTER_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 */ ret = _gnutls_psk_pwd_find_entry(session, info->username, strlen(info->username), &pwd_psk); --- a/lib/gnutls_int.h +++ b/lib/gnutls_int.h @@ -983,11 +983,10 @@ struct gnutls_priority_st { bool _allow_large_records; bool _allow_small_records; 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 */ DEF_ATOMIC_INT(usage_cnt); }; @@ -1001,20 +1000,18 @@ struct gnutls_priority_st { (x)->allow_large_records = 1; \ (x)->allow_small_records = 1; \ (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) \ (x)->_allow_large_records = 1; \ (x)->_allow_small_records = 1; \ (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. */ typedef struct gnutls_dh_params_int { @@ -1135,11 +1132,10 @@ typedef struct { bool allow_large_records; bool allow_small_records; 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 * and dh_prime_bits as they don't overlap */ /* For SRP: minimum bits to allow for SRP --- a/lib/priority.c +++ b/lib/priority.c @@ -699,11 +699,10 @@ gnutls_priority_set(gnutls_session_t ses COPY_TO_INTERNALS(allow_large_records); COPY_TO_INTERNALS(allow_small_records); 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); return 0; }