Skip to content

Commit a00d757

Browse files
bernd-edlingert8m
bernd-edlinger
authored and
t8m
committed
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 <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20283)
1 parent 908eace commit a00d757

File tree

6 files changed

+112
-69
lines changed

6 files changed

+112
-69
lines changed

‎CHANGES.md

+11
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,17 @@ breaking changes, and mappings for the large list of deprecated functions.
3030

3131
### Changes between 3.0.8 and 3.0.9 [xx XXX xxxx]
3232

33+
* Reworked the Fix for the Timing Oracle in RSA Decryption ([CVE-2022-4304]).
34+
The previous fix for this timing side channel turned out to cause
35+
a severe 2-3x performance regression in the typical use case
36+
compared to 3.0.7. The new fix uses existing constant time
37+
code paths, and restores the previous performance level while
38+
fully eliminating all existing timing side channels.
39+
The fix was developed by Bernd Edlinger with testing support
40+
by Hubert Kario.
41+
42+
*Bernd Edlinger*
43+
3344
* Corrected documentation of X509_VERIFY_PARAM_add0_policy() to mention
3445
that it does not enable policy checking. Thanks to David Benjamin for
3546
discovering this issue.

‎crypto/bn/bn_asm.c

+58-48
Original file line numberDiff line numberDiff line change
@@ -381,25 +381,33 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
381381
#ifndef OPENSSL_SMALL_FOOTPRINT
382382
while (n & ~3) {
383383
t1 = a[0];
384-
t2 = b[0];
385-
r[0] = (t1 - t2 - c) & BN_MASK2;
386-
if (t1 != t2)
387-
c = (t1 < t2);
384+
t2 = (t1 - c) & BN_MASK2;
385+
c = (t2 > t1);
386+
t1 = b[0];
387+
t1 = (t2 - t1) & BN_MASK2;
388+
r[0] = t1;
389+
c += (t1 > t2);
388390
t1 = a[1];
389-
t2 = b[1];
390-
r[1] = (t1 - t2 - c) & BN_MASK2;
391-
if (t1 != t2)
392-
c = (t1 < t2);
391+
t2 = (t1 - c) & BN_MASK2;
392+
c = (t2 > t1);
393+
t1 = b[1];
394+
t1 = (t2 - t1) & BN_MASK2;
395+
r[1] = t1;
396+
c += (t1 > t2);
393397
t1 = a[2];
394-
t2 = b[2];
395-
r[2] = (t1 - t2 - c) & BN_MASK2;
396-
if (t1 != t2)
397-
c = (t1 < t2);
398+
t2 = (t1 - c) & BN_MASK2;
399+
c = (t2 > t1);
400+
t1 = b[2];
401+
t1 = (t2 - t1) & BN_MASK2;
402+
r[2] = t1;
403+
c += (t1 > t2);
398404
t1 = a[3];
399-
t2 = b[3];
400-
r[3] = (t1 - t2 - c) & BN_MASK2;
401-
if (t1 != t2)
402-
c = (t1 < t2);
405+
t2 = (t1 - c) & BN_MASK2;
406+
c = (t2 > t1);
407+
t1 = b[3];
408+
t1 = (t2 - t1) & BN_MASK2;
409+
r[3] = t1;
410+
c += (t1 > t2);
403411
a += 4;
404412
b += 4;
405413
r += 4;
@@ -408,10 +416,12 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
408416
#endif
409417
while (n) {
410418
t1 = a[0];
411-
t2 = b[0];
412-
r[0] = (t1 - t2 - c) & BN_MASK2;
413-
if (t1 != t2)
414-
c = (t1 < t2);
419+
t2 = (t1 - c) & BN_MASK2;
420+
c = (t2 > t1);
421+
t1 = b[0];
422+
t1 = (t2 - t1) & BN_MASK2;
423+
r[0] = t1;
424+
c += (t1 > t2);
415425
a++;
416426
b++;
417427
r++;
@@ -446,7 +456,7 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
446456
t += c0; /* no carry */ \
447457
c0 = (BN_ULONG)Lw(t); \
448458
hi = (BN_ULONG)Hw(t); \
449-
c1 = (c1+hi)&BN_MASK2; if (c1<hi) c2++; \
459+
c1 = (c1+hi)&BN_MASK2; c2 += (c1<hi); \
450460
} while(0)
451461

452462
# define mul_add_c2(a,b,c0,c1,c2) do { \
@@ -455,11 +465,11 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
455465
BN_ULLONG tt = t+c0; /* no carry */ \
456466
c0 = (BN_ULONG)Lw(tt); \
457467
hi = (BN_ULONG)Hw(tt); \
458-
c1 = (c1+hi)&BN_MASK2; if (c1<hi) c2++; \
468+
c1 = (c1+hi)&BN_MASK2; c2 += (c1<hi); \
459469
t += c0; /* no carry */ \
460470
c0 = (BN_ULONG)Lw(t); \
461471
hi = (BN_ULONG)Hw(t); \
462-
c1 = (c1+hi)&BN_MASK2; if (c1<hi) c2++; \
472+
c1 = (c1+hi)&BN_MASK2; c2 += (c1<hi); \
463473
} while(0)
464474

