RT3066: rewrite RSA padding checks to be slightly more constant time.
authorEmilia Kasper <emilia@openssl.org>
Thu, 28 Aug 2014 17:43:49 +0000 (19:43 +0200)
committerEmilia Kasper <emilia@openssl.org>
Wed, 24 Sep 2014 10:45:42 +0000 (12:45 +0200)
Also tweak s3_cbc.c to use new constant-time methods.
Also fix memory leaks from internal errors in RSA_padding_check_PKCS1_OAEP_mgf1

This patch is based on the original RT submission by Adam Langley <agl@chromium.org>,
as well as code from BoringSSL and OpenSSL.

Reviewed-by: Kurt Roeckx <kurt@openssl.org>
crypto/constant_time_locl.h
crypto/constant_time_test.c
crypto/rsa/Makefile
crypto/rsa/rsa.h
crypto/rsa/rsa_err.c
crypto/rsa/rsa_oaep.c
crypto/rsa/rsa_pk1.c
ssl/s3_cbc.c

index 0d3acd5..ccf7b62 100644 (file)
@@ -54,7 +54,7 @@ extern "C" {
 #endif
 
 /*
- * The following methods return a bitmask of all ones (0xff...f) for true
+ * The boolean methods return a bitmask of all ones (0xff...f) for true
  * and 0 for false. This is useful for choosing a value based on the result
  * of a conditional in constant time. For example,
  *
@@ -67,7 +67,7 @@ extern "C" {
  * can be written as
  *
  * unsigned int lt = constant_time_lt(a, b);
- * c = a & lt | b & ~lt;
+ * c = constant_time_select(lt, a, b);
  */
 
 /*
@@ -107,6 +107,21 @@ static inline unsigned int constant_time_eq(unsigned int a, unsigned int b);
 /* Convenience method for getting an 8-bit mask. */
 static inline unsigned char constant_time_eq_8(unsigned int a, unsigned int b);
 
+/*
+ * Returns (mask & a) | (~mask & b).
+ *
+ * When |mask| is all 1s or all 0s (as returned by the methods above),
+ * the select methods return either |a| (if |mask| is nonzero) or |b|
+ * (if |mask| is zero).
+ */
+static inline unsigned int constant_time_select(unsigned int mask,
+       unsigned int a, unsigned int b);
+/* Convenience method for unsigned chars. */
+static inline unsigned char constant_time_select_8(unsigned char mask,
+       unsigned char a, unsigned char b);
+/* Convenience method for signed integers. */
+static inline int constant_time_select_int(unsigned int mask, int a, int b);
+
 static inline unsigned int constant_time_msb(unsigned int a)
        {
        return (unsigned int)((int)(a) >> (sizeof(int) * 8 - 1));
@@ -162,6 +177,23 @@ static inline unsigned char constant_time_eq_8(unsigned int a, unsigned int b)
        return (unsigned char)(constant_time_eq(a, b));
        }
 
+static inline unsigned int constant_time_select(unsigned int mask,
+       unsigned int a, unsigned int b)
+       {
+       return (mask & a) | (~mask & b);
+       }
+
+static inline unsigned char constant_time_select_8(unsigned char mask,
+       unsigned char a, unsigned char b)
+       {
+       return (unsigned char)(constant_time_select(mask, a, b));
+       }
+
+inline int constant_time_select_int(unsigned int mask, int a, int b)
+       {
+       return (int)(constant_time_select(mask, (unsigned)(a), (unsigned)(b)));
+       }
+
 #ifdef __cplusplus
 }
 #endif
index 89fad7d..0e51892 100644 (file)
@@ -50,9 +50,9 @@
 #include <stdio.h>
 #include <stdlib.h>
 
-static const unsigned int CONSTTIME_TRUE = ~0;
+static const unsigned int CONSTTIME_TRUE = (unsigned)(~0);
 static const unsigned int CONSTTIME_FALSE = 0;
