Backport of 5b8fa43 and remove resolved TODO: see PR#3924.

Make RSA key exchange code actually constant-time.

Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3935)

diff --git a/crypto/rsa/rsa_pk1.c b/crypto/rsa/rsa_pk1.c
index efa1fd3e993f0200bd5b8dd0b0bd2405617016a1..017766ce7166841ea61d62aef3ee1375e4e7ff56 100644 (file)
--- a/crypto/rsa/rsa_pk1.c
+++ b/crypto/rsa/rsa_pk1.c
@@ -255,8 +255,6 @@ int RSA_padding_check_PKCS1_type_2(unsigned char *to, int tlen,
      * We can't continue in constant-time because we need to copy the result
      * and we cannot fake its length. This unavoidably leaks timing
      * information at the API boundary.
-     * TODO(emilia): this could be addressed at the call site,
-     * see BoringSSL commit 0aa0767340baf925bda4804882aab0cb974b2d26.
      */
     if (!good) {
         mlen = -1;

diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index ba17f1b562812c2fc2ce6719bdf7782e5c667393..0fb4845d44fa3ae3c7a7c59b06e24c76d9d6a49f 100644 (file)
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -2202,7 +2202,7 @@ int ssl3_get_client_key_exchange(SSL *s)
         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 */
         if (s->s3->tmp.use_rsa_tmp) {
@@ -2270,16 +2270,38 @@ int ssl3_get_client_key_exchange(SSL *s)
         if (RAND_bytes(rand_premaster_secret,
                        sizeof(rand_premaster_secret)) <= 0)
             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((int)n, p, p, rsa, RSA_PKCS1_PADDING);
-        ERR_clear_error();
+            RSA_private_decrypt((int)n, p, p, 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_SSL3_GET_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(p[0], 0) &
+                       constant_time_eq_int_8(p[1], 2);
+        for (j = 2; j < padding_len - 1; j++) {
+            decrypt_good &= ~constant_time_is_zero_8(p[j]);
+        }
+        decrypt_good &= constant_time_is_zero_8(p[padding_len - 1]);
+        p += padding_len;
 
         /*
          * If the version in the decrypted pre-master secret is correct then