kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Deny CRT to unbalanced RSA keys.

Browse Source 
Our technique to perform the reduction only works for balanced key
sizes. For unbalanced keys, we fall back to variable-time logic.
Instead, fall back earlier to the non-CRT codepath, which is still
secure, just slower. This also aligns with the advice here:

https://github.com/HACS-workshop/spectre-mitigations/blob/master/crypto_guidelines.md#1-do-not-conditionally-choose-between-constant-and-non-constant-time

Update-Note: This is a performance hit (some keys will run 3x slower),
but only for keys with different-sized primes. I believe the Windows
crypto APIs will not accept such keys at all. There are two scenarios to
be concerned with for RSA performance:

1. Performance of reasonably-generated keys. Keys that BoringSSL or
anyone else reasonable generates will all be balanced, so this change
does not affect them.

2. Worst-case performance for DoS purposes. This CL does not change the
worst-case performance for RSA at a given bit size. In fact, it improves
it slightly. A sufficiently unbalanced RSA key is as slow as not doing
CRT at all.

In both cases, this change does not affect performance. The affected
keys are pathologically-generated ones that were not quite pathological
enough.

Bug: 235
Change-Id: Ie298dabb549ab9108fa9374aa86ebffe8b6c6c88
Reviewed-on: https://boringssl-review.googlesource.com/27504
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This commit is contained in:
David Benjamin 2018-01-26 17:22:27 -05:00 committed by CQ bot account: commit-bot@chromium.org
parent 024f5df3c8
commit d319205007
1 changed files with 11 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
crypto/fipsmodule/rsa/rsa_impl.c
View File

@ -715,7 +715,13 @@ int rsa_default_private_transform(RSA *rsa, uint8_t *out, const uint8_t *in,
}
if (rsa->p != NULL && rsa->q != NULL && rsa->e != NULL && rsa->dmp1 != NULL &&
rsa->dmq1 != NULL && rsa->iqmp != NULL) {
rsa->dmq1 != NULL && rsa->iqmp != NULL &&
// Require that we can reduce |f| by |rsa->p| and |rsa->q| in constant
// time, which requires primes be the same size, rounded to the Montgomery
// coefficient. (See |mod_montgomery|.) This is not required by RFC 8017,
// but it is true for keys generated by us and all common implementations.
bn_less_than_montgomery_R(rsa->q, rsa->mont_p) &&
bn_less_than_montgomery_R(rsa->p, rsa->mont_q)) {
if (!mod_exp(result, f, rsa, ctx)) {
goto err;
}
@ -780,11 +786,11 @@ static int mod_montgomery(BIGNUM *r, const BIGNUM *I, const BIGNUM *p,
const BN_MONT_CTX *mont_p, const BIGNUM *q,
BN_CTX *ctx) {
// Reducing in constant-time with Montgomery reduction requires I <= p * R. We
// have I < p * q, so this follows if q < R. In particular, this always holds
// if p and q are the same size, which is true for any RSA keys we or anyone
// sane generates. For other keys, we fall back to |BN_mod|.
// have I < p * q, so this follows if q < R. The caller should have checked
// this already.
if (!bn_less_than_montgomery_R(q, mont_p)) {
return BN_mod(r, I, p, ctx);
OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);
return 0;
}
if (// Reduce mod p with Montgomery reduction. This computes I * R^-1 mod p.