-static const unsigned char CONSTTIME_TRUE_8 = ~0;
+static const unsigned char CONSTTIME_TRUE_8 = 0xff;
 static const unsigned char CONSTTIME_FALSE_8 = 0;
 
 static int test_binary_op(unsigned int (*op)(unsigned int a, unsigned int b),
@@ -133,13 +133,86 @@ static int test_is_zero_8(unsigned int a)
         return 0;
        }
 
+static int test_select(unsigned int a, unsigned int b)
+       {
+       unsigned int selected = constant_time_select(CONSTTIME_TRUE, a, b);
+       if (selected != a)
+               {
+               fprintf(stderr, "Test failed for constant_time_select(%du, %du,"
+                       "%du): expected %du(first value), got %du\n",
+                       CONSTTIME_TRUE, a, b, a, selected);
+               return 1;
+               }
+       selected = constant_time_select(CONSTTIME_FALSE, a, b);
+       if (selected != b)
+               {
+               fprintf(stderr, "Test failed for constant_time_select(%du, %du,"
+                       "%du): expected %du(second value), got %du\n",
+                       CONSTTIME_FALSE, a, b, b, selected);
+               return 1;
+               }
+       return 0;
+       }
+
+static int test_select_8(unsigned char a, unsigned char b)
+       {
+       unsigned char selected = constant_time_select_8(CONSTTIME_TRUE_8, a, b);
+       if (selected != a)
+               {
+               fprintf(stderr, "Test failed for constant_time_select(%u, %u,"
+                       "%u): expected %u(first value), got %u\n",
+                       CONSTTIME_TRUE, a, b, a, selected);
+               return 1;
+               }
+       selected = constant_time_select_8(CONSTTIME_FALSE_8, a, b);
+       if (selected != b)
+               {
+               fprintf(stderr, "Test failed for constant_time_select(%u, %u,"
+                       "%u): expected %u(second value), got %u\n",
+                       CONSTTIME_FALSE, a, b, b, selected);
+               return 1;
+               }
+       return 0;
+       }
+
+static int test_select_int(int a, int b)
+       {
+       int selected = constant_time_select_int(CONSTTIME_TRUE, a, b);
+       if (selected != a)
+               {
+               fprintf(stderr, "Test failed for constant_time_select(%du, %d,"
+                       "%d): expected %d(first value), got %d\n",
+                       CONSTTIME_TRUE, a, b, a, selected);
+               return 1;
+               }
+       selected = constant_time_select_int(CONSTTIME_FALSE, a, b);
+       if (selected != b)
+               {
+               fprintf(stderr, "Test failed for constant_time_select(%du, %d,"
+                       "%d): expected %d(second value), got %d\n",
+                       CONSTTIME_FALSE, a, b, b, selected);
+               return 1;
+               }
+       return 0;
+       }
+
+
 static unsigned int test_values[] = {0, 1, 1024, 12345, 32000, UINT_MAX/2-1,
                                      UINT_MAX/2, UINT_MAX/2+1, UINT_MAX-1,
                                      UINT_MAX};
 
