Allow all-zero messages

[sorpaas]

In Ethereum's ecrecover precompile, we need to operate on the raw preimage/message. The message is user input and can be all-zero. However, the current Message struct treats all-zero message as InvalidMessage. This breaks some of our use cases.

cc https://github.com/openethereum/openethereum/issues/11603

[real-or-random]

Uh, the all-zero message is evil, at least when it comes to verification, see https://crypto.stackexchange.com/a/50290 . Can you explain where you need this?

(A more precise statement is "ECDSA without hashing is evil." Unfortunately upstream secp256k1 relies on the caller providing the hash directly. This has the advantage that everything is fixed-size but it's certainly not optimal.)

[apoelstra]

Agreed, concept NACK. But I am curious why you need this?

[sorpaas]

Ethereum has an ecrecover precompile (a function that smart contracts can call on-chain). It accepts parameters message, v, r, s, where message is the raw preimage. All those parameters are user inputs, so currently it's indeed possible to pass all-zero messages to this ecrecover precompile. In OpenEthereum we use rust-secp256k1 as the underlying library for this precompile, and to stay in consensus with other Ethereum nodes, it has to somehow handle the situation when message is all-zero.

After @real-or-random's explanation I do see that all-zero message does not look safe. However, to stay in consensus we currently just have to support that case. We're thinking about (mis-)use the ThirtyTwoByteHash trait to pass raw preimage directly into messages, or I wonder @real-or-random @apoelstra do you have any better ways you think we can handle this?

[apoelstra]

I think that if Ethereum supports flagrantly insecure modes of operation you will need to fork this library to support that. I highly doubt that this specific problem will be the only instance of this sort of thing.

I'm really disappointed to hear people talking about abuse of ThirtyTwoByteHash before it's even merged.

[real-or-random]

I think that if Ethereum supports flagrantly insecure modes of operation you will need to fork this library to support that. I highly doubt that this specific problem will be the only instance of this sort of thing.

To be fair, upstream does the same.
https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/recovery/main_impl.h#L183
That's also true for normal verification. This is intentional:
https://github.com/bitcoin-core/secp256k1/blob/master/src/tests.c#L4790
(Maybe because OpenSSL does the same?)

So strictly speaking we indeed differ from upstream (as used by go-ethereum) here. Which is fine unless someone finds a SHA256 preimage of 000...0. But maybe it's still not optimal?

We all know consensus is hard, that's why our point is that rust-bitcoin shouldn't be used for Bitcoin consensus. (https://github.com/rust-bitcoin/rust-bitcoin#consensus) I don't want to start this discussion. But I still wonder if it's avoidable to have a consensus failure of two implementations that essentially rely on the same library, just one implementation uses a Rust wrapper.

The problem here is that we want to provide an API that's as safe as possible to use. That means we don't want to offer low-level functionality. ecrecover on the other hand is a low-level function, and it's hard to implement a low-level function using a high-level API...

I'm really disappointed to hear people talking about abuse of ThirtyTwoByteHash before it's even merged.

It's merged, #206 extends it only.

[elichai]

Hmmm I do agree that a zeroed message is a bad idea.
BUT if you pass the group order as the message we will not produce an error and this will work, even though it's equivalent.
And as long as upstream supports it (probably because the RFC/SEC of ECDSA supports it) idk what the right answer is.

I definitely agree that a correct API should hash the message for you.

[real-or-random]

BUT if you pass the group order as the message we will not produce an error and this will work, even though it's equivalent.

Oh I haven't thought of this. That's a neat hack indeed.
@sorpaas Does this work for you?

(Should we prevent this? If we assume zeroes is some default value, then a careless user may pass this as a hash but probably not the group order. If we assume the hash is chosen by the attacker, things are different.)

[sorpaas]

@real-or-random Can you provide some more info how to use group order directly in secp256k1 lib? Does that require using secp256k1-sys?

[elichai]

@real-or-random Can you provide some more info how to use group order directly in secp256k1 lib?

let msg = secp256k1::Message::from_slice(&secp256k1::constants::CURVE_ORDER).unwrap();

But I'm somewhat with @apoelstra here, that this is a bug in ethereum.

[stevenroose]

I personally feel double of disallowing things that upstream allows. If the zero message is unsafe, upstream should not allow it. If upstream doesn't, then why would we make an exception?

[apoelstra]

Upstream allows it because in Bitcoin Core's use there is no way to get an all-0s hash (though there almost was, and this would've made coins trivial to steal), and preventing this would greatly complicate the C API.

Unlike upstream, we are working in a reasonable programming language and do not have only one user, whose accidental safety we can rely on when designing APIs.

[real-or-random]

Upstream allows it because in Bitcoin Core's use there is no way to get an all-0s hash (though there almost was, and this would've made coins trivial to steal), and preventing this would greatly complicate the C API.

Unlike upstream, we are working in a reasonable programming language and do not have only one user, whose accidental safety we can rely on when designing APIs.

How would rejecting 0s complicate the C API? I think it could just return 0? I think upstream has more than one user too, that's part of the issue here.

[stevenroose]

Given that a zero message is clearly a special case, how about leaving the from_slice check in place and have a Message::zero constructor? If we really want to discourage this, we can even make it unsafe.

[apoelstra]

@real-or-random we could return 0 upstream, but this is different from preventing the user from creating a 0 Message in the first place (which is what we are trying to do here). The difficulty with C is that we are using unsigned char* as a message type, and adding a new type is very unergonomic.

[fanatid]

While I agree that that zero-messages should not be used I do not think that making fork, periodically sync it with upstream (and maybe publish to cargo) is a good idea. unsafe constructor of default enabled some safe feature looks really good for this.

[apoelstra]

Yeah let's just pull this in. I agree with the moral argument that we shouldn't deviate from upstream. I'm also unsure we're really protecting anybody with this check to be honest. If you have freeform user-provided hashes you are vulnerable to forged signatures anyway (Craig Wright tried this a year or two ago, though I've lost the link), just blocking all-zeros doesn't get you much.