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

Normalize RSA private component widths.

Browse Source 
d, dmp1, dmq1, and iqmp have private magnitudes. This is awkward because
the RSAPrivateKey serialization leaks the magnitudes. Do the best we can
and fix them up before any RSA operations.

This moves the piecemeal BN_MONT_CTX_set_locked into a common function
where we can do more complex canonicalization on the keys.  Ideally this
would be done on key import, but the exposed struct (and OpenSSL 1.1.0's
bad API design) mean there is no single point in time when key import is
finished.

Also document the constraints on RSA_set0_* functions. (These
constraints aren't new. They just were never documented before.)

Update-Note: If someone tried to use an invalid RSA key where d >= n,
   dmp1 >= p, dmq1 >= q, or iqmp >= p, this may break. Such keys would not
   have passed RSA_check_key, but it's possible to manually assemble
   keys that bypass it.
Bug: 232
Change-Id: I421f883128952f892ac0cde0d224873a625f37c5
Reviewed-on: https://boringssl-review.googlesource.com/25259
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This commit is contained in:
David Benjamin 2018-01-23 16:48:53 -05:00 committed by CQ bot account: commit-bot@chromium.org
parent c7b6e0a664
commit 08805fe279
2 changed files with 100 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -109,6 +109,82 @@ static int check_modulus_and_exponent_sizes(const RSA *rsa) {
return 1;
}
// freeze_private_key finishes initializing |rsa|'s private key components.
// After this function has returned, |rsa| may not be changed. This is needed
// because |RSA| is a public struct and, additionally, OpenSSL 1.1.0 opaquified
// it wrong (see https://github.com/openssl/openssl/issues/5158).
static int freeze_private_key(RSA *rsa, BN_CTX *ctx) {
CRYPTO_MUTEX_lock_read(&rsa->lock);
int flags = rsa->flags;
CRYPTO_MUTEX_unlock_read(&rsa->lock);
if (flags & RSA_FLAG_PRIVATE_KEY_FROZEN) {
return 1;
}
int ret = 0;
CRYPTO_MUTEX_lock_write(&rsa->lock);
if (rsa->flags & RSA_FLAG_PRIVATE_KEY_FROZEN) {
ret = 1;
goto err;
}
// |rsa->n| is public. Normalize the width.
bn_set_minimal_width(rsa->n);
if (rsa->mont_n == NULL) {
rsa->mont_n = BN_MONT_CTX_new_for_modulus(rsa->n, ctx);
if (rsa->mont_n == NULL) {
goto err;
}
}
// The only public upper-bound of |rsa->d| is the bit length of |rsa->n|. The
// ASN.1 serialization of RSA private keys unfortunately leaks the byte length
// of |rsa->d|, but normalize it so we only leak it once, rather than per
// operation.
if (rsa->d != NULL &&
!bn_resize_words(rsa->d, rsa->n->width)) {
goto err;
}
if (rsa->p != NULL && rsa->q != NULL) {
// |p| and |q| have public bit lengths.
bn_set_minimal_width(rsa->p);
bn_set_minimal_width(rsa->q);
if (rsa->mont_p == NULL) {
rsa->mont_p = BN_MONT_CTX_new_for_modulus(rsa->p, ctx);
if (rsa->mont_p == NULL) {
goto err;
}
}
if (rsa->mont_q == NULL) {
rsa->mont_q = BN_MONT_CTX_new_for_modulus(rsa->q, ctx);
if (rsa->mont_q == NULL) {
goto err;
}
}
// CRT components are only publicly bounded by their corresponding moduli's
// bit lengths.
if ((rsa->dmp1 != NULL &&
!bn_resize_words(rsa->dmp1, rsa->p->width)) ||
(rsa->dmq1 != NULL &&
!bn_resize_words(rsa->dmq1, rsa->q->width)) ||
(rsa->iqmp != NULL &&
!bn_resize_words(rsa->iqmp, rsa->p->width))) {
goto err;
}
}
rsa->flags |= RSA_FLAG_PRIVATE_KEY_FROZEN;
ret = 1;
err:
CRYPTO_MUTEX_unlock_write(&rsa->lock);
return ret;
}
size_t rsa_default_size(const RSA *rsa) {
return BN_num_bytes(rsa->n);
}
@ -560,7 +636,7 @@ int rsa_default_private_transform(RSA *rsa, uint8_t *out, const uint8_t *in,
goto err;
}
if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx)) {
if (!freeze_private_key(rsa, ctx)) {
OPENSSL_PUT_ERROR(RSA, ERR_R_INTERNAL_ERROR);
goto err;
}
@ -694,12 +770,7 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) {
goto err;
}
if (!BN_MONT_CTX_set_locked(&rsa->mont_p, &rsa->lock, rsa->p, ctx) ||
!BN_MONT_CTX_set_locked(&rsa->mont_q, &rsa->lock, rsa->q, ctx)) {
goto err;
}
if (!BN_MONT_CTX_set_locked(&rsa->mont_n, &rsa->lock, rsa->n, ctx)) {
if (!freeze_private_key(rsa, ctx)) {
goto err;
}
@ -727,20 +798,17 @@ static int mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) {
goto err;
}
// TODO(davidben): The code below is not constant-time, even ignoring
// |bn_correct_top|. To fix this:
// TODO(davidben): The code below is not constant-time:
//
// 1. Canonicalize keys on p > q. (p > q for keys we generate, but not ones we
// import.) We have exposed structs, but we can generalize the
// |BN_MONT_CTX_set_locked| trick to do a one-time canonicalization of the
// private key where we optionally swap p and q (re-computing iqmp if
// necessary) and fill in mont_*. This removes the p < q case below.
// 1. Finish adding support for non-minimal |BIGNUM|s.
//
// 2. Compute r0 - m1 (mod p) in constant-time. With (1) done, this is just a
// constant-time modular subtraction. It should be doable with
// |bn_sub_words| and a select on the borrow bit.
// 2. Canonicalize keys on p > q in |freeze_private_key|. (p > q for keys we
// generate, but not ones we import.) This removes the p < q case below.
//
// 3. When computing mont_*, additionally compute iqmp_mont, iqmp in
// 3. Make |BN_mod_sub_quick| constant-time (use |bn_sub_words| and select on
// the borrow bit) and compute r0 - m1 (mod p) with it.
//
// 4. When computing mont_*, additionally compute iqmp_mont, iqmp in
// Montgomery form. The |BN_mul| and |BN_mod| pair can then be replaced
// with |BN_mod_mul_montgomery|.
@ -1055,7 +1123,7 @@ int RSA_generate_key_ex(RSA *rsa, int bits, BIGNUM *e_value, BN_GENCB *cb) {
// from constant-time, |bn_mod_inverse_secret_prime| uses the same modular
// exponentation logic as in RSA private key operations and, if the RSAZ-1024
// code is enabled, will be optimized for common RSA prime sizes.
if (!BN_MONT_CTX_set_locked(&rsa->mont_p, &rsa->lock, rsa->p, ctx) ||
if (!freeze_private_key(rsa, ctx) ||
!bn_mod_inverse_secret_prime(rsa->iqmp, rsa->q, rsa->p, ctx,
rsa->mont_p)) {
goto bn_err;

13
include/openssl/rsa.h
View File

@ -117,6 +117,9 @@ OPENSSL_EXPORT void RSA_get0_crt_params(const RSA *rsa, const BIGNUM **out_dmp1,
//
// |d| may be NULL, but |n| and |e| must either be non-NULL or already
// configured on |rsa|.
//
// It is an error to call this function after |rsa| has been used for a
// cryptographic operation. Construct a new |RSA| object instead.
OPENSSL_EXPORT int RSA_set0_key(RSA *rsa, BIGNUM *n, BIGNUM *e, BIGNUM *d);
// RSA_set0_factors sets |rsa|'s prime factors to |p| and |q|, if non-NULL, and
@ -124,6 +127,9 @@ OPENSSL_EXPORT int RSA_set0_key(RSA *rsa, BIGNUM *n, BIGNUM *e, BIGNUM *d);
// returns one. Otherwise, it returns zero.
//
// Each argument must either be non-NULL or already configured on |rsa|.
//
// It is an error to call this function after |rsa| has been used for a
// cryptographic operation. Construct a new |RSA| object instead.
OPENSSL_EXPORT int RSA_set0_factors(RSA *rsa, BIGNUM *p, BIGNUM *q);
// RSA_set0_crt_params sets |rsa|'s CRT parameters to |dmp1|, |dmq1|, and
@ -131,6 +137,9 @@ OPENSSL_EXPORT int RSA_set0_factors(RSA *rsa, BIGNUM *p, BIGNUM *q);
// ownership of its parameters and returns one. Otherwise, it returns zero.
//
// Each argument must either be non-NULL or already configured on |rsa|.
//
// It is an error to call this function after |rsa| has been used for a
// cryptographic operation. Construct a new |RSA| object instead.
OPENSSL_EXPORT int RSA_set0_crt_params(RSA *rsa, BIGNUM *dmp1, BIGNUM *dmq1,
BIGNUM *iqmp);
@ -500,6 +509,10 @@ OPENSSL_EXPORT void *RSA_get_ex_data(const RSA *rsa, int idx);
// RSA_FLAG_EXT_PKEY is deprecated and ignored.
#define RSA_FLAG_EXT_PKEY 0x20
// RSA_FLAG_PRIVATE_KEY_FROZEN specifies that the key has been used for a
// private key operation and may no longer be mutated.
#define RSA_FLAG_PRIVATE_KEY_FROZEN 0x40
// RSA public exponent values.