From 71844800d543162f709c6a223d993a50506028c2 Mon Sep 17 00:00:00 2001 From: Pauli Date: Wed, 1 Nov 2017 06:58:13 +1000 Subject: [PATCH] Address a timing side channel whereby it is possible to determine some information about the length of a value used in DSA operations from a large number of signatures. This doesn't rate as a CVE because: * For the non-constant time code, there are easier ways to extract more information. * For the constant time code, it requires a significant number of signatures to leak a small amount of information. Thanks to Neals Fournaise, Eliane Jaulmes and Jean-Rene Reinhard for reporting this issue. Reviewed-by: Andy Polyakov Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/4576) (cherry picked from commit c0caa945f6ef30363e0d01d75155f20248403df4) --- crypto/dsa/dsa_ossl.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/crypto/dsa/dsa_ossl.c b/crypto/dsa/dsa_ossl.c index 479337763b..7f48cf2e33 100644 --- a/crypto/dsa/dsa_ossl.c +++ b/crypto/dsa/dsa_ossl.c @@ -148,7 +148,9 @@ static int dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, { BN_CTX *ctx = NULL; BIGNUM *k, *kinv = NULL, *r = *rp; + BIGNUM *l, *m; int ret = 0; + int q_bits; if (!dsa->p || !dsa->q || !dsa->g) { DSAerr(DSA_F_DSA_SIGN_SETUP, DSA_R_MISSING_PARAMETERS); @@ -156,7 +158,9 @@ static int dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, } k = BN_new(); - if (k == NULL) + l = BN_new(); + m = BN_new(); + if (k == NULL || l == NULL || m == NULL) goto err; if (ctx_in == NULL) { @@ -165,6 +169,13 @@ static int dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, } else ctx = ctx_in; + /* Preallocate space */ + q_bits = BN_num_bits(dsa->q); + if (!BN_set_bit(k, q_bits) + || !BN_set_bit(l, q_bits) + || !BN_set_bit(m, q_bits)) + goto err; + /* Get random k */ do { if (dgst != NULL) { @@ -191,17 +202,19 @@ static int dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, /* * We do not want timing information to leak the length of k, so we - * compute g^k using an equivalent exponent of fixed length. (This - * is a kludge that we need because the BN_mod_exp_mont() does not - * let us specify the desired timing behaviour.) + * compute G^k using an equivalent scalar of fixed bit-length. + * + * We unconditionally perform both of these additions to prevent a + * small timing information leakage. We then choose the sum that is + * one bit longer than the modulus. + * + * TODO: revisit the BN_copy aiming for a memory access agnostic + * conditional copy. */ - - if (!BN_add(k, k, dsa->q)) + if (!BN_add(l, k, dsa->q) + || !BN_add(m, l, dsa->q) + || !BN_copy(k, BN_num_bits(l) > q_bits ? l : m)) goto err; - if (BN_num_bits(k) <= BN_num_bits(dsa->q)) { - if (!BN_add(k, k, dsa->q)) - goto err; - } if ((dsa)->meth->bn_mod_exp != NULL) { if (!dsa->meth->bn_mod_exp(dsa, r, dsa->g, k, dsa->p, ctx, @@ -229,6 +242,8 @@ static int dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, if (ctx != ctx_in) BN_CTX_free(ctx); BN_clear_free(k); + BN_clear_free(l); + BN_clear_free(m); return ret; } -- 2.25.1