Recovery signing: add to constant time test, and eliminate non ct operators

[elichai]

Hi,
The recovery module was overlooked in #708 and #710, so this adds it to the valgrind_ctime_test and replaces the secret dependent branching with the cmovs,
I created a new function secp256k1_ecdsa_sign_inner (feel free to bikeshed) which does the logic both for ecdsa_sign and for ecdsa_sign_recoverable, such that next time when things get changed/improved in ecdsa it will affect the recoverable signing too.

[real-or-random]

ACK except nit

[real-or-random]

need rebase

[elichai]

I first fixed the comments from #755 (comment)
so it will be easy to see the diff(https://github.com/bitcoin-core/secp256k1/compare/009a515800ddcb76230a5fd49a78c4ce22fae23b..dcc80cf985dfed5659699eca2f830857b5a42497)

Now I'll rebase

[real-or-random]

ACK 9e6287f I inspected the diff and tested it

[jonasnick]

ACK 2860950

[sipa]

Why? recid is not secret.

[elichai]

The problem isn't recid being secret, it's ret being secret.
(ret includes the validity of seckey so we don't want to branch over it)

[sipa]

I don't think there is much of an issue with declassifying ret. If the API is used correctly, ret is always 1 anyway.

[elichai]

If we assume a user won't call sign_* without verifying the secret key first then yeah we can declassify is_sec_valid (which is what contaminates ret)

secp256k1/src/secp256k1.c

Lines 510 to 513 in a39c2b0

	/* We don't want to declassify is_sec_valid and therefore the range of
	* seckey. As a result is_sec_valid is included in ret only after ret was
	* used as a branching variable. */
	ret &= is_sec_valid;

[real-or-random]

@sipa This is not the scope of this PR. If we want to change this, I think that's a different discussion.

The current signing code on master is overly cautious and assumes is_sec_valid to be a secret. That's exactly the reason why we cmov r and s (introduced by #710), see also the corresponding code comment and the exact reasoning in the PR.

[sipa]

@real-or-random Thanks for the references, that makes sense. My point was mostly that at some point, the complexity of removing decreasingly relevant forms of non-ctness aren't worth it, but this isn't too bad yet.

[real-or-random]

ACK 2860950 read the diff, tested with valgrind including ctime tests