Signed wallet messages can be forged

[AaronFeickert]

The wallet permits signing of arbitrary messages, and verification of message signatures. These functions are wrappers for Schnorr signature functionality in the tari-crypto crate.

However, these signatures can be trivially forged, such that an attacker can generate valid signatures for any public key on messages of its choice. The challenge generated for the signature instance binds only to the message, and not to the public key or public nonce. As a result, an attacker in possession of only the public key and any message can sample the response s value at random, and then simply solve the verification equation for the public nonce.

The correct approach for Schnorr challenge generation is to bind to the public key, public nonce, and message. The new hashing API should be used for this purpose, as it takes additional steps to ensure this binding is done correctly.

[CjS77]

Should we change the signing method in Tari crypto to bind the public nonce regardless?
We could also bind the pubkey, but that doesn't strictly follow the schnorr sig as defined in Wikpedia.

This would remove a bunch of footguns where people don't realise that the challenge is expected to already include whatever you're binding to it.

And the way to implement this is to add an additional method, so as not to break existing clients, and deprecate the current method (which would throw a warning when compiling code using the current method)

[AaronFeickert]

Do you mean keep the same API (where the caller provides a challenge) but then modify the challenge in some way to additionally bind to the transcript data? I don't see a real advantage to that versus abstracting away the challenge entirely, if use cases permit.

Binding the public key is important to avoid another type of forgery, where the attacker is able to forge messages on keys that it cannot control directly. That is, any particular forgery would then yield a verifying public key that the attacker can't choose in advance.

[AaronFeickert]

After discussion, looks like the strategy will be twofold.

First up will be to update the wallet sign and verify functions to properly handling transcripting by:

• using the hashing API for domain separation and proper input handling
• binding the public key into the transcript
• binding the public nonce into the transcript

This will break any existing signatures, but that's a good thing! Any existing signatures could have been trivially forged and should be rejected by the verifier; this will happen automatically because of the hash function properties.

Next up will be to add a new API for safer handling of signatures, where the caller has no control over the robotic innards. This will add a new tari-crypto signature struct. The signer will take a private key and message, and produce a signature (handling things like nonce generation and the transcript challenge internally). The verifier will take a public key, message, and signature, and determine the validity of the signature. It may be useful to allow flexibility in the transcript domain separator, since such signatures may be used for multiple use cases where we need proper separation.

[sdbondi]

Sorely needed, there are at least 2 cases where I've abstracted challenge construction w.r.t the nonce and public key for specific cases, just for reference.

https://github.com/tari-project/tari/blob/development/comms/dht/src/message_signature.rs
https://github.com/tari-project/tari/blob/development/comms/core/src/peer_manager/identity_signature.rs

The only case we have where it is valid to construct a potentially invalid signature is when constructing a signature from a network message for verification purposes. This is currently a <public nonce, sig scalar> pair constructed as:

let signature = Signature::new(msg.R, msg.s);

Ideally, we would encode and decode the signature as one binary blob, but this would break any message with a signature and be a larger refactor.

let signature = Signature2_0::from_bytes(&msg.signature).unwrap();
signature.verify(challenge)

I think we want to avoid new(public_nonce, sig) in signature 2.0 and make that refactor part of migrating away from the previous version.

[AaronFeickert]

The new API being developed in this PR from tari-crypto should be used once complete, since the wallet sign_message and verify_message_signature functions are only used by FFI callers, which themselves internally handle nonce operations. However, we should ensure that custom domain separation is used to prevent malicious context switching of signatures; this is not yet implemented in the tari-crypto PR.

[stringhandler]

Removed the offending code in #4891 but will leave this open until the pr is merged in tari-crypto

[AaronFeickert]

You'll still intend to create new versions of these functions that wrap the tari-crypto signature API when it's completed, right? Will PR 4891 then be updated with those new functions?

[stringhandler]

Can I close this?

[stringhandler]

The offending methods have been removed from the wallet. All of the current places in the wallet that sign transactions are aggregate signatures of the kernel, so the nonce is pre-determined and needs to use sign_raw. Any other place that uses this method will have a warning of deprecated. New functionality using sign should use the new signing API

[AaronFeickert]

Can I close this?

Sounds good. I suppose the remaining task is just a chore to migrate existing sign calls to sign_raw to remove the warning, which is also a good opportunity to confirm that each use case is properly applying strong Fiat-Shamir.