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>

diff --git a/lib/auth/rsa.c b/lib/auth/rsa.c
index bfd6977ca85cb400112e058a4488def5e454795c..b5ecc092f8a7f8d95cd5508cc915eaa7508acccb 100644 (file)
--- a/lib/auth/rsa.c
+++ b/lib/auth/rsa.c
@@ -206,7 +206,7 @@ static int proc_rsa_client_kx(gnutls_session_t session, uint8_t *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

diff --git a/lib/auth/rsa_psk.c b/lib/auth/rsa_psk.c
index 7b361642f667d063bcbf95a424e85d760861c752..99f908460aefcfd0d3d94ef5c97bb4981b13ce4e 100644 (file)
--- a/lib/auth/rsa_psk.c
+++ b/lib/auth/rsa_psk.c
@@ -251,14 +251,13 @@ static int _gnutls_proc_rsa_psk_client_kx(gnutls_session_t session,
 {
        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);
@@ -316,68 +315,47 @@ static int _gnutls_proc_rsa_psk_client_kx(gnutls_session_t session,
        }
        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 68b1257408d17c043144a57866ecd3bc64e57769..e9ec36d5855450ffeb3b2808f3ac314421863f0a 100644 (file)
--- a/lib/gnutls_int.h
+++ b/lib/gnutls_int.h
@@ -1088,7 +1088,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 */
 
@@ -1106,7 +1105,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)                \
@@ -1115,7 +1113,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.
@@ -1242,7 +1239,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 3e95e269c8140230412a964b94950bd89db21499..d84af07f99b5edfc0affccc85c10e2a0d4b6a173 100644 (file)
--- a/lib/priority.c
+++ b/lib/priority.c
@@ -624,7 +624,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);