Avoid BN reallocations

[paulidale]

The earlier fix -- #4576 -- to avoid side channels in the DSA code was off by one in terms of the size the big numbers were pre-allocated to. In some cases the allocations needed to be increased by one word and this resulted in a timing attack that could potentially leak information. Increasing the size of the BNs prior to doing anything with them should prevent the leak.

A second variable time fix is included which avoids a variable length big number copy in favour of a constant time swap.

Thanks to Samuel Weiser for finding and reporting this.

[mspncp]

Since it's security related, I assume this should go to 1.1.1. too, right?

[bernd-edlinger]

agreed.

[davidben]

These gymnastics are questionably useful (and perhaps even counterproductive) for the same reason as mentioned in #5001 (comment)

For instance, observe that #7487 is correct, but this logic of adjusting the size of k actually worsens the inversion timing leak because BN_mod_exp_mont calls a (variable-time) division function on unreduced inputs:

openssl/crypto/bn/bn_exp.c

Lines 353 to 357 in f81b043

	if (a->neg || BN_ucmp(a, m) >= 0) {
	if (!BN_nnmod(val[0], a, m, ctx))
	goto err;
	aa = val[0];
	} else

This is analogous to how the ECDSA version of this previously tripped this logic:

openssl/crypto/ec/ecp_nistp256.c

Lines 2122 to 2129 in f81b043

	/*
	* this is an unusual input, and we don't guarantee
	* constant-timeness
	*/
	if (!BN_nnmod(tmp_scalar, p_scalar, group->order, ctx)) {
	ECerr(EC_F_EC_GFP_NISTP256_POINTS_MUL, ERR_R_BN_LIB);
	goto err;
	}

The real fix for all of this length-leaking BIGNUM mess is things along the line of @dot-asm's recent work allowing non-minimal BIGNUMs in limited situations, or #6640 for the complete fix. With the complete fix, setting up a DSA signing operation becomes much more straightforward:
https://boringssl.googlesource.com/boringssl/+/53d9fdd548fc6cefd76fc685f0c6d185dff4b069/crypto/dsa/dsa.c#886

[paulidale]

I agree that there is (a lot) of work to be done to the big number implementations and that it is something that we need to do. However, the question for me, at the moment, is if the changes here are better than what was there before. I can't see how they might be worse.

[davidben]

Fair enough, but you may wish to leave a comment about the efficacy of this mechanism at the least. The perturbation itself also still isn't constant-time due to BN_add and the overall bn_correct_top problem.

(Whether there is any point in this perturbation is... questionable. You may wish to just remove it at this point? The perturbation itself introduces a minor timing leak in itself, introduces another in the inversion, but potentially avoids one in the exponentiation, though one that is already made less likely after 39eeb64. After that commit, the only cases where this helps at all are the cases where everything unavoidably leaks due to bn_correct_top. That means we're now talk about magnitude of leaks and likelihood of exploitability, rather than the much more robust "constant-time" computing discipline that is usually expected for a crypto library.)

[paulidale]

I've added a comment referring to here.

[paulidale]

Merged to master, 1.1.1 and 1.1.0.
1.0.2 needed to be done manually see #7513