Reads good to me, and passes the regress here, so OK from me.
On Mon, Jun 20, 2016 at 04:40:25AM -0500, Brent Cook wrote: > Hi, > > This is a patch from Cesar Pereida, removing support for > DSA_FLAG_NO_EXP_CONSTTIME by making DSA always operate in constant time. > > See https://github.com/libressl-portable/openbsd/pull/61 for more > details. > > ok? > > diff --git a/src/lib/libssl/src/crypto/dsa/dsa.h > b/src/lib/libssl/src/crypto/dsa/dsa.h > index 909096d..d2d1d5f 100644 > --- a/src/lib/libssl/src/crypto/dsa/dsa.h > +++ b/src/lib/libssl/src/crypto/dsa/dsa.h > @@ -89,12 +89,8 @@ > #endif > > #define DSA_FLAG_CACHE_MONT_P 0x01 > -#define DSA_FLAG_NO_EXP_CONSTTIME 0x02 /* new with 0.9.7h; the > built-in DSA > - * implementation now uses > constant time > - * modular exponentiation for > secret exponents > - * by default. This flag causes > the > - * faster variable sliding > window method to > - * be used for all exponents. > +#define DSA_FLAG_NO_EXP_CONSTTIME 0x00 /* Does nothing. Previously > this switched off > + * constant time behaviour. > */ > > /* If this flag is set the DSA method is FIPS compliant and can be used > diff --git a/src/lib/libssl/src/crypto/dsa/dsa_key.c > b/src/lib/libssl/src/crypto/dsa/dsa_key.c > index 2968fa2..e01bacb 100644 > --- a/src/lib/libssl/src/crypto/dsa/dsa_key.c > +++ b/src/lib/libssl/src/crypto/dsa/dsa_key.c > @@ -104,18 +104,18 @@ dsa_builtin_keygen(DSA *dsa) > pub_key=dsa->pub_key; > > { > - BIGNUM local_prk; > - BIGNUM *prk; > + BIGNUM *prk = BN_new(); > > - if ((dsa->flags & DSA_FLAG_NO_EXP_CONSTTIME) == 0) { > - BN_init(&local_prk); > - prk = &local_prk; > - BN_with_flags(prk, priv_key, BN_FLG_CONSTTIME); > - } else > - prk = priv_key; > + if (prk == NULL) > + goto err; > + > + BN_with_flags(prk, priv_key, BN_FLG_CONSTTIME); > > - if (!BN_mod_exp(pub_key, dsa->g, prk, dsa->p, ctx)) > + if (!BN_mod_exp(pub_key, dsa->g, prk, dsa->p, ctx)) { > + BN_free(prk); > goto err; > + } > + BN_free(prk); > } > > dsa->priv_key = priv_key; > @@ -130,4 +130,4 @@ err: > BN_CTX_free(ctx); > return ok; > } > -#endif > +#endif > \ No newline at end of file > diff --git a/src/lib/libssl/src/crypto/dsa/dsa_ossl.c > b/src/lib/libssl/src/crypto/dsa/dsa_ossl.c > index 726e6c7..4a3b417 100644 > --- a/src/lib/libssl/src/crypto/dsa/dsa_ossl.c > +++ b/src/lib/libssl/src/crypto/dsa/dsa_ossl.c > @@ -83,46 +83,6 @@ static DSA_METHOD openssl_dsa_meth = { > .finish = dsa_finish > }; > > -/* > - * These macro wrappers replace attempts to use the dsa_mod_exp() and > - * bn_mod_exp() handlers in the DSA_METHOD structure. We avoid the problem of > - * having a the macro work as an expression by bundling an "err_instr". So; > - * > - * if (!dsa->meth->bn_mod_exp(dsa, r,dsa->g,&k,dsa->p,ctx, > - * dsa->method_mont_p)) goto err; > - * > - * can be replaced by; > - * > - * DSA_BN_MOD_EXP(goto err, dsa, r, dsa->g, &k, dsa->p, ctx, > - * dsa->method_mont_p); > - */ > - > -#define DSA_MOD_EXP(err_instr,dsa,rr,a1,p1,a2,p2,m,ctx,in_mont) \ > -do { \ > - int _tmp_res53; \ > - if ((dsa)->meth->dsa_mod_exp) \ > - _tmp_res53 = (dsa)->meth->dsa_mod_exp((dsa), (rr), \ > - (a1), (p1), (a2), (p2), (m), (ctx), (in_mont)); \ > - else \ > - _tmp_res53 = BN_mod_exp2_mont((rr), (a1), \ > - (p1), (a2), (p2), (m), (ctx), (in_mont)); \ > - if (!_tmp_res53) \ > - err_instr; \ > -} while(0) > - > -#define DSA_BN_MOD_EXP(err_instr,dsa,r,a,p,m,ctx,m_ctx) \ > -do { \ > - int _tmp_res53; \ > - if ((dsa)->meth->bn_mod_exp) \ > - _tmp_res53 = (dsa)->meth->bn_mod_exp((dsa), (r), \ > - (a), (p), (m), (ctx), (m_ctx)); \ > - else \ > - _tmp_res53 = BN_mod_exp_mont((r), (a), (p), (m), \ > - (ctx), (m_ctx)); \ > - if (!_tmp_res53) \ > - err_instr; \ > -} while(0) > - > const DSA_METHOD * > DSA_OpenSSL(void) > { > @@ -222,7 +182,7 @@ static int > dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) > { > BN_CTX *ctx; > - BIGNUM k, kq, *K, *kinv = NULL, *r = NULL; > + BIGNUM k, *kinv = NULL, *r = NULL; > int ret = 0; > > if (!dsa->p || !dsa->q || !dsa->g) { > @@ -231,7 +191,6 @@ dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, > BIGNUM **rp) > } > > BN_init(&k); > - BN_init(&kq); > > if (ctx_in == NULL) { > if ((ctx = BN_CTX_new()) == NULL) > @@ -248,6 +207,8 @@ dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, > BIGNUM **rp) > goto err; > } while (BN_is_zero(&k)); > > + BN_set_flags(&k, BN_FLG_CONSTTIME); > + > if (dsa->flags & DSA_FLAG_CACHE_MONT_P) { > if (!BN_MONT_CTX_set_locked(&dsa->method_mont_p, > CRYPTO_LOCK_DSA, dsa->p, ctx)) > @@ -256,37 +217,31 @@ dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM > **kinvp, BIGNUM **rp) > > /* Compute r = (g^k mod p) mod q */ > > - if ((dsa->flags & DSA_FLAG_NO_EXP_CONSTTIME) == 0) { > - if (!BN_copy(&kq, &k)) > - goto err; > - > - /* > - * 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.) > - */ > + /* > + * 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.) > + */ > > - if (!BN_add(&kq, &kq, dsa->q)) > + if (!BN_add(&k, &k, dsa->q)) > + goto err; > + if (BN_num_bits(&k) <= BN_num_bits(dsa->q)) { > + if (!BN_add(&k, &k, dsa->q)) > goto err; > - if (BN_num_bits(&kq) <= BN_num_bits(dsa->q)) { > - if (!BN_add(&kq, &kq, dsa->q)) > - goto err; > - } > + } > > - K = &kq; > + if (dsa->meth->bn_mod_exp != NULL) { > + if (!dsa->meth->bn_mod_exp(dsa, r, dsa->g, &k, dsa->p, ctx, > + dsa->method_mont_p)) > + goto err; > } else { > - K = &k; > + if (!BN_mod_exp_mont(r, dsa->g, &k, dsa->p, ctx, > dsa->method_mont_p)) > + goto err; > } > > - if ((dsa->flags & DSA_FLAG_NO_EXP_CONSTTIME) == 0) { > - BN_set_flags(K, BN_FLG_CONSTTIME); > - } > - > - DSA_BN_MOD_EXP(goto err, dsa, r, dsa->g, K, dsa->p, ctx, > - dsa->method_mont_p); > if (!BN_mod(r,r,dsa->q,ctx)) > goto err; > > @@ -308,7 +263,6 @@ err: > if (ctx_in == NULL) > BN_CTX_free(ctx); > BN_clear_free(&k); > - BN_clear_free(&kq); > return ret; > } > > @@ -386,8 +340,16 @@ dsa_do_verify(const unsigned char *dgst, int dgst_len, > DSA_SIG *sig, DSA *dsa) > goto err; > } > > - DSA_MOD_EXP(goto err, dsa, &t1, dsa->g, &u1, dsa->pub_key, &u2, dsa->p, > - ctx, mont); > + if (dsa->meth->dsa_mod_exp != NULL) { > + if (!dsa->meth->dsa_mod_exp(dsa, &t1, dsa->g, &u1, > dsa->pub_key, &u2, > + dsa->p, ctx, mont)) > + goto err; > + } else { > + if (!BN_mod_exp2_mont(&t1, dsa->g, &u1, dsa->pub_key, &u2, > dsa->p, ctx, > + mont)) > + goto err; > + } > + > /* BN_copy(&u1,&t1); */ > /* let u1 = u1 mod q */ > if (!BN_mod(&u1, &t1, dsa->q, ctx)) >