+static unsigned char test_values_8[] = {0, 1, 2, 20, 32, 127, 128, 129, 255};
+
+static int signed_test_values[] = {0, 1, -1, 1024, -1024, 12345, -12345,
+                                  32000, -32000, INT_MAX, INT_MIN, INT_MAX-1,
+                                  INT_MIN+1};
+
+
 int main(int argc, char *argv[])
        {
        unsigned int a, b, i, j;
+       int c, d;
+       unsigned char e, f;
        int num_failed = 0, num_all = 0;
        fprintf(stdout, "Testing constant time operations...\n");
 
@@ -148,20 +221,8 @@ int main(int argc, char *argv[])
                a = test_values[i];
                num_failed += test_is_zero(a);
                num_failed += test_is_zero_8(a);
-               num_failed += test_binary_op(&constant_time_lt,
-                       "constant_time_lt", a, a, 0);
-               num_failed += test_binary_op_8(&constant_time_lt_8,
-                       "constant_time_lt_8", a, a, 0);
-               num_failed += test_binary_op(&constant_time_ge,
-                       "constant_time_ge", a, a, 1);
-               num_failed += test_binary_op_8(&constant_time_ge_8,
-                       "constant_time_ge_8", a, a, 1);
-               num_failed += test_binary_op(&constant_time_eq,
-                       "constant_time_eq", a, a, 1);
-               num_failed += test_binary_op_8(&constant_time_eq_8,
-                       "constant_time_eq_8", a, a, 1);
-               num_all += 8;
-               for (j = i + 1; j < sizeof(test_values)/sizeof(int); ++j)
+               num_all += 2;
+               for (j = 0; j < sizeof(test_values)/sizeof(int); ++j)
                        {
                        b = test_values[j];
                        num_failed += test_binary_op(&constant_time_lt,
@@ -188,7 +249,30 @@ int main(int argc, char *argv[])
                                "constant_time_eq", b, a, b == a);
                        num_failed += test_binary_op_8(&constant_time_eq_8,
                                "constant_time_eq_8", b, a, b == a);
-                       num_all += 12;
+                       num_failed += test_select(a, b);
+                       num_all += 13;
+                       }
+               }
+
+       for (i = 0; i < sizeof(signed_test_values)/sizeof(int); ++i)
+               {
+               c = signed_test_values[i];
+               for (j = 0; j < sizeof(signed_test_values)/sizeof(int); ++j)
+                       {
+                       d = signed_test_values[j];
+                       num_failed += test_select_int(c, d);
+                       num_all += 1;
+                       }
+               }
+
+       for (i = 0; i < sizeof(test_values_8); ++i)
+               {
+               e = test_values_8[i];
+               for (j = 0; j < sizeof(test_values_8); ++j)
+                       {
+                       f = test_values_8[j];
+                       num_failed += test_select_8(e, f);
+                       num_all += 1;
                        }
                }
 
index 5d1bdf9..ba0bb7a 100644 (file)
@@ -206,7 +206,7 @@ rsa_oaep.o: ../../include/openssl/opensslv.h ../../include/openssl/ossl_typ.h
 rsa_oaep.o: ../../include/openssl/rand.h ../../include/openssl/rsa.h
 rsa_oaep.o: ../../include/openssl/safestack.h ../../include/openssl/sha.h
 rsa_oaep.o: ../../include/openssl/stack.h ../../include/openssl/symhacks.h
-rsa_oaep.o: ../cryptlib.h rsa_oaep.c
+rsa_oaep.o: ../constant_time_locl.h ../cryptlib.h rsa_oaep.c
 rsa_pk1.o: ../../e_os.h ../../include/openssl/asn1.h
 rsa_pk1.o: ../../include/openssl/bio.h ../../include/openssl/bn.h
 rsa_pk1.o: ../../include/openssl/buffer.h ../../include/openssl/crypto.h
@@ -215,7 +215,8 @@ rsa_pk1.o: ../../include/openssl/lhash.h ../../include/openssl/opensslconf.h
 rsa_pk1.o: ../../include/openssl/opensslv.h ../../include/openssl/ossl_typ.h
 rsa_pk1.o: ../../include/openssl/rand.h ../../include/openssl/rsa.h
 rsa_pk1.o: ../../include/openssl/safestack.h ../../include/openssl/stack.h
-rsa_pk1.o: ../../include/openssl/symhacks.h ../cryptlib.h rsa_pk1.c
+rsa_pk1.o: ../../include/openssl/symhacks.h ../constant_time_locl.h
+rsa_pk1.o: ../cryptlib.h rsa_pk1.c
 rsa_pmeth.o: ../../e_os.h ../../include/openssl/asn1.h
 rsa_pmeth.o: ../../include/openssl/asn1t.h ../../include/openssl/bio.h
 rsa_pmeth.o: ../../include/openssl/bn.h ../../include/openssl/buffer.h
