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 /crypto/rsa/rsa_pk1.c | |
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
Diffstat (limited to 'crypto/rsa/rsa_pk1.c')
-rw-r--r-- | crypto/rsa/rsa_pk1.c | 103 |
1 files changed, 74 insertions, 29 deletions
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; + } |