Make RSA key exchange code actually constant-time.

Using RSA_PKCS1_PADDING with RSA_private_decrypt is inherently unsafe.
The API requires writing output on success and touching the error queue
on error. Thus, although the padding check itself is constant-time as of
294d1e36c2495ff00e697c9ff622856d3114f14f, and the logic after the
decryption in the SSL code is constant-time as of
adb46dbc6dd7347750df2468c93e8c34bcb93a4b, the API boundary in the middle
still leaks whether the padding check succeeded, giving us our
much-loved Bleichenbacher padding oracle.

Instead, PKCS#1 padding must be handled by the caller which uses
RSA_NO_PADDING, in timing-sensitive code integrated with the
Bleichenbacher mitigation. Removing PKCS#1 padding in constant time is
actually much simpler when the expected length is a constant (and if
it's not a constant, avoiding a padding oracle seems unlikely), so just
do it inline.

Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Rich Salz <rsalz@openssl.org>

GH: #1222

1 files changed, 36 insertions, 15 deletions

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index f88b6c8194..a88b3219ad 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -2087,7 +2087,7 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
         unsigned char rand_premaster_secret[SSL_MAX_MASTER_KEY_LENGTH];
         int decrypt_len;
         unsigned char decrypt_good, version_good;
-        size_t j;
+        size_t j, padding_len;
 
         /* FIX THIS UP EAY EAY EAY EAY */
         rsa = EVP_PKEY_get0_RSA(s->cert->pkeys[SSL_PKEY_RSA_ENC].privatekey);
@@ -2144,17 +2144,37 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
             goto err;
         }
 
+        /*
+         * Decrypt with no padding. PKCS#1 padding will be removed as part of
+         * the timing-sensitive code below.
+         */
         decrypt_len = RSA_private_decrypt(PACKET_remaining(&enc_premaster),
                                           PACKET_data(&enc_premaster),
-                                          rsa_decrypt, rsa, RSA_PKCS1_PADDING);
-        ERR_clear_error();
+                                          rsa_decrypt, rsa, RSA_NO_PADDING);
+        if (decrypt_len < 0) {
+            goto err;
+        }
+
+        /* Check the padding. See RFC 3447, section 7.2.2. */
 
         /*
-         * decrypt_len should be SSL_MAX_MASTER_KEY_LENGTH. decrypt_good will
-         * be 0xff if so and zero otherwise.
+         * The smallest padded premaster is 11 bytes of overhead. Small keys
+         * are publicly invalid, so this may return immediately. This ensures
+         * PS is at least 8 bytes.
          */
-        decrypt_good =
-            constant_time_eq_int_8(decrypt_len, SSL_MAX_MASTER_KEY_LENGTH);
+        if (decrypt_len < 11 + SSL_MAX_MASTER_KEY_LENGTH) {
+            al = SSL_AD_DECRYPT_ERROR;
+            SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, SSL_R_DECRYPTION_FAILED);
+            goto f_err;
+        }
+
+        padding_len = decrypt_len - SSL_MAX_MASTER_KEY_LENGTH;
+        decrypt_good = constant_time_eq_int_8(rsa_decrypt[0], 0) &
+                       constant_time_eq_int_8(rsa_decrypt[1], 2);
+        for (j = 2; j < padding_len - 1; j++) {
+            decrypt_good &= ~constant_time_is_zero_8(rsa_decrypt[j]);
+        }
+        decrypt_good &= constant_time_is_zero_8(rsa_decrypt[padding_len - 1]);
 
         /*
          * If the version in the decrypted pre-master secret is correct then
@@ -2165,10 +2185,10 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
          * constant time and are treated like any other decryption error.
          */
         version_good =
-            constant_time_eq_8(rsa_decrypt[0],
+            constant_time_eq_8(rsa_decrypt[padding_len],
                                (unsigned)(s->client_version >> 8));
         version_good &=
-            constant_time_eq_8(rsa_decrypt[1],
+            constant_time_eq_8(rsa_decrypt[padding_len + 1],
                                (unsigned)(s->client_version & 0xff));
 
         /*
@@ -2182,10 +2202,10 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
          */
         if (s->options & SSL_OP_TLS_ROLLBACK_BUG) {
             unsigned char workaround_good;
-            workaround_good =
-                constant_time_eq_8(rsa_decrypt[0], (unsigned)(s->version >> 8));
+            workaround_good = constant_time_eq_8(rsa_decrypt[padding_len],
+                                                 (unsigned)(s->version >> 8));
             workaround_good &=
-                constant_time_eq_8(rsa_decrypt[1],
+                constant_time_eq_8(rsa_decrypt[padding_len + 1],
                                    (unsigned)(s->version & 0xff));
             version_good |= workaround_good;
         }
@@ -2203,12 +2223,13 @@ MSG_PROCESS_RETURN tls_process_client_key_exchange(SSL *s, PACKET *pkt)
          * it is still sufficiently large to read from.
          */
         for (j = 0; j < sizeof(rand_premaster_secret); j++) {
-            rsa_decrypt[j] =
-                constant_time_select_8(decrypt_good, rsa_decrypt[j],
+            rsa_decrypt[padding_len + j] =
+                constant_time_select_8(decrypt_good,
+                                       rsa_decrypt[padding_len + j],
                                        rand_premaster_secret[j]);
         }
 
-        if (!ssl_generate_master_secret(s, rsa_decrypt,
+        if (!ssl_generate_master_secret(s, rsa_decrypt + padding_len,
                                         sizeof(rand_premaster_secret), 0)) {
             al = SSL_AD_INTERNAL_ERROR;
             SSLerr(SSL_F_TLS_PROCESS_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);