Fix threading issues with RSA freeze_private_key.
OpenSSL's RSA API is poorly designed and does not have a single place to
properly initialize the key. See
https://github.com/openssl/openssl/issues/5158.
To workaround this flaw, we must lazily instantiate pre-computed
Montgomery bits with locking. This is a ton of complexity. More
importantly, it makes it very difficult to implement RSA without side
channels. The correct in-memory representation of d, dmp1, and dmq1
depend on n, p, and q, respectively. (Those values have private
magnitudes and must be sized relative to the respective moduli.)
08805fe27910e09d05e87d61bc5411a4e3b2d999 attempted to fix up the various
widths under lock, when we set up BN_MONT_CTX. However, this introduces
threading issues because other threads may access those exposed
components (RSA_get0_* also count as exposed for these purposes because
they are get0 functions), while a private key operation is in progress.
Instead, we do the following:
- There is no actual need to minimize n, p, and q, but we have minimized
copies in the BN_MONT_CTXs, so use those.
- Store additional copies of d, dmp1, and dmq1, at the cost of more
memory used. These copies have the correct width and are private,
unlike d, dmp1, and dmq1 which are sadly exposed. Fix private key
operations to use them.
- Move the frozen bit out of rsa->flags, as that too was historically
accessible without locking.
(Serialization still uses the original BIGNUMs, but the RSAPrivateKey
serialization format already inherently leaks the magnitude, so this
doesn't matter.)
Change-Id: Ia3a9b0629f8efef23abb30bfed110d247d1db42f
Reviewed-on: https://boringssl-review.googlesource.com/25824
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>
diff --git a/include/openssl/rsa.h b/include/openssl/rsa.h
index 59f008e..7059e7c 100644
--- a/include/openssl/rsa.h
+++ b/include/openssl/rsa.h
@@ -509,10 +509,6 @@
// 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.
@@ -640,6 +636,9 @@
struct rsa_st {
RSA_METHOD *meth;
+ // Access to the following fields was historically allowed, but
+ // deprecated. Use |RSA_get0_*| and |RSA_set0_*| instead. Access to all other
+ // fields is forbidden and will cause threading errors.
BIGNUM *n;
BIGNUM *e;
BIGNUM *d;
@@ -662,6 +661,13 @@
BN_MONT_CTX *mont_p;
BN_MONT_CTX *mont_q;
+ // The following fields are copies of |d|, |dmp1|, and |dmq1|, respectively,
+ // but with the correct widths to prevent side channels. These must use
+ // separate copies due to threading concerns caused by OpenSSL's API
+ // mistakes. See https://github.com/openssl/openssl/issues/5158 and
+ // the |freeze_private_key| implementation.
+ BIGNUM *d_fixed, *dmp1_fixed, *dmq1_fixed;
+
// inv_small_mod_large_mont is q^-1 mod p in Montgomery form, using |mont_p|,
// if |p| >= |q|. Otherwise, it is p^-1 mod q in Montgomery form, using
// |mont_q|.
@@ -676,6 +682,10 @@
// |blindings_inuse| from 0 to 1.
BN_BLINDING **blindings;
unsigned char *blindings_inuse;
+
+ // private_key_frozen is one if the key has been used for a private key
+ // operation and may no longer be mutated.
+ unsigned private_key_frozen:1;
};