465475
# define sqr_add_c(a,i,c0,c1,c2) do { \
@@ -468,7 +478,7 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
468478
t += c0; /* no carry */ \
469479
c0 = (BN_ULONG)Lw(t); \
470480
hi = (BN_ULONG)Hw(t); \
471-
c1 = (c1+hi)&BN_MASK2; if (c1<hi) c2++; \
481+
c1 = (c1+hi)&BN_MASK2; c2 += (c1<hi); \
472482
} while(0)
473483

474484
# define sqr_add_c2(a,i,j,c0,c1,c2) \
@@ -483,26 +493,26 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
483493
BN_ULONG ta = (a), tb = (b); \
484494
BN_ULONG lo, hi; \
485495
BN_UMULT_LOHI(lo,hi,ta,tb); \
486-
c0 += lo; hi += (c0<lo)?1:0; \
487-
c1 += hi; c2 += (c1<hi)?1:0; \
496+
c0 += lo; hi += (c0<lo); \
497+
c1 += hi; c2 += (c1<hi); \
488498
} while(0)
489499

490500
# define mul_add_c2(a,b,c0,c1,c2) do { \
491501
BN_ULONG ta = (a), tb = (b); \
492502
BN_ULONG lo, hi, tt; \
493503
BN_UMULT_LOHI(lo,hi,ta,tb); \
494-
c0 += lo; tt = hi+((c0<lo)?1:0); \
495-
c1 += tt; c2 += (c1<tt)?1:0; \
496-
c0 += lo; hi += (c0<lo)?1:0; \
497-
c1 += hi; c2 += (c1<hi)?1:0; \
504+
c0 += lo; tt = hi + (c0<lo); \
505+
c1 += tt; c2 += (c1<tt); \
506+
c0 += lo; hi += (c0<lo); \
507+
c1 += hi; c2 += (c1<hi); \
498508
} while(0)
499509

500510
# define sqr_add_c(a,i,c0,c1,c2) do { \
501511
BN_ULONG ta = (a)[i]; \
502512
BN_ULONG lo, hi; \
503513
BN_UMULT_LOHI(lo,hi,ta,ta); \
504-
c0 += lo; hi += (c0<lo)?1:0; \
505-
c1 += hi; c2 += (c1<hi)?1:0; \
514+
c0 += lo; hi += (c0<lo); \
515+
c1 += hi; c2 += (c1<hi); \
506516
} while(0)
507517

