In ecdsa_verify_digest() allow the digest to be equal to the order of the group

[andrewkozlik]

Based on a bug report from Guido Vranken:

Here's a new discrepancy I found.

operation name: ECDSA_Verify
ecc curve: secp256k1
public key X: 55066263022277343669578718895168534326250603453777594175500187360389116729240
public key Y: 32670510020758816978083085130507043184471273380659243275938904335757337482424
in: {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe,
0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, 0x41, 0x41} (32 bytes)
signature R: 72687201794607335770376625339678533083457947201644627590368835286197510267364
signature S: 8109447218838624529250472094270127202499713651044141199238310867220054199890

This fails verification in Trezor, but passes verification in libsecp256k1, Botan, wolfCrypt and the Decred secp256k1 implementation.

The reason for this is that in is congruent to 0 modulo the order of the group. So it's equivalent to the case in = 0000...00 we had earlier. The other libraries are behaving correctly and Trezor should also accept the signature as valid, since I think the original argument for rejecting no longer applies in this case.

[prusnak]

LGTM. Let's wait until the test go green in the CI and for Ondrej's review.

[guidovranken]

Just did a quick test and I believe verification still fails but I have more time to test this later today.

[real-or-random]

It's correct that libsecp256k1 allows the group order as a digest. But libsecp256k1 also allows the all-zero digest, so I think this should be allowed here, too.

[onvej-sl]

The reasons why we decided to forbid all-zero digest are:

• The probability that you find a message with all-zero digest is infinitesimal (it's even harder than finding the discrete logarithm of a public key), so we don't hurt anyone.
• We want to thwart some kind of bug or fault injection attack that changes a hash (to be verified) to all-zero hash, because
• the signature of all-zero digest can be easily forged (for example (r, s) = (Q.x mod n, Q.x mod n) is a valid signature of all-zero digest, where Q is a public key and n is the order of the curve).

[real-or-random]

The reasons why we decided to forbid all-zero digest are:

* The probability that you find a message with all-zero digest is infinitesimal (it's even harder than finding the discrete logarithm of a public key), so we don't hurt anyone.

* We want to thwart some kind of bug or fault injection attack that changes a hash (to be verified) to all-zero hash, because

* the signature of all-zero digest can be easily forged (for example `(r, s) = (Q.x mod n, Q.x mod n)` is a valid signature of all-zero digest, where `Q` is a public key and `n` is the order of the curve).

Ok, but then I believe this PR should be rejected entirely because the same is true for the digest that is encodes the group order.

[onvej-sl]

the same is true for the digest that is encodes the group order.

I don't think so, because it's easier (or more likely) to "inject" zero bytes than any pattern with both zeros and ones.

[andrewkozlik]

Let me just summarize and elaborate a bit on what has already been written by @onvej-sl:
ECDSA, like most other signature schemes, is vulnerable to existential forgery with respect to the digest. So if an attacker can force the digest to come out to any value of their choice, then we are screwed no matter how many digests we disallow. What's special about the all-zero digest is a combination of two things:

1. ECDSA is vulnerable to a selective forgery attack for digests that are congruent to 0 modulo the group order.
2. An attacker being able to force the digest to come out all-zero is significantly more realistic than him being able to force the digest to come out to any other value of his choice. Buffers are often zero-initialized either intentionally by calling memset or memzero or automatically zero-initialized by the compiler in case of arrays with static storage duration. The attacker then only needs to find a way to prevent the digest from being written into the buffer, which might be achievable by fault injection or bugs such as an unhandled error code.

[guidovranken]

I've tested again, I've verified that the patch is working as intended.