diff options
author | Emilia Kasper <emilia@openssl.org> | 2014-08-28 19:43:49 +0200 |
---|---|---|
committer | Emilia Kasper <emilia@openssl.org> | 2014-09-24 12:47:19 +0200 |
commit | 9bed73adaa6f834177f29e478d9a2247a6577c04 (patch) | |
tree | d52c148f9a92161d3896ee9a1f5530236b348f6a | |
parent | e9128d9401ad617e17c5eb3772512c24b038b967 (diff) |
RT3066: rewrite RSA padding checks to be slightly more constant time.
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>
Conflicts:
crypto/rsa/rsa_oaep.c
crypto/rsa/rsa_pk1.c
ssl/s3_cbc.c
-rw-r--r-- | crypto/constant_time_locl.h | 36 | ||||
-rw-r--r-- | crypto/constant_time_test.c | 118 | ||||
-rw-r--r-- | crypto/rsa/Makefile | 5 | ||||
-rw-r--r-- | crypto/rsa/rsa.h | 3 | ||||
-rw-r--r-- | crypto/rsa/rsa_err.c | 3 | ||||
-rw-r--r-- | crypto/rsa/rsa_oaep.c | 149 | ||||
-rw-r--r-- | crypto/rsa/rsa_pk1.c | 103 | ||||
-rw-r--r-- | ssl/s3_cbc.c | 9 |
8 files changed, 308 insertions, 118 deletions
diff --git a/crypto/constant_time_locl.h b/crypto/constant_time_locl.h index 0d3acd58b3..ccf7b62f5f 100644 --- a/crypto/constant_time_locl.h +++ b/crypto/constant_time_locl.h @@ -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 diff --git a/crypto/constant_time_test.c b/crypto/constant_time_test.c index 89fad7d3f2..0e51892af5 100644 --- a/crypto/constant_time_test.c +++ b/crypto/constant_time_test.c @@ -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; } } diff --git a/crypto/rsa/Makefile b/crypto/rsa/Makefile index b61e9e9deb..af487b6004 100644 --- a/crypto/rsa/Makefile +++ b/crypto/rsa/Makefile @@ -212,7 +212,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 @@ -221,7 +221,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 diff --git a/crypto/rsa/rsa.h b/crypto/rsa/rsa.h index 2d57c2f386..f8e0cd04d5 100644 --- a/crypto/rsa/rsa.h +++ b/crypto/rsa/rsa.h @@ -576,7 +576,7 @@ void ERR_load_RSA_strings(void); #define RSA_R_DATA_TOO_LARGE_FOR_MODULUS 132 #define RSA_R_DATA_TOO_SMALL 111 #define RSA_R_DATA_TOO_SMALL_FOR_KEY_SIZE 122 -#define RSA_R_DIGEST_DOES_NOT_MATCH 159 +#define RSA_R_DIGEST_DOES_NOT_MATCH 166 #define RSA_R_DIGEST_TOO_BIG_FOR_RSA_KEY 112 #define RSA_R_DMP1_NOT_CONGRUENT_TO_D 124 #define RSA_R_DMQ1_NOT_CONGRUENT_TO_D 125 @@ -610,6 +610,7 @@ void ERR_load_RSA_strings(void); #define RSA_R_OPERATION_NOT_ALLOWED_IN_FIPS_MODE 158 #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 diff --git a/crypto/rsa/rsa_err.c b/crypto/rsa/rsa_err.c index 3a5c580209..ae905c6cb1 100644 --- a/crypto/rsa/rsa_err.c +++ b/crypto/rsa/rsa_err.c @@ -1,6 +1,6 @@ /* crypto/rsa/rsa_err.c */ /* ==================================================================== - * Copyright (c) 1999-2013 The OpenSSL Project. All rights reserved. + * Copyright (c) 1999-2014 The OpenSSL Project. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -185,6 +185,7 @@ static ERR_STRING_DATA RSA_str_reasons[]= {ERR_REASON(RSA_R_OPERATION_NOT_ALLOWED_IN_FIPS_MODE),"operation not allowed in fips mode"}, {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"}, diff --git a/crypto/rsa/rsa_oaep.c b/crypto/rsa/rsa_oaep.c index 5446d4db92..1a7a0c7301 100644 --- a/crypto/rsa/rsa_oaep.c +++ b/crypto/rsa/rsa_oaep.c @@ -18,6 +18,7 @@ * an equivalent notion. */ +#include "../constant_time_locl.h" #if !defined(OPENSSL_NO_SHA) && !defined(OPENSSL_NO_SHA1) #include <stdio.h> @@ -119,12 +120,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) @@ -134,85 +136,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, diff --git a/crypto/rsa/rsa_pk1.c b/crypto/rsa/rsa_pk1.c index 8560755f1d..e028ff7189 100644 --- a/crypto/rsa/rsa_pk1.c +++ b/crypto/rsa/rsa_pk1.c @@ -56,6 +56,8 @@ * [including the GNU Public Licence.] */ +#include "../constant_time_locl.h" + #include <stdio.h> #include "cryptlib.h" #include <openssl/bn.h> @@ -181,44 +183,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; + } diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c index d7ae3cf735..4c1a8940b3 100644 --- a/ssl/s3_cbc.c +++ b/ssl/s3_cbc.c @@ -96,7 +96,7 @@ int ssl3_cbc_remove_padding(const SSL* s, padding_length = good & (padding_length+1); rec->length -= padding_length; rec->type |= padding_length<<8; /* kludge: pass padding length */ - 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 @@ -193,7 +193,7 @@ int tls1_cbc_remove_padding(const SSL* s, rec->length -= padding_length; rec->type |= padding_length<<8; /* kludge: pass padding length */ - 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 @@ -652,7 +652,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. */ @@ -668,7 +668,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; } |