508518
# define sqr_add_c2(a,i,j,c0,c1,c2) \
@@ -517,26 +527,26 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
517527
BN_ULONG ta = (a), tb = (b); \
518528
BN_ULONG lo = ta * tb; \
519529
BN_ULONG hi = BN_UMULT_HIGH(ta,tb); \
520-
c0 += lo; hi += (c0<lo)?1:0; \
521-
c1 += hi; c2 += (c1<hi)?1:0; \
530+
c0 += lo; hi += (c0<lo); \
531+
c1 += hi; c2 += (c1<hi); \
522532
} while(0)
523533

524534
# define mul_add_c2(a,b,c0,c1,c2) do { \
525535
BN_ULONG ta = (a), tb = (b), tt; \
526536
BN_ULONG lo = ta * tb; \
527537
BN_ULONG hi = BN_UMULT_HIGH(ta,tb); \
528-
c0 += lo; tt = hi + ((c0<lo)?1:0); \
529-
c1 += tt; c2 += (c1<tt)?1:0; \
530-
c0 += lo; hi += (c0<lo)?1:0; \
531-
c1 += hi; c2 += (c1<hi)?1:0; \
538+
c0 += lo; tt = hi + (c0<lo); \
539+
c1 += tt; c2 += (c1<tt); \
540+
c0 += lo; hi += (c0<lo); \
541+
c1 += hi; c2 += (c1<hi); \
532542
} while(0)
533543

534544
# define sqr_add_c(a,i,c0,c1,c2) do { \
535545
BN_ULONG ta = (a)[i]; \
536546
BN_ULONG lo = ta * ta; \
537547
BN_ULONG hi = BN_UMULT_HIGH(ta,ta); \
538-
c0 += lo; hi += (c0<lo)?1:0; \
539-
c1 += hi; c2 += (c1<hi)?1:0; \
548+
c0 += lo; hi += (c0<lo); \
549+
c1 += hi; c2 += (c1<hi); \
540550
} while(0)
541551

542552
# define sqr_add_c2(a,i,j,c0,c1,c2) \
@@ -551,8 +561,8 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
551561
BN_ULONG lo = LBITS(a), hi = HBITS(a); \
552562
BN_ULONG bl = LBITS(b), bh = HBITS(b); \
553563
mul64(lo,hi,bl,bh); \
554-
c0 = (c0+lo)&BN_MASK2; if (c0<lo) hi++; \
555-
c1 = (c1+hi)&BN_MASK2; if (c1<hi) c2++; \
564+
c0 = (c0+lo)&BN_MASK2; hi += (c0<lo); \
565+
c1 = (c1+hi)&BN_MASK2; c2 += (c1<hi); \
556566
} while(0)
557567

558568
# define mul_add_c2(a,b,c0,c1,c2) do { \
@@ -561,17 +571,17 @@ BN_ULONG bn_sub_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
561571
BN_ULONG bl = LBITS(b), bh = HBITS(b); \
562572
mul64(lo,hi,bl,bh); \
563573
tt = hi; \
564-
c0 = (c0+lo)&BN_MASK2; if (c0<lo) tt++; \
565-
c1 = (c1+tt)&BN_MASK2; if (c1<tt) c2++; \
566-
c0 = (c0+lo)&BN_MASK2; if (c0<lo) hi++; \
567-
c1 = (c1+hi)&BN_MASK2; if (c1<hi) c2++; \
574+
c0 = (c0+lo)&BN_MASK2; tt += (c0<lo); \
575+
c1 = (c1+tt)&BN_MASK2; c2 += (c1<tt); \
576+
c0 = (c0+lo)&BN_MASK2; hi += (c0<lo); \
577+
c1 = (c1+hi)&BN_MASK2; c2 += (c1<hi); \
568578
} while(0)
569579

570580
# define sqr_add_c(a,i,c0,c1,c2) do { \
571581
BN_ULONG lo, hi; \
572582
sqr64(lo,hi,(a)[i]); \
573-
c0 = (c0+lo)&BN_MASK2; if (c0<lo) hi++; \
574-
c1 = (c1+hi)&BN_MASK2; if (c1<hi) c2++; \
583+
c0 = (c0+lo)&BN_MASK2; hi += (c0<lo); \
584+
c1 = (c1+hi)&BN_MASK2; c2 += (c1<hi); \
575585
} while(0)
576586

