From: Andy Polyakov Date: Sat, 1 Sep 2018 10:00:33 +0000 (+0200) Subject: rsa/rsa_pk1.c: remove memcpy calls from RSA_padding_check_PKCS1_type_2. X-Git-Tag: openssl-3.0.0-alpha1~2859 X-Git-Url: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff_plain;h=e875b0cf2f10bf2adf73e0c2ec81428290f4660c rsa/rsa_pk1.c: remove memcpy calls from RSA_padding_check_PKCS1_type_2. And make RSAErr call unconditional. Reviewed-by: Richard Levitte Reviewed-by: Matt Caswell --- diff --git a/crypto/rsa/rsa_pk1.c b/crypto/rsa/rsa_pk1.c index d07c0d6f85..b3a7a06117 100644 --- a/crypto/rsa/rsa_pk1.c +++ b/crypto/rsa/rsa_pk1.c @@ -158,7 +158,7 @@ int RSA_padding_check_PKCS1_type_2(unsigned char *to, int tlen, int i; /* |em| is the encoded message, zero-padded to exactly |num| bytes */ unsigned char *em = NULL; - unsigned int good, found_zero_byte; + unsigned int good, found_zero_byte, mask; int zero_index = 0, msg_index, mlen = -1; if (tlen < 0 || flen < 0) @@ -169,39 +169,41 @@ int RSA_padding_check_PKCS1_type_2(unsigned char *to, int tlen, * section 7.2.2. */ - if (flen > num) - goto err; - - if (num < 11) - goto err; + if (flen > num || num < 11) { + RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, + RSA_R_PKCS_DECODING_ERROR); + return -1; + } - if (flen != num) { - em = OPENSSL_zalloc(num); - if (em == NULL) { - RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, ERR_R_MALLOC_FAILURE); - return -1; - } - /* - * Caller is encouraged to pass zero-padded message created with - * BN_bn2binpad, but if it doesn't, we do this zero-padding copy - * 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|. - */ - memcpy(em + num - flen, from, flen); - from = em; + em = OPENSSL_malloc(num); + if (em == NULL) { + RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, ERR_R_MALLOC_FAILURE); + return -1; + } + /* + * Caller is encouraged to pass zero-padded message created with + * BN_bn2binpad. Trouble is that since we can't read out of |from|'s + * bounds, it's impossible to have an invariant memory access pattern + * in case |from| was not zero-padded in advance. + */ + for (from += flen, em += num, i = 0; i < num; i++) { + mask = ~constant_time_is_zero(flen); + flen -= 1 & mask; + from -= 1 & mask; + *--em = *from & mask; } + from = em; good = constant_time_is_zero(from[0]); good &= constant_time_eq(from[1], 2); + /* scan over padding data */ found_zero_byte = 0; for (i = 2; i < num; i++) { unsigned int equals0 = constant_time_is_zero(from[i]); - zero_index = - constant_time_select_int(~found_zero_byte & equals0, i, - zero_index); + + zero_index = constant_time_select_int(~found_zero_byte & equals0, + i, zero_index); found_zero_byte |= equals0; } @@ -210,7 +212,7 @@ int RSA_padding_check_PKCS1_type_2(unsigned char *to, int tlen, * 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); + good &= constant_time_ge(zero_index, 2 + 8); /* * Skip the zero byte. This is incorrect if we never found a zero-byte @@ -220,27 +222,34 @@ int RSA_padding_check_PKCS1_type_2(unsigned char *to, int tlen, 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. + * For good measure, do this check in constant time as well. */ - good &= constant_time_ge((unsigned int)(tlen), (unsigned int)(mlen)); + good &= constant_time_ge(tlen, 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. + * Even though we can't fake result's length, we can pretend copying + * |tlen| bytes where |mlen| bytes would be real. Last |tlen| of |num| + * bytes are viewed as circular buffer with start at |tlen|-|mlen'|, + * where |mlen'| is "saturated" |mlen| value. Deducing information + * about failure or |mlen| would take attacker's ability to observe + * memory access pattern with byte granularity *as it occurs*. It + * should be noted that failure is indistinguishable from normal + * operation if |tlen| is fixed by protocol. */ - if (!good) { - mlen = -1; - goto err; - } + tlen = constant_time_select_int(constant_time_lt(num, tlen), num, tlen); + msg_index = constant_time_select_int(good, msg_index, num - tlen); + mlen = num - msg_index; + for (from += msg_index, mask = good, i = 0; i < tlen; i++) { + unsigned int equals = constant_time_eq(i, mlen); - memcpy(to, from + msg_index, mlen); + from -= tlen & equals; /* if (i == mlen) rewind */ + mask &= mask ^ equals; /* if (i == mlen) mask = 0 */ + to[i] = constant_time_select_8(mask, from[i], to[i]); + } - err: OPENSSL_clear_free(em, num); - if (mlen == -1) - RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, - RSA_R_PKCS_DECODING_ERROR); - return mlen; + RSAerr(RSA_F_RSA_PADDING_CHECK_PKCS1_TYPE_2, RSA_R_PKCS_DECODING_ERROR); + err_clear_last_constant_time(1 & good); + + return constant_time_select_int(good, mlen, -1); } diff --git a/doc/man3/RSA_padding_add_PKCS1_type_1.pod b/doc/man3/RSA_padding_add_PKCS1_type_1.pod index 93911cac97..9ea2634c03 100644 --- a/doc/man3/RSA_padding_add_PKCS1_type_1.pod +++ b/doc/man3/RSA_padding_add_PKCS1_type_1.pod @@ -110,7 +110,12 @@ L. The RSA_padding_check_PKCS1_type_2() padding check leaks timing information which can potentially be used to mount a Bleichenbacher padding oracle attack. This is an inherent weakness in the PKCS #1 -v1.5 padding design. Prefer PKCS1_OAEP padding. +v1.5 padding design. Prefer PKCS1_OAEP padding. Otherwise it can +be recommended to pass zero-padded B, so that B equals to +B, and if fixed by protocol, B being set to the +expected length. In such case leakage would be minimal, it would +take attacker's ability to observe memory access pattern with byte +granilarity as it occurs, post-factum timing analysis won't do. =head1 SEE ALSO