Fix noir ecdsa verification in acc contract.

[suyash67]

Description

resolves #913

Detailed hackmd: https://hackmd.io/9QRzytElQE2sMLf4S7qR1A?view

TLDR: ECDSA signing in bberg and verification in noir has a minor "difference". Signature construction and verification should operate consistently on the message.

The noir ecdsa verification takes in the hashed message $z$ as an argument. So we need to hash the message before calling std::ecdsa_secp256k1::verify_signature.

The reason this worked with the noble curves package was: in the noble package, the message is never hashed. It is used as is for signing a message. Therefore the noir-ecdsa-verify works as we don't need to hash the message in verification.

Checklist:

• I have reviewed my diff in github, line by line.
• Every change is related to the PR description.
• I have linked this pull request to the issue(s) that it resolves.
• There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
• The branch has been merged or rebased against the head of its merge target.
• I'm happy for the PR to be merged at the reviewer's next convenience.

[spalladino]

Whoa, great find! To make sure I understand: does this mean that secp256k1.sign(msg, privateKey.value); from noble was just signing whatever input it received, versus barretenberg that's both hashing and signing? Is it possible to use bb while still doing a single sha256 in the contract, instead of two?

[suyash67]

Whoa, great find! To make sure I understand: does this mean that secp256k1.sign(msg, privateKey.value); from noble was just signing whatever input it received, versus barretenberg that's both hashing and signing? Is it possible to use bb while still doing a single sha256 in the contract, instead of two?

Yes exactly. Hashing the message in ECDSA signature construction is critical for security and efficiency. That's non-negotiable. What could be done is: instead of defining: message = hash(payload), we can directly set message = payload.

(I've explained in the PR description as well as a detailed hackmd. Hope that helps.)

[spalladino]

What could be done is: instead of defining: message = hash(payload), we can directly set message = payload.

I think that makes sense, so we don't need the extra hash in the contract. Note that this may break the schnorr account contract, since it uses the same base StoredKeyAccountContract class just swapping the signer.

Also, we'll need to make sure we document this properly in noir-lib!

[suyash67]

@kevaundray can you elaborate a bit on why we decided to use verify_signature_prehashed_message_noassert to verify ECDSA signatures in noir vs verify_signature_noassert? This could confuse devs as it did even to our engineers.

[dbanks12]

LGTM

[dbanks12]

Just remove the use ... debug_log; at top of noir file.

[paulmillr]

This issue could have been resolved with 1 loc:

secp256k1.sign(msg, privateKey.value, { prehash: true });

Hashing the message in ECDSA signature construction is critical for security and efficiency. That's non-negotiable.

@suyash67 it's actually negotiable.

1. libsecp256k1, that empowers bitcoin — had unhashed API since inception
2. Also ed25519 / eddsa, in ed25519ph variant
3. You don't know which hash is going to be used for a curve. It's not obvious that sha2_256 is always used for secp256k1: in ETH, it's keccak256 instead
4. Hashed api is terrible in embedded envs with very limited RAM

[spalladino]

Well, it isn't every day that you find the maintainer of a package you're working with that drops in the conversation. Thanks so much @paulmillr for chiming in, I'm now wondering how you stumbled upon this thread.

This issue could have been resolved with 1 loc

Absolutely true, but our goal here was to remove dependencies on third party crypto libraries when possible, and since barretenberg (Aztec's cpp crypto library) covered ecdsa, we wanted to leverage it. noble was a stopgap solution to get an e2e working, and to validate that our own library was working as expected.

@suyash67 it's actually negotiable.

Now this is interesting. Given we'll need to optimise as much as possible in our circuits to reduce user's proving times, maybe it makes sense to keep the verify_signature_prehashed_message version exposed in Noir, just with documentation around it?

[paulmillr]

It's wise to use cpp bindings if your constraints and environments allow it: it's safer and faster. Less deps is also great.

I'm now wondering how you stumbled upon this thread.

I regularly check noble GitHub dependents and dive into projects and their issues.

[suyash67]

Thanks for your inputs @paulmillr and @spalladino! Well I thought it'd be easy to forge signatures if we use the un-hashed signature construction in ECDSA. Particularly, any signature for message $0 \le m \le 2^n - r$ is also valid for message $m' := m + r$. To avoid such a forgery, we'll have to enforce that: $m \in (0, r)$. That said, I think hashing doesn't necessarily guarantee this constraint (unless we ensure that the output of the hash function is truncated to bit-size less than the subgroup field bit-size). I should agree that this is more involved than I thought it was, see Yehuda's answer on crypto stackexchange.