index 79905fe..f164d74 100644 (file)
@@ -616,6 +616,7 @@ void ERR_load_RSA_strings(void);
 #define RSA_R_OAEP_DECODING_ERROR                       121
 #define RSA_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE  148
 #define RSA_R_PADDING_CHECK_FAILED                      114
+#define RSA_R_PKCS_DECODING_ERROR                       159
 #define RSA_R_P_NOT_PRIME                               128
 #define RSA_R_Q_NOT_PRIME                               129
 #define RSA_R_RSA_OPERATIONS_NOT_SUPPORTED              130
@@ -624,7 +625,7 @@ void ERR_load_RSA_strings(void);
 #define RSA_R_SSLV3_ROLLBACK_ATTACK                     115
 #define RSA_R_THE_ASN1_OBJECT_IDENTIFIER_IS_NOT_KNOWN_FOR_THIS_MD 116
 #define RSA_R_UNKNOWN_ALGORITHM_TYPE                    117
-#define RSA_R_UNKNOWN_DIGEST                            159
+#define RSA_R_UNKNOWN_DIGEST                            166
 #define RSA_R_UNKNOWN_MASK_DIGEST                       151
 #define RSA_R_UNKNOWN_PADDING_TYPE                      118
 #define RSA_R_UNKNOWN_PSS_DIGEST                        152
index 6a5685b..60cf77c 100644 (file)
@@ -181,6 +181,7 @@ static ERR_STRING_DATA RSA_str_reasons[]=
 {ERR_REASON(RSA_R_OAEP_DECODING_ERROR)   ,"oaep decoding error"},
 {ERR_REASON(RSA_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE),"operation not supported for this keytype"},
 {ERR_REASON(RSA_R_PADDING_CHECK_FAILED)  ,"padding check failed"},
+{ERR_REASON(RSA_R_PKCS_DECODING_ERROR)   ,"pkcs decoding error"},
 {ERR_REASON(RSA_R_P_NOT_PRIME)           ,"p not prime"},
 {ERR_REASON(RSA_R_Q_NOT_PRIME)           ,"q not prime"},
 {ERR_REASON(RSA_R_RSA_OPERATIONS_NOT_SUPPORTED),"rsa operations not supported"},
index 882c7c5..18ee6f4 100644 (file)
@@ -20,6 +20,7 @@
 
 #define OPENSSL_FIPSAPI
 
+#include "../constant_time_locl.h"
 
 #if !defined(OPENSSL_NO_SHA) && !defined(OPENSSL_NO_SHA1)
 #include <stdio.h>
