Skip to content

Commit 3f499b2

Browse files
bernd-edlingerbernd-edlinger
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 #20284)
1 parent 0372649 commit 3f499b2

File tree

6 files changed

+111
-69
lines changed

6 files changed

+111
-69
lines changed

‎CHANGES

+10
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,16 @@
99

1010
Changes between 1.1.1t and 1.1.1u [xx XXX xxxx]
1111

12+
*) Reworked the Fix for the Timing Oracle in RSA Decryption (CVE-2022-4304).
13+
The previous fix for this timing side channel turned out to cause
14+
a severe 2-3x performance regression in the typical use case
15+
compared to 1.1.1s. The new fix uses existing constant time
16+
code paths, and restores the previous performance level while
17+
fully eliminating all existing timing side channels.
18+
The fix was developed by Bernd Edlinger with testing support
19+
by Hubert Kario.
20+
[Bernd Edlinger]
21+
1222
*) Corrected documentation of X509_VERIFY_PARAM_add0_policy() to mention
1323
that it does not enable policy checking. Thanks to
1424
David Benjamin for discovering this issue. (CVE-2023-0466)

‎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
@@ -1001,6 +1001,28 @@ BIGNUM *bn_wexpand(BIGNUM *a, int words)
10011001
return (words <= a->dmax) ? a : bn_expand2(a, words);
10021002
}
10031003

1004+
void bn_correct_top_consttime(BIGNUM *a)
1005+
{
1006+
int j, atop;
1007+
BN_ULONG limb;
1008+
unsigned int mask;
1009+
1010+
for (j = 0, atop = 0; j < a->dmax; j++) {
1011+
limb = a->d[j];
1012+
limb |= 0 - limb;
1013+
limb >>= BN_BITS2 - 1;
1014+
limb = 0 - limb;
1015+
mask = (unsigned int)limb;
1016+
mask &= constant_time_msb(j - a->top);
1017+
atop = constant_time_select_int(mask, j + 1, atop);
1018+
}
1019+
1020+
mask = constant_time_eq_int(atop, 0);
1021+
a->top = atop;
1022+
a->neg = constant_time_select_int(mask, 0, a->neg);
1023+
a->flags &= ~BN_FLG_FIXED_TOP;
1024+
}
1025+
10041026
void bn_correct_top(BIGNUM *a)
10051027
{
10061028
BN_ULONG *ftl;

‎crypto/bn/bn_local.h

+13-13
Original file line numberDiff line numberDiff line change
@@ -515,10 +515,10 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
515515
ret = (r); \
516516
BN_UMULT_LOHI(low,high,w,tmp); \
517517
ret += (c); \
518-
(c) = (ret<(c))?1:0; \
518+
(c) = (ret<(c)); \
519519
(c) += high; \
520520
ret += low; \
521-
(c) += (ret<low)?1:0; \
521+
(c) += (ret<low); \
522522
(r) = ret; \
523523
}
524524

@@ -527,7 +527,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
527527
BN_UMULT_LOHI(low,high,w,ta); \
528528
ret = low + (c); \
529529
(c) = high; \
530-
(c) += (ret<low)?1:0; \
530+
(c) += (ret<low); \
531531
(r) = ret; \
532532
}
533533

@@ -543,10 +543,10 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
543543
high= BN_UMULT_HIGH(w,tmp); \
544544
ret += (c); \
545545
low = (w) * tmp; \
546-
(c) = (ret<(c))?1:0; \
546+
(c) = (ret<(c)); \
547547
(c) += high; \
548548
ret += low; \
549-
(c) += (ret<low)?1:0; \
549+
(c) += (ret<low); \
550550
(r) = ret; \
551551
}
552552

@@ -556,7 +556,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
556556
high= BN_UMULT_HIGH(w,ta); \
557557
ret = low + (c); \
558558
(c) = high; \
559-
(c) += (ret<low)?1:0; \
559+
(c) += (ret<low); \
560560
(r) = ret; \
561561
}
562562

