crypto and x/auth: Add extra checks on signatures/pubkeys + check the signature first in antehandler

[facundomedica]

These are proposed by @0x0ece, thank you!

Check signature before anything else in antehandler

See: https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/ante/ante.go#L39-L48

Check pubkey integrity

Suggested checks:

• if pubkey length is ok (without panicking would be nice). We currently do this but we throw a panic.
• if pubkey is valid by checking if it corresponds to a point in the curve

Fuzz testing txs

In my exploration, by simply tweaking some bytes in the tx one can reach many places in the cosmos-sdk source code, due to the current structure of the ante-handlers.

---

Let's discuss these suggestions and eventually plan for their implementation.

[alexanderbez]

Errrr what's the problem and what's the proposal?

[facundomedica]

So it's not a problem but rather some observations made by the reporter/OP. So for example if you make a transaction with an invalid pubkey you would get errors from other steps first (like gas for example). So for me it makes sense that we check for the validity of everything related to the signature before anything else; you are also avoiding querying on-chain stuff for a tx that was going to be impossible to accept.
On the second and third points I don't have a strong opinion. Checking if the pubkey corresponds to a point in the elliptical curve sounds nice, but I'm not sure if that could bring any other issues

[alexanderbez]

So for example if you make a transaction with an invalid pubkey you would get errors from other steps first (like gas for example).

Why is this the case? You mean if you get other errors first? I'm still not understanding why signature verification should be moved up the ante-handler order stack. To me, it makes sense moving the more expensive checks further up the stack.

[minxylynx]

So kind of related, I have noticed that if a tx fails on signature re: sequence, it will still get added to the chain state (as in, it produces a tx hash). I dont know if that's been addressed, but if yall are doing signature validation, can you rework that as well?

[alexanderbez]

So kind of related, I have noticed that if a tx fails on signature re: sequence, it will still get added to the chain state (as in, it produces a tx hash). I dont know if that's been addressed, but if yall are doing signature validation, can you rework that as well?

That isn't part of signature verification. I'm still not sure how that happens as ReCheck on the Tendermint node should remove any tx that is no longer valid.

In any case, with ABCI++ (in 0.47), an app can explicitly ensure txs with invalid sequences will be rejected. I.e. Osmosis will be doing this.

[bizk]

Does checking if the public key belongs to a derivation curve adds value worth implementing?

[IdaTucker]

So for example if you make a transaction with an invalid pubkey you would get errors from other steps first (like gas for example).

Why is this the case? You mean if you get other errors first? I'm still not understanding why signature verification should be moved up the ante-handler order stack. To me, it makes sense moving the more expensive checks further up the stack.

Hi @alexanderbez and @facundomedica, adding a check for the pubkey length would not be an expensive check. Checking that the public key belongs to the curve is more expensive, but not unduly so. And I can't see that it would bring any other issues.
Should we go for just implementing the pubkey length check? Or do you think it is worth implementing both?

[robert-zaremba]

checking if a point is on a curve is also cheap. It's usually 2 exponentiations and addition and modulo.

[IdaTucker]

Exactly! Sorry my wording was poorly chosen, I should have said "slightly more expensive but still cheap".

Also re-reading Aleksandr's message I realize he was referring to verifying the signature, not the public key checks.

I would also be inclined to move the signature checks first, so that no unauthenticated party can get information on the system (e.g. leaked by previous errors), or cannot try to exploit the code (occurring before signature verification), as they would be rejected early on.
I don't think I'm the best placed to comment on the impact on the system's efficiency that such a change would entail.

[alexanderbez]

Makes sense, thanks @IdaTucker!