@@ -121,12 +122,13 @@ int RSA_padding_check_PKCS1_OAEP_mgf1(unsigned char *to, int tlen,
        const unsigned char *param, int plen,
        const EVP_MD *md, const EVP_MD *mgf1md)
        {
-       int i, dblen, mlen = -1;
-       const unsigned char *maskeddb;
-       int lzero;
-       unsigned char *db = NULL, seed[EVP_MAX_MD_SIZE], phash[EVP_MAX_MD_SIZE];
-       unsigned char *padded_from;
-       int bad = 0;
+       int i, dblen, mlen = -1, one_index = 0, msg_index;
+       unsigned int good, found_one_byte;
+       const unsigned char *maskedseed, *maskeddb;
+       /* |em| is the encoded message, zero-padded to exactly |num| bytes:
+        * em = Y || maskedSeed || maskedDB */
+       unsigned char *db = NULL, *em = NULL, seed[EVP_MAX_MD_SIZE],
+               phash[EVP_MAX_MD_SIZE];
        int mdlen;
 
        if (md == NULL)
@@ -136,85 +138,108 @@ int RSA_padding_check_PKCS1_OAEP_mgf1(unsigned char *to, int tlen,
 
        mdlen = EVP_MD_size(md);
 
-       if (--num < 2 * mdlen + 1)
-               /* 'num' is the length of the modulus, i.e. does not depend on the
-                * particular ciphertext. */
+        if (tlen <= 0 || flen <= 0)
+               return -1;
+       /*
+        * |num| is the length of the modulus; |flen| is the length of the
+        * encoded message. Therefore, for any |from| that was obtained by
+        * decrypting a ciphertext, we must have |flen| <= |num|. Similarly,
+        * num < 2 * mdlen + 2 must hold for the modulus irrespective of
+        * the ciphertext, see PKCS #1 v2.2, section 7.1.2.
+        * This does not leak any side-channel information.
+        */
+       if (num < flen || num < 2 * mdlen + 2)
                goto decoding_err;
 
-       lzero = num - flen;
-       if (lzero < 0)
-               {
-               /* signalling this error immediately after detection might allow
-                * for side-channel attacks (e.g. timing if 'plen' is huge
-                * -- cf. James H. Manger, "A Chosen Ciphertext Attack on RSA Optimal
-                * Asymmetric Encryption Padding (OAEP) [...]", CRYPTO 2001),
-                * so we use a 'bad' flag */
-               bad = 1;
-               lzero = 0;
-               flen = num; /* don't overflow the memcpy to padded_from */
-               }
-
-       dblen = num - mdlen;
-       db = OPENSSL_malloc(dblen + num);
-       if (db == NULL)
+       dblen = num - mdlen - 1;
+       db = OPENSSL_malloc(dblen);
+       em = OPENSSL_malloc(num);
+       if (db == NULL || em == NULL)
                {
                RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, ERR_R_MALLOC_FAILURE);
-               return -1;
+               goto cleanup;
                }
 
-       /* Always do this zero-padding copy (even when lzero == 0)
-        * to avoid leaking timing info about the value of lzero. */
-       padded_from = db + dblen;
-       memset(padded_from, 0, lzero);
-       memcpy(padded_from + lzero, from, flen);
-
-       maskeddb = padded_from + mdlen;
+       /*
+        * Always do this zero-padding copy (even when num == flen) to avoid
+        * leaking that information. The copy still leaks some side-channel
+        * information, but it's impossible to have a fixed  memory access
+        * pattern since we can't read out of the bounds of |from|.
+        *
+        * TODO(emilia): Consider porting BN_bn2bin_padded from BoringSSL.
+        */
+       memset(em, 0, num);
+       memcpy(em + num - flen, from, flen);
+
+       /*
+        * The first byte must be zero, however we must not leak if this is
+        * true. See James H. Manger, "A Chosen Ciphertext  Attack on RSA
+        * Optimal Asymmetric Encryption Padding (OAEP) [...]", CRYPTO 2001).
+        */
+       good = constant_time_is_zero(em[0]);
+
+       maskedseed = em + 1;
+       maskeddb = em + 1 + mdlen;
 
        if (PKCS1_MGF1(seed, mdlen, maskeddb, dblen, mgf1md))
-               return -1;
+               goto cleanup;
        for (i = 0; i < mdlen; i++)
-               seed[i] ^= padded_from[i];
-  
+               seed[i] ^= maskedseed[i];
+
        if (PKCS1_MGF1(db, dblen, seed, mdlen, mgf1md))
-               return -1;
+               goto cleanup;
        for (i = 0; i < dblen; i++)
                db[i] ^= maskeddb[i];
 
        if (!EVP_Digest((void *)param, plen, phash, NULL, md, NULL))
-               return -1;
+               goto cleanup;
+
+       good &= constant_time_is_zero(CRYPTO_memcmp(db, phash, mdlen));
+
+       found_one_byte = 0;
+       for (i = mdlen; i < dblen; i++)
+               {
+               /* Padding consists of a number of 0-bytes, followed by a 1. */
+               unsigned int equals1 = constant_time_eq(db[i], 1);
+               unsigned int equals0 = constant_time_is_zero(db[i]);
+               one_index = constant_time_select_int(~found_one_byte & equals1,
+                       i, one_index);
+               found_one_byte |= equals1;
+               good &= (found_one_byte | equals0);
+               }
+
+       good &= found_one_byte;
 
-       if (CRYPTO_memcmp(db, phash, mdlen) != 0 || bad)
+       /*
+        * At this point |good| is zero unless the plaintext was valid,
+        * so plaintext-awareness ensures timing side-channels are no longer a
+        * concern.
+        */
+       if (!good)
                goto decoding_err;
+
+       msg_index = one_index + 1;
+       mlen = dblen - msg_index;
+
+       if (tlen < mlen)
+               {
+               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, RSA_R_DATA_TOO_LARGE);
+               mlen = -1;
+               }
        else
                {
-               for (i = mdlen; i < dblen; i++)
-                       if (db[i] != 0x00)
-                               break;
-               if (i == dblen || db[i] != 0x01)
-                       goto decoding_err;
-               else
-                       {
-                       /* everything looks OK */
-
-                       mlen = dblen - ++i;
-                       if (tlen < mlen)
-                               {
-                               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, RSA_R_DATA_TOO_LARGE);
-                               mlen = -1;
-                               }
-                       else
-                               memcpy(to, db + i, mlen);
-                       }
+               memcpy(to, db + msg_index, mlen);
+               goto cleanup;
                }