@@ -589,10 +589,10 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
589589
lt=(bl)*(lt); \
590590
m1=(bl)*(ht); \
591591
ht =(bh)*(ht); \
592-
m=(m+m1)&BN_MASK2; if (m < m1) ht+=L2HBITS((BN_ULONG)1); \
592+
m=(m+m1)&BN_MASK2; ht += L2HBITS((BN_ULONG)(m < m1)); \
593593
ht+=HBITS(m); \
594594
m1=L2HBITS(m); \
595-
lt=(lt+m1)&BN_MASK2; if (lt < m1) ht++; \
595+
lt=(lt+m1)&BN_MASK2; ht += (lt < m1); \
596596
(l)=lt; \
597597
(h)=ht; \
598598
}
@@ -609,7 +609,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
609609
h*=h; \
610610
h+=(m&BN_MASK2h1)>>(BN_BITS4-1); \
611611
m =(m&BN_MASK2l)<<(BN_BITS4+1); \
612-
l=(l+m)&BN_MASK2; if (l < m) h++; \
612+
l=(l+m)&BN_MASK2; h += (l < m); \
613613
(lo)=l; \
614614
(ho)=h; \
615615
}
@@ -623,9 +623,9 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
623623
mul64(l,h,(bl),(bh)); \
624624
\
625625
/* non-multiply part */ \
626-
l=(l+(c))&BN_MASK2; if (l < (c)) h++; \
626+
l=(l+(c))&BN_MASK2; h += (l < (c)); \
627627
(c)=(r); \
628-
l=(l+(c))&BN_MASK2; if (l < (c)) h++; \
628+
l=(l+(c))&BN_MASK2; h += (l < (c)); \
629629
(c)=h&BN_MASK2; \
630630
(r)=l; \
631631
}
@@ -639,7 +639,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
639639
mul64(l,h,(bl),(bh)); \
640640
\
641641
/* non-multiply part */ \
642-
l+=(c); if ((l&BN_MASK2) < (c)) h++; \
642+
l+=(c); h += ((l&BN_MASK2) < (c)); \
643643
(c)=h&BN_MASK2; \
644644
(r)=l&BN_MASK2; \
645645
}
@@ -669,7 +669,7 @@ BN_ULONG bn_sub_part_words(BN_ULONG *r, const BN_ULONG *a, const BN_ULONG *b,
669669
int cl, int dl);
670670
int bn_mul_mont(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp,
671671
const BN_ULONG *np, const BN_ULONG *n0, int num);
672-
672+
void bn_correct_top_consttime(BIGNUM *a);
673673
BIGNUM *int_bn_mod_inverse(BIGNUM *in,
674674
const BIGNUM *a, const BIGNUM *n, BN_CTX *ctx,
675675
int *noinv);

‎crypto/rsa/rsa_ossl.c

+6-7
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,7 @@ static int rsa_blinding_invert(BN_BLINDING *b, BIGNUM *f, BIGNUM *unblind,
226226
* will only read the modulus from BN_BLINDING. In both cases it's safe
227227
* to access the blinding without a lock.
228228
*/
229+
BN_set_flags(f, BN_FLG_CONSTTIME);
229230
return BN_BLINDING_invert_ex(f, unblind, b, ctx);
230231
}
231232

@@ -412,6 +413,11 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from,
412413
goto err;
413414
}
414415

416+
if (rsa->flags & RSA_FLAG_CACHE_PUBLIC)
417+
if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, rsa->lock,
418+
rsa->n, ctx))
419+
goto err;
420+
415421
if (!(rsa->flags & RSA_FLAG_NO_BLINDING)) {
416422
blinding = rsa_get_blinding(rsa, &local_blinding, ctx);
417423
if (blinding == NULL) {
@@ -449,13 +455,6 @@ static int rsa_ossl_private_decrypt(int flen, const unsigned char *from,
449455
goto err;
450456
}
451457
BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME);
452-
453-
if (rsa->flags & RSA_FLAG_CACHE_PUBLIC)
454-
if (!BN_MONT_CTX_set_locked(&rsa->_method_mod_n, rsa->lock,
455-
rsa->n, ctx)) {
456-
BN_free(d);
457-
goto err;
458-
}
459458
if (!rsa->meth->bn_mod_exp(ret, f, d, rsa->n, ctx,
460459
rsa->_method_mod_n)) {
461460
BN_free(d);

0 commit comments

Comments
 (0)