577587
# define sqr_add_c2(a,i,j,c0,c1,c2) \

‎crypto/bn/bn_blind.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,8 @@ int BN_BLINDING_invert_ex(BIGNUM *n, const BIGNUM *r, BN_BLINDING *b,
191191
n->top = (int)(rtop & ~mask) | (ntop & mask);
192192
n->flags |= (BN_FLG_FIXED_TOP & ~mask);
193193
}
194-
ret = BN_mod_mul_montgomery(n, n, r, b->m_ctx, ctx);
194+
ret = bn_mul_mont_fixed_top(n, n, r, b->m_ctx, ctx);
195+
bn_correct_top_consttime(n);
195196
} else {
196197
ret = BN_mod_mul(n, n, r, b->mod, ctx);
197198
}

‎crypto/bn/bn_lib.c

+22
Original file line numberDiff line numberDiff line change
@@ -1021,6 +1021,28 @@ BIGNUM *bn_wexpand(BIGNUM *a, int words)
10211021
return (words <= a->dmax) ? a : bn_expand2(a, words);
10221022
}
10231023

1024+
void bn_correct_top_consttime(BIGNUM *a)
1025+
{
1026+
int j, atop;
1027+
BN_ULONG limb;
1028+
unsigned int mask;
1029+
1030+
for (j = 0, atop = 0; j < a->dmax; j++) {
1031+
limb = a->d[j];
1032+
limb |= 0 - limb;
1033+
limb >>= BN_BITS2 - 1;
1034+
limb = 0 - limb;
1035+
mask = (unsigned int)limb;
1036+
mask &= constant_time_msb(j - a->top);
1037+
atop = constant_time_select_int(mask, j + 1, atop);
1038+
}
1039+
1040+
mask = constant_time_eq_int(atop, 0);
1041+
a->top = atop;
1042+
a->neg = constant_time_select_int(mask, 0, a->neg);
1043+
a->flags &= ~BN_FLG_FIXED_TOP;
1044+
}
1045+
10241046
void bn_correct_top(BIGNUM *a)
10251047
{
10261048
BN_ULONG *ftl;

‎crypto/bn/bn_local.h

+13-13
Original file line numberDiff line numberDiff line change
@@ -522,10 +522,10 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
522522
ret = (r); \
523523
BN_UMULT_LOHI(low,high,w,tmp); \
524524
ret += (c); \
525-
(c) = (ret<(c))?1:0; \
525+
(c) = (ret<(c)); \
526526
(c) += high; \
527527
ret += low; \
528-
(c) += (ret<low)?1:0; \
528+
(c) += (ret<low); \
529529
(r) = ret; \
530530
}
531531

@@ -534,7 +534,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
534534
BN_UMULT_LOHI(low,high,w,ta); \
535535
ret = low + (c); \
536536
(c) = high; \
537-
(c) += (ret<low)?1:0; \
537+
(c) += (ret<low); \
538538
(r) = ret; \
539539
}
540540

@@ -550,10 +550,10 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
550550
high= BN_UMULT_HIGH(w,tmp); \
551551
ret += (c); \
552552
low = (w) * tmp; \
553-
(c) = (ret<(c))?1:0; \
553+
(c) = (ret<(c)); \
554554
(c) += high; \
555555
ret += low; \
556-
(c) += (ret<low)?1:0; \
556+
(c) += (ret<low); \
557557
(r) = ret; \
558558
}
559559

@@ -563,7 +563,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
563563
high= BN_UMULT_HIGH(w,ta); \
564564
ret = low + (c); \
565565
(c) = high; \
566-
(c) += (ret<low)?1:0; \
566+
(c) += (ret<low); \
567567
(r) = ret; \
568568
}
569569