-       OPENSSL_free(db);
-       return mlen;
 
 decoding_err:
-       /* to avoid chosen ciphertext attacks, the error message should not reveal
-        * which kind of decoding error happened */
+       /* To avoid chosen ciphertext attacks, the error message should not reveal
+        * which kind of decoding error happened. */
        RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_OAEP_MGF1, RSA_R_OAEP_DECODING_ERROR);
+cleanup:
        if (db != NULL) OPENSSL_free(db);
-       return -1;
+       if (em != NULL) OPENSSL_free(em);
+       return mlen;
        }
 
 int PKCS1_MGF1(unsigned char *mask, long len,
index 0cce4bf..e01fe62 100644 (file)
@@ -58,6 +58,8 @@
 
 #define OPENSSL_FIPSAPI
 
+#include "../constant_time_locl.h"
+
 #include <stdio.h>
 #include "cryptlib.h"
 #include <openssl/bn.h>
@@ -183,44 +185,87 @@ int RSA_padding_add_PKCS1_type_2(unsigned char *to, int tlen,
 int RSA_padding_check_PKCS1_type_2(unsigned char *to, int tlen,
             const unsigned char *from, int flen, int num)
        {
-       int i,j;
-       const unsigned char *p;
+       int i;
+       /* |em| is the encoded message, zero-padded to exactly |num| bytes */
+       unsigned char *em = NULL;
+       unsigned int good, found_zero_byte;
+       int zero_index = 0, msg_index, mlen = -1;
 
-       p=from;
-       if ((num != (flen+1)) || (*(p++) != 02))
-               {
-               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_BLOCK_TYPE_IS_NOT_02);
-               return(-1);
-               }
-#ifdef PKCS1_CHECK
-       return(num-11);
-#endif
+        if (tlen < 0 || flen < 0)
+               return -1;
 
-       /* scan over padding data */
-       j=flen-1; /* one for type. */
-       for (i=0; i<j; i++)
-               if (*(p++) == 0) break;
+       /* PKCS#1 v1.5 decryption. See "PKCS #1 v2.2: RSA Cryptography
+        * Standard", section 7.2.2. */
 
-       if (i == j)
+       if (flen > num)
+               goto err;
+
+       if (num < 11)
+               goto err;
+
+       em = OPENSSL_malloc(num);
+       if (em == NULL)
                {
-               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_NULL_BEFORE_BLOCK_MISSING);
-               return(-1);
+               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, ERR_R_MALLOC_FAILURE);
+               return -1;
                }
+       memset(em, 0, num);
+       /*
+        * Always do this zero-padding copy (even when num == flen) to avoid
+        * leaking that information. The copy still leaks some side-channel
+        * information, but it's impossible to have a fixed  memory access
+        * pattern since we can't read out of the bounds of |from|.
+        *
+        * TODO(emilia): Consider porting BN_bn2bin_padded from BoringSSL.
+        */
+       memcpy(em + num - flen, from, flen);
 
