From 0372649a943fb23f7f08c7acdbc01464b9df03f0 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Tue, 14 Feb 2023 14:28:47 +0100 Subject: Revert "Fix Timing Oracle in RSA decryption" This reverts commit 43d8f88511991533f53680a751e9326999a6a31f. Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/20284) --- crypto/bn/bn_blind.c | 14 ++++++++++++++ crypto/bn/bn_err.c | 2 -- crypto/bn/bn_local.h | 14 -------------- crypto/rsa/rsa_ossl.c | 17 ++++------------- include/crypto/bn.h | 5 ----- include/openssl/bnerr.h | 1 - 6 files changed, 18 insertions(+), 35 deletions(-) diff --git a/crypto/bn/bn_blind.c b/crypto/bn/bn_blind.c index dd5beea7c9..15d9e0a544 100644 --- a/crypto/bn/bn_blind.c +++ b/crypto/bn/bn_blind.c @@ -13,6 +13,20 @@ #define BN_BLINDING_COUNTER 32 +struct bn_blinding_st { + BIGNUM *A; + BIGNUM *Ai; + BIGNUM *e; + BIGNUM *mod; /* just a reference */ + CRYPTO_THREAD_ID tid; + int counter; + unsigned long flags; + BN_MONT_CTX *m_ctx; + int (*bn_mod_exp) (BIGNUM *r, const BIGNUM *a, const BIGNUM *p, + const BIGNUM *m, BN_CTX *ctx, BN_MONT_CTX *m_ctx); + CRYPTO_RWLOCK *lock; +}; + BN_BLINDING *BN_BLINDING_new(const BIGNUM *A, const BIGNUM *Ai, BIGNUM *mod) { BN_BLINDING *ret = NULL; diff --git a/crypto/bn/bn_err.c b/crypto/bn/bn_err.c index 6f5464b540..6ea5fc3d5c 100644 --- a/crypto/bn/bn_err.c +++ b/crypto/bn/bn_err.c @@ -73,8 +73,6 @@ static const ERR_STRING_DATA BN_str_functs[] = { {ERR_PACK(ERR_LIB_BN, BN_F_BN_SET_WORDS, 0), "bn_set_words"}, {ERR_PACK(ERR_LIB_BN, BN_F_BN_STACK_PUSH, 0), "BN_STACK_push"}, {ERR_PACK(ERR_LIB_BN, BN_F_BN_USUB, 0), "BN_usub"}, - {ERR_PACK(ERR_LIB_BN, BN_F_OSSL_BN_RSA_DO_UNBLIND, 0), - "ossl_bn_rsa_do_unblind"}, {0, NULL} }; diff --git a/crypto/bn/bn_local.h b/crypto/bn/bn_local.h index 30b7614fdb..ee6342b60c 100644 --- a/crypto/bn/bn_local.h +++ b/crypto/bn/bn_local.h @@ -283,20 +283,6 @@ struct bn_gencb_st { } cb; }; -struct bn_blinding_st { - BIGNUM *A; - BIGNUM *Ai; - BIGNUM *e; - BIGNUM *mod; /* just a reference */ - CRYPTO_THREAD_ID tid; - int counter; - unsigned long flags; - BN_MONT_CTX *m_ctx; - int (*bn_mod_exp) (BIGNUM *r, const BIGNUM *a, const BIGNUM *p, - const BIGNUM *m, BN_CTX *ctx, BN_MONT_CTX *m_ctx); - CRYPTO_RWLOCK *lock; -}; - /*- * BN_window_bits_for_exponent_size -- macro for sliding window mod_exp functions * diff --git a/crypto/rsa/rsa_ossl.c b/crypto/rsa/rsa_ossl.c index 2e3ee4ab33..53cf2d03c9 100644 --- a/crypto/rsa/rsa_ossl.c +++ b/crypto/rsa/rsa_ossl.c @@ -465,20 +465,11 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from, BN_free(d); } - if (blinding) { - /* - * ossl_bn_rsa_do_unblind() combines blinding inversion and - * 0-padded BN BE serialization - */ - j = ossl_bn_rsa_do_unblind(ret, blinding, unblind, rsa->n, ctx, - buf, num); - if (j == 0) - goto err; - } else { - j = BN_bn2binpad(ret, buf, num); - if (j < 0) + if (blinding) + if (!rsa_blinding_invert(blinding, ret, unblind, ctx)) goto err; - } + + j = BN_bn2binpad(ret, buf, num); switch (padding) { case RSA_PKCS1_PADDING: diff --git a/include/crypto/bn.h b/include/crypto/bn.h index 9f866ed71a..250914c46a 100644 --- a/include/crypto/bn.h +++ b/include/crypto/bn.h @@ -86,10 +86,5 @@ int bn_lshift_fixed_top(BIGNUM *r, const BIGNUM *a, int n); int bn_rshift_fixed_top(BIGNUM *r, const BIGNUM *a, int n); int bn_div_fixed_top(BIGNUM *dv, BIGNUM *rem, const BIGNUM *m, const BIGNUM *d, BN_CTX *ctx); -int ossl_bn_rsa_do_unblind(const BIGNUM *intermediate, - const BN_BLINDING *blinding, - const BIGNUM *possible_arg2, - const BIGNUM *to_mod, BN_CTX *ctx, - unsigned char *buf, int num); #endif diff --git a/include/openssl/bnerr.h b/include/openssl/bnerr.h index 5c83777f9f..a703efc92b 100644 --- a/include/openssl/bnerr.h +++ b/include/openssl/bnerr.h @@ -72,7 +72,6 @@ int ERR_load_BN_strings(void); # define BN_F_BN_SET_WORDS 144 # define BN_F_BN_STACK_PUSH 148 # define BN_F_BN_USUB 115 -# define BN_F_OSSL_BN_RSA_DO_UNBLIND 151 /* * BN reason codes. -- 2.25.1 From 3f499b24f3bcd66db022074f7e8b4f6ee266a3ae Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Mon, 13 Feb 2023 17:46:41 +0100 Subject: Alternative fix for CVE-2022-4304 This is about a timing leak in the topmost limb of the internal result of RSA_private_decrypt, before the padding check. There are in fact at least three bugs together that caused the timing leak: First and probably most important is the fact that the blinding did not use the constant time code path at all when the RSA object was used for a private decrypt, due to the fact that the Montgomery context rsa->_method_mod_n was not set up early enough in rsa_ossl_private_decrypt, when BN_BLINDING_create_param needed it, and that was persisted as blinding->m_ctx, although the RSA object creates the Montgomery context just a bit later. Then the infamous bn_correct_top was used on the secret value right after the blinding was removed. And finally the function BN_bn2binpad did not use the constant-time code path since the BN_FLG_CONSTTIME was not set on the secret value. In order to address the first problem, this patch makes sure that the rsa->_method_mod_n is initialized right before the blinding context. And to fix the second problem, we add a new utility function bn_correct_top_consttime, a const-time variant of bn_correct_top. Together with the fact, that BN_bn2binpad is already constant time if the flag BN_FLG_CONSTTIME is set, this should eliminate the timing oracle completely. In addition the no-asm variant may also have branches that depend on secret values, because the last invocation of bn_sub_words in bn_from_montgomery_word had branches when the function is compiled by certain gcc compiler versions, due to the clumsy coding style. So additionally this patch stream-lined the no-asm C-code in order to avoid branches where possible and improve the resulting code quality. Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/20284) --- crypto/bn/bn_asm.c | 106 +++++++++++++++++++++++------------------- crypto/bn/bn_blind.c | 3 +- crypto/bn/bn_lib.c | 22 +++++++++ crypto/bn/bn_local.h | 26 +++++------ crypto/rsa/rsa_ossl.c | 13 +++--- 5 files changed, 101 insertions(+), 69 deletions(-) diff --git a/crypto/bn/bn_asm.c b/crypto/bn/bn_asm.c index 4d83a8cf11..177558c647 100644 --- a/crypto/bn/bn_asm.c +++ b/crypto/bn/bn_asm.c @@ -381,25 +381,33 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, #ifndef OPENSSL_SMALL_FOOTPRINT while (n & ~3) { t1 = a[0]; - t2 = b[0]; - r[0] = (t1 - t2 - c) & BN_MASK2; - if (t1 != t2) - c = (t1 < t2); + t2 = (t1 - c) & BN_MASK2; + c = (t2 > t1); + t1 = b[0]; + t1 = (t2 - t1) & BN_MASK2; + r[0] = t1; + c += (t1 > t2); t1 = a[1]; - t2 = b[1]; - r[1] = (t1 - t2 - c) & BN_MASK2; - if (t1 != t2) - c = (t1 < t2); + t2 = (t1 - c) & BN_MASK2; + c = (t2 > t1); + t1 = b[1]; + t1 = (t2 - t1) & BN_MASK2; + r[1] = t1; + c += (t1 > t2); t1 = a[2]; - t2 = b[2]; - r[2] = (t1 - t2 - c) & BN_MASK2; - if (t1 != t2) - c = (t1 < t2); + t2 = (t1 - c) & BN_MASK2; + c = (t2 > t1); + t1 = b[2]; + t1 = (t2 - t1) & BN_MASK2; + r[2] = t1; + c += (t1 > t2); t1 = a[3]; - t2 = b[3]; - r[3] = (t1 - t2 - c) & BN_MASK2; - if (t1 != t2) - c = (t1 < t2); + t2 = (t1 - c) & BN_MASK2; + c = (t2 > t1); + t1 = b[3]; + t1 = (t2 - t1) & BN_MASK2; + r[3] = t1; + c += (t1 > t2); a += 4; b += 4; r += 4; @@ -408,10 +416,12 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, #endif while (n) { t1 = a[0]; - t2 = b[0]; - r[0] = (t1 - t2 - c) & BN_MASK2; - if (t1 != t2) - c = (t1 < t2); + t2 = (t1 - c) & BN_MASK2; + c = (t2 > t1); + t1 = b[0]; + t1 = (t2 - t1) & BN_MASK2; + r[0] = t1; + c += (t1 > t2); a++; b++; r++; @@ -446,7 +456,7 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, t += c0; /* no carry */ \ c0 = (BN_ULONG)Lw(t); \ hi = (BN_ULONG)Hw(t); \ - c1 = (c1+hi)&BN_MASK2; if (c1top = (int)(rtop & ~mask) | (ntop & mask); n->flags |= (BN_FLG_FIXED_TOP & ~mask); } - ret = BN_mod_mul_montgomery(n, n, r, b->m_ctx, ctx); + ret = bn_mul_mont_fixed_top(n, n, r, b->m_ctx, ctx); + bn_correct_top_consttime(n); } else { ret = BN_mod_mul(n, n, r, b->mod, ctx); } diff --git a/crypto/bn/bn_lib.c b/crypto/bn/bn_lib.c index eb4a31849b..fe6fb0e40f 100644 --- a/crypto/bn/bn_lib.c +++ b/crypto/bn/bn_lib.c @@ -1001,6 +1001,28 @@ BIGNUM *bn_wexpand(BIGNUM *a, int words) return (words <= a->dmax) ? a : bn_expand2(a, words); } +void bn_correct_top_consttime(BIGNUM *a) +{ + int j, atop; + BN_ULONG limb; + unsigned int mask; + + for (j = 0, atop = 0; j < a->dmax; j++) { + limb = a->d[j]; + limb |= 0 - limb; + limb >>= BN_BITS2 - 1; + limb = 0 - limb; + mask = (unsigned int)limb; + mask &= constant_time_msb(j - a->top); + atop = constant_time_select_int(mask, j + 1, atop); + } + + mask = constant_time_eq_int(atop, 0); + a->top = atop; + a->neg = constant_time_select_int(mask, 0, a->neg); + a->flags &= ~BN_FLG_FIXED_TOP; +} + void bn_correct_top(BIGNUM *a) { BN_ULONG *ftl; diff --git a/crypto/bn/bn_local.h b/crypto/bn/bn_local.h index ee6342b60c..818e34348e 100644 --- a/crypto/bn/bn_local.h +++ b/crypto/bn/bn_local.h @@ -515,10 +515,10 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b, ret = (r); \ BN_UMULT_LOHI(low,high,w,tmp); \ ret += (c); \ - (c) = (ret<(c))?1:0; \ + (c) = (ret<(c)); \ (c) += high; \ ret += low; \ - (c) += (ret>(BN_BITS4-1); \ m =(m&BN_MASK2l)<<(BN_BITS4+1); \ - l=(l+m)&BN_MASK2; if (l < m) h++; \ + l=(l+m)&BN_MASK2; h += (l < m); \ (lo)=l; \ (ho)=h; \ } @@ -623,9 +623,9 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b, mul64(l,h,(bl),(bh)); \ \ /* non-multiply part */ \ - l=(l+(c))&BN_MASK2; if (l < (c)) h++; \ + l=(l+(c))&BN_MASK2; h += (l < (c)); \ (c)=(r); \ - l=(l+(c))&BN_MASK2; if (l < (c)) h++; \ + l=(l+(c))&BN_MASK2; h += (l < (c)); \ (c)=h&BN_MASK2; \ (r)=l; \ } @@ -639,7 +639,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b, mul64(l,h,(bl),(bh)); \ \ /* non-multiply part */ \ - l+=(c); if ((l&BN_MASK2) < (c)) h++; \ + l+=(c); h += ((l&BN_MASK2) < (c)); \ (c)=h&BN_MASK2; \ (r)=l&BN_MASK2; \ } @@ -669,7 +669,7 @@ BN_ULONG bn_sub_part_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b, int cl, int dl); int bn_mul_mont(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp, const BN_ULONG *np, const BN_ULONG *n0, int num); - +void bn_correct_top_consttime(BIGNUM *a); BIGNUM *int_bn_mod_inverse(BIGNUM *in, const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx, int *noinv); diff --git a/crypto/rsa/rsa_ossl.c b/crypto/rsa/rsa_ossl.c index 53cf2d03c9..cf5a10ab43 100644 --- a/crypto/rsa/rsa_ossl.c +++ b/crypto/rsa/rsa_ossl.c @@ -226,6 +226,7 @@ static int rsa_blinding_invert(BN_BLINDING *b, BIGNUM *f, BIGNUM *unblind, * will only read the modulus from BN_BLINDING. In both cases it's safe * to access the blinding without a lock. */ + BN_set_flags(f, BN_FLG_CONSTTIME); return BN_BLINDING_invert_ex(f, unblind, b, ctx); } @@ -412,6 +413,11 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from, goto err; } + if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) + if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, rsa->lock, + rsa->n, ctx)) + goto err; + if (!(rsa->flags & RSA_FLAG_NO_BLINDING)) { blinding = rsa_get_blinding(rsa, &local_blinding, ctx); if (blinding == NULL) { @@ -449,13 +455,6 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from, goto err; } BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME); - - if (rsa->flags & RSA_FLAG_CACHE_PUBLIC) - if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, rsa->lock, - rsa->n, ctx)) { - BN_free(d); - goto err; - } if (!rsa->meth->bn_mod_exp(ret, f, d, rsa->n, ctx, rsa->_method_mod_n)) { BN_free(d); -- 2.25.1