@@ -596,10 +596,10 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
596596
lt=(bl)*(lt); \
597597
m1=(bl)*(ht); \
598598
ht =(bh)*(ht); \
599-
m=(m+m1)&BN_MASK2; if (m < m1) ht+=L2HBITS((BN_ULONG)1); \
599+
m=(m+m1)&BN_MASK2; ht += L2HBITS((BN_ULONG)(m < m1)); \
600600
ht+=HBITS(m); \
601601
m1=L2HBITS(m); \
602-
lt=(lt+m1)&BN_MASK2; if (lt < m1) ht++; \
602+
lt=(lt+m1)&BN_MASK2; ht += (lt < m1); \
603603
(l)=lt; \
604604
(h)=ht; \
605605
}
@@ -616,7 +616,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
616616
h*=h; \
617617
h+=(m&BN_MASK2h1)>>(BN_BITS4-1); \
618618
m =(m&BN_MASK2l)<<(BN_BITS4+1); \
619-
l=(l+m)&BN_MASK2; if (l < m) h++; \
619+
l=(l+m)&BN_MASK2; h += (l < m); \
620620
(lo)=l; \
621621
(ho)=h; \
622622
}
@@ -630,9 +630,9 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
630630
mul64(l,h,(bl),(bh)); \
631631
\
632632
/* non-multiply part */ \
633-
l=(l+(c))&BN_MASK2; if (l < (c)) h++; \
633+
l=(l+(c))&BN_MASK2; h += (l < (c)); \
634634
(c)=(r); \
635-
l=(l+(c))&BN_MASK2; if (l < (c)) h++; \
635+
l=(l+(c))&BN_MASK2; h += (l < (c)); \
636636
(c)=h&BN_MASK2; \
637637
(r)=l; \
638638
}
@@ -646,7 +646,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
646646
mul64(l,h,(bl),(bh)); \
647647
\
648648
/* non-multiply part */ \
649-
l+=(c); if ((l&BN_MASK2) < (c)) h++; \
649+
l+=(c); h += ((l&BN_MASK2) < (c)); \
650650
(c)=h&BN_MASK2; \
651651
(r)=l&BN_MASK2; \
652652
}
@@ -676,7 +676,7 @@ BN_ULONG bn_sub_part_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
676676
int cl, int dl);
677677
int bn_mul_mont(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp,
678678
const BN_ULONG *np, const BN_ULONG *n0, int num);
679-
679+
void bn_correct_top_consttime(BIGNUM *a);
680680
BIGNUM *int_bn_mod_inverse(BIGNUM *in,
681681
const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx,
682682
int *noinv);

‎crypto/rsa/rsa_ossl.c

+6-7
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ static int rsa_blinding_invert(BN_BLINDING *b, BIGNUM *f, BIGNUM *unblind,
234234
* will only read the modulus from BN_BLINDING. In both cases it's safe
235235
* to access the blinding without a lock.
236236
*/
237+
BN_set_flags(f, BN_FLG_CONSTTIME);
237238
return BN_BLINDING_invert_ex(f, unblind, b, ctx);
238239
}
239240

@@ -416,6 +417,11 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from,
416417
goto err;
417418
}
418419

420+
if (rsa->flags & RSA_FLAG_CACHE_PUBLIC)
421+
if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, rsa->lock,
422+
rsa->n, ctx))
423+
goto err;
424+
419425
if (!(rsa->flags & RSA_FLAG_NO_BLINDING)) {
420426
blinding = rsa_get_blinding(rsa, &local_blinding, ctx);
421427
if (blinding == NULL) {
@@ -453,13 +459,6 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from,
453459
goto err;
454460
}
455461
BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
456-
457-
if (rsa->flags & RSA_FLAG_CACHE_PUBLIC)
458-
if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, rsa->lock,
459-
rsa->n, ctx)) {
460-
BN_free(d);
461-
goto err;
462-
}
463462
if (!rsa->meth->bn_mod_exp(ret, f, d, rsa->n, ctx,
464463
rsa->_method_mod_n)) {
465464
BN_free(d);

0 commit comments

Comments
 (0)