-       if (i < 8)
+       good = constant_time_is_zero(em[0]);
+       good &= constant_time_eq(em[1], 2);
+
+       found_zero_byte = 0;
+       for (i = 2; i < num; i++)
                {
-               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_BAD_PAD_BYTE_COUNT);
-               return(-1);
+               unsigned int equals0 = constant_time_is_zero(em[i]);
+               zero_index = constant_time_select_int(~found_zero_byte & equals0, i, zero_index);
+               found_zero_byte |= equals0;
                }
-       i++; /* Skip over the '\0' */
-       j-=i;
-       if (j > tlen)
+
+       /*
+        * PS must be at least 8 bytes long, and it starts two bytes into |em|.
+         * If we never found a 0-byte, then |zero_index| is 0 and the check
+        * also fails.
+        */
+       good &= constant_time_ge((unsigned int)(zero_index), 2 + 8);
+
+       /* Skip the zero byte. This is incorrect if we never found a zero-byte
+        * but in this case we also do not copy the message out. */
+       msg_index = zero_index + 1;
+       mlen = num - msg_index;
+
+       /* For good measure, do this check in constant time as well; it could
+        * leak something if |tlen| was assuming valid padding. */
+       good &= constant_time_ge((unsigned int)(tlen), (unsigned int)(mlen));
+
+       /*
+        * 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)
                {
-               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2,RSA_R_DATA_TOO_LARGE);
-               return(-1);
+               mlen = -1;
+               goto err;
                }
-       memcpy(to,p,(unsigned int)j);
 
-       return(j);
-       }
+       memcpy(to, em + msg_index, mlen);
 
+err:
+       if (em != NULL)
+               OPENSSL_free(em);
+       if (mlen == -1)
+               RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, RSA_R_PKCS_DECODING_ERROR);
+       return mlen;
+       }
index f15c5b5..27f309e 100644 (file)
@@ -94,7 +94,7 @@ int ssl3_cbc_remove_padding(const SSL* s,
        /* SSLv3 requires that the padding is minimal. */
        good &= constant_time_ge(block_size, padding_length+1);
        rec->length -= good & (padding_length+1);
-       return (int)((good & 1) | (~good & -1));
+       return constant_time_select_int(good, 1, -1);
        }
 
 /* tls1_cbc_remove_padding removes the CBC padding from the decrypted, TLS, CBC
@@ -190,7 +190,7 @@ int tls1_cbc_remove_padding(const SSL* s,
        good = constant_time_eq(0xff, good & 0xff);
        rec->length -= good & (padding_length+1);
 
-       return (int)((good & 1) | (~good & -1));
+       return constant_time_select_int(good, 1, -1);
        }
 
 /* ssl3_cbc_copy_mac copies |md_size| bytes from the end of |rec| to |out| in
@@ -650,7 +650,7 @@ void ssl3_cbc_digest_record(
                        /* If this is the block containing the end of the
                         * application data, and we are at the offset for the
                         * 0x80 value, then overwrite b with 0x80. */
-                       b = (b&~is_past_c) | (0x80&is_past_c);
+                        b =  constant_time_select_8(is_past_c, 0x80, b);
                        /* If this the the block containing the end of the
                         * application data and we're past the 0x80 value then
                         * just write zero. */
@@ -666,7 +666,8 @@ void ssl3_cbc_digest_record(
                        if (j >= md_block_size - md_length_size)
                                {
                                /* If this is index_b, write a length byte. */
-                               b = (b&~is_block_b) | (is_block_b&length_bytes[j-(md_block_size-md_length_size)]);
+                               b = constant_time_select_8(
+                                       is_block_b, length_bytes[j-(md_block_size-md_length_size)], b);
                                }
                        block[j] = b;
                        }