Deterministic signatures

[sipa]

This implements deterministic (RFC6979-based) signatures using libsecp256k1's signing. Note that the test cases here were constructed using an earlier version of this patch (which implemented them on top of OpenSSL), so the deterministism has been consistent.

Earlier version of this message:
The OpenSSL-based deterministic signing implementation is likely very temporary, as we may switch to libsecp256k1 for signing soon (#5220), but it allows testing. The signatures generated by the OpenSSL and libsecp256k1 based implementations should be identical, so the test cases are already useful.

[sipa]

Also, here is a sweet canary: bcdf879e47de3b3d93d111d4cada5b59961f51c6879a01ad53df98ae76144b7c

[theuni]

For 0.10, why not take both and verify them against eachother?

[gmaxwell]

@theuni that would defeat one of the advantages of libsecp2561k: its signing is effectively constant time, openssl currently leaks secret data over cache/timing sidechannels like a sieve.

Verification tests the signature in any case (and doesn't touch secret data).

[sipa]

Note that this implementation is certainly worse in terms of timing leaks than OpenSSL's own (which takes some steps to prevent those, at least in their current master branch). If we'd want to merge this without libsecp256k1-based signing, it could be improved though.

EDIT: no longer relevant, as this is now using libsecp256k1's signing.

[sipa]

Rebased on top of #5256, as it's otherwise hard to merge with #5220.

[sipa]

By the way, the unit tests here were generated using @ciphrex's CoinCore implementation of RFC6979 with HMAC-SHA256 (https://github.com/ciphrex/CoinVault/blob/5da653249680169822afa3aecd0bcffb2ddf4961/deps/CoinCore/tests/secp256k1/src/secp256k1_rfc6979_test.cpp).

[gmaxwell]

The iter argument is kinda ugly, since thats really not a good way to stir the nonce. I'd find it preferable to xor it into the message input into the PRNG. Of course this is pointless nitpicking since it's only a testing shim... I did wtf at seeing a nonce += iter in that inner loop.

WRT timing, I think the "takes some steps" is only true in openssl pre-release code. :(

In any case, ACK. (needs preimage for merge)

[sipa]

@gmaxwell I initially wrote it so that the iter argument just skipped the first N outputs of the PRNG. Turns out that that makes the unit tests take many minutes, as some script generations require a few hundred attempts (which becomes squared...).

Mixing it into the message is fine to me.

[gmaxwell]

If you do make that change, perhaps also rename the argument to be test_case or something? I'm concerned that someone immitating this code will think that iter is related to nonce retries for out of range nonces. (Though I suppose they may have more problems, like leaving the secret key out.)

[sipa]

I'm not updating the code now, people may already be reviewing it. As you say, it's only for testing (and the comment in header even says so explicitly).

[sipa]

Rebased on top of #5220 and #5313.

[sipa]

And renamed the argument to test_case as suggested by @gmaxwell.

[Diapolo]

New format would result in BITCOIN_CRYPTO_HMAC_SHA256_H I guess you should look at the oder headers and end comments, too :). I just will give this single nit, nice, isn't it ^^?

[sipa]

Added a commit to make @Diapolo happy.

[theuni]

Isn't it possible that these will be optimized away by the compiler? Is it worth worrying about?

[sipa]

Yup, that's possible. OpenSSL has OPENSSL_cleanse() for that, which we could use too (but I would prefer crypto to not depend on OpenSSL...).

Ideally, I think we use belt-and-suspenders and let every layer do as much as it can to avoid this sort of leakage (which can't be 100% avoided anyway, unless you only ever let assembly code touch the confidential data).

[gmaxwell]

To add some color there... we could (and perhaps should-- I'd be a fan of it) have some extern-ed memsetting function for these cases which we can be sure won't get optimized away.

However, thats not sufficient: The compiler can randomly stash data from variables into random spots on the stack, the compiler can and commonly will leave data sitting around in registers from where other functions may push them onto the stack (e.g. consider how PUSHA works on x86) and will never zeroize them.

[sipa]

Rebased.

[sipa]

Updated the PR description.

[laanwj]

I was looking at 36fa4a7 and it looks like it is not entirely move-only:

@@ -131,8 +123,8 @@ void Transform(uint64_t* s, const unsigned char* chunk)
     Round(f, g, h, a, b, c, d, e, 0x431d67c49c100d4cull, w11 += sigma1(w9) + w4 + sigma0(w12));
     Round(e, f, g, h, a, b, c, d, 0x4cc5d4becb3e42b6ull, w12 += sigma1(w10) + w5 + sigma0(w13));
     Round(d, e, f, g, h, a, b, c, 0x597f299cfc657e2aull, w13 += sigma1(w11) + w6 + sigma0(w14));
-    Round(c, d, e, f, g, h, a, b, 0x5fcb6fab3ad6faecull, w14 + sigma1(w12) + w7 + sigma0(w15));
-    Round(b, c, d, e, f, g, h, a, 0x6c44198c4a475817ull, w15 + sigma1(w13) + w8 + sigma0(w0));
+    Round(c, d, e, f, g, h, a, b, 0x5fcb6fab3ad6faecull, w14 += sigma1(w12) + w7 + sigma0(w15));
+    Round(b, c, d, e, f, g, h, a, 0x6c44198c4a475817ull, w15 += sigma1(w13) + w8 + sigma0(w0));

     s[0] += a;
     s[1] += b;

Doesn't make a difference in practice as wX are local variables that are not used afterwards.

[laanwj]

Better to move this before secp256k1_ecdsa_sign below, with the nSigLen = 72 line. Otherwise it could theoretically happened that the buffer has been made smaller and receives 72 bytes in a next iteration of the loop.

[sipa]

Fixed (in a different way).

[apoelstra]

This is maybe not worth the hassle, but it's possible to do this with only a single CSHA256. In Finalize you'd put the output of inner into hash, then reset inner, write the outer key to it, write hash to it, then output to hash. This way you don't have any non-final HMAC state hanging around in memory after finalization.

[sipa]

I prefer keeping an extra SHA256 state in memory than keeping the key in memory.

[apoelstra]

Oh, of course. I somehow missed that you avoided storing the key.

[apoelstra]

ACK, verified code correctness against my Rust code and the RFC, checked for secret data being left in memory, but did not think about timing since I don't know the first thing about that. Also ran make check.

[gmaxwell]

ACK

[SergioDemianLerner]

Does OpenSSL implements RFC6979?
I propose that the test cases include the code that implements deterministic signatures over OpenSSL (which Sipa already coded) and compares the results with the test vectors. I cannot validate the correctness of the test vectors without creating such a tool myself. Alternatively, the tool to generate the test vectors from OpenSSL should be part of the source code, stored somewhere.
After that I check those test vectors, I ACK this commit.

[sipa]

AFAIK, OpenSSL does not do RFC6979. It just has a means of mixing in the private key and message into the RNG (in addition to the normal randomness) used for generating the nonce.

[SergioDemianLerner]

I would prefer not to iterate though all the words of the nonce unnecessarily, even if nothing is changed. It reduces the possibility that part of the nonce is kept in the stack or in registers.

I propose adding:
if (test_case)
nonce += test_case;

[sipa]

@SergioDemianLerner I created the unit tests using an independent implementation (@CodeShark's CoinVault/CoinCore).

[SergioDemianLerner]

Ok, so I'll use it.

[gmaxwell]

@SergioDemianLerner What pieter said, OpenSSL git (not a release version; release versions are all stright up random) implements a non-standard DSA RNG hardening scheme. The most important thing is that the hash depend on at least the message and a secret, which OpenSSL achieves. Satisifying 6979 is an extra perk.

I believe our test vectors were actually generated with the Ciphrex software rather than the implemention for Bitcoin core.

[sipa]

Well if anything, I agree that RFC6979 seems total overkill. One HMAC should have sufficed. But sticking to accepted standards is nice.

Regarding specific concerns: I don't believe the SHA256 code we have should result in variable-time code on any supported platform. EM or power comsumption... I don't believe there is any reasonable way to avoid those.

[gmaxwell]

Additions are constant time up to my ability to measure on the platforms we support. If they are not constant time, then all bets are off for timing sidechannel immunity anywhere in the signature system. That hashing the key may have some sidechannel leak is very small concern compared to any place else there where there could be (or would be) a timing sidechannel. (Multiplies are also constant time on the platforms we currently support, though I believe (some) PPC has non-constant time multiplies).

They are almost certantly not constant power (in general without special differential logic it's impossible to be especially strong against power sidechannels), but again, the concern in the hash is very small compared to the potential for leaks elsewhere in the signature process, which likely have much stronger power signatures. (unfortunately the only scopes and spectrum analizer I have only go to 100MHz, which isn't really sutiable for exploring power sidechannels on modern hardware :) )

[gmaxwell]

But sticking to accepted standards is nice.

Not just nice, but producing a common reproducable output means that you can do diverse signing with multiple signers to guard against nonce covert channel backdooring.

It also provides some assurance against things like insiders intentionally making the nonce H(message) in order to leak keys.

[SergioDemianLerner]

Sorry about deleting my post. I didn't want to waste your time. But since you've responded, I leave it here for consistency of the conversation.

I don't like RFC6979. I know I'm being terrible audacious by this assertion. But I don't like the way the key is mapped into the SHA-256 key schedule.
The value V (32 bytes long) ends up at the beginning of the compression block, so that the key ends up being combined with mostly fixed bytes with sparse ones. E.g. w[16], w[17] ... could be susceptible to timing attacks if the addition operation has variable time depending on the number of carries. Also electromagnetic and power consumption side-channels could be used to detect those carries.
Again, this is pure speculation, but I think the key should have been laid at the beginning of the compression block, and not after a fixed known mostly-zero pattern

[SergioDemianLerner]

Still, if I could change RFC6979, I would make K = key at the beginning, and not part of the message, instead of K = zero.

[gmaxwell]

Yea, I would have done that in terms of a preitter construction. I suspect the authors of 6979 would have done that too, but I believe what they wanted to do was to take a pre-existing, standardized, construction for a CSPRNG and turn it into a standard for derandomized DSA without any change that might have introduced an application specific weakness.

(No biggie on the response. Messages cross in flight on github all the time. Sometimes both parties will just remove their responses and move on. :) )