TalismanSociety / talisman

Multi-Chain Made Easy with Talisman Wallet. An ultra-secure Ethereum and Polkadot wallet for both beginners and pros.
https://talisman.xyz/download
GNU General Public License v3.0
56 stars 35 forks source link

Support signing raw "signer payload" bytes? #924

Closed jsdw closed 2 weeks ago

jsdw commented 1 year ago

Talisman currently seems to support signing:

interface SignerPayloadJSON {
    /** The ss-58 encoded address */
    address: string;
    /** The checkpoint hash of the block, in hex */
    blockHash: string;
    /** The checkpoint block number, in hex */
    blockNumber: string;
    /** The era for this transaction, in hex */
    era: string;
    /** The genesis hash of the chain, in hex */
    genesisHash: string;
    /** The encoded method (with arguments) in hex */
    method: string;
    /** The nonce for this transaction, in hex */
    nonce: string;
    /** The current spec version for the runtime */
    specVersion: string;
    /** The tip for this transaction, in hex */
    tip: string;
    /** The current transaction version for the runtime */
    transactionVersion: string;
    /** The applicable signed extensions for this runtime */
    signedExtensions: string[];
    /** The version of the extrinsic we are dealing with */
    version: number;
}

Libraries like Subxt and CAPI are able to generate "signer payloads" (ie call_data + extra_bytes + additional_bytes, black2_256 hashed if longer than 256 bytes) that can be signed natively. It'd be super helpful if wallets like Talisman also supported being passed and signing these same bytes.

I wonder whether this has been deliberately avoided though (hence wrapping arbitrary bytes in <bytes>.. before signing them)? now that metadata exists to decode and display such bytes, I wonder whether there's any scope to support it again?

jsdw commented 1 year ago

(I think the bug label was automatically added; apologies if not; this is not a bug but a feature request)

0xKheops commented 1 year ago

Hey James,

Thanks for bringing this up, we'll have a look !

jsdw commented 1 year ago

Just to provide some more context for my request:

Subxt can be used to construct, sign and submit transactions in Rust. It can also compile to WASM and be used to make Rust based browser apps.

Subxt can produce "signer payloads" (ie the call_data + extra_bytes + additional_bytes bytes) to be signed. These payloads are what Substrate's native signing logic supports signing, and so we can hand them to that logic to be signed, or use Subxt's native signing support to sign them.

I had hoped that it'd also be possible to take this payload and have a wallet like Talisman sign it, too, but instead we've struggled to produce the format that Talisman requires (that SignerPayloadJSON), and of course have to bear a lot of extra complexity taking our payload-thats-ready-to-be-signed and breaking it up into that JSON, presumably only to have it be reconstructed within Talisman to be signed.

I hope that helps, and thankyou for having a look!

chidg commented 1 year ago

Hi @jsdw, thanks for the request Currently Talisman inherits its signing logic directly from the polkadot.js extension. So, if polkadot.js can sign something, we should be able to too, and if we can't but they can, we would consider it a bug. Talisman doesn't strictly require SignerPayloadJSON, you can make a signing request with SignerPayloadRaw, but as you noticed, the polkadot libraries we use will wrap arbitrary bytes in <Bytes>. This is explained in this PR from the polkadot.js extension repo. We could potentially expose a method which allowed truly arbitrary bytes to be signed, but we're unclear what the use case at this stage would be apart from development purposes, and we'd have to contend with the security hole opened up. Most likely we will wait until something like this is supported more broadly at the ecosystem level and in the polkadot libraries.

jsdw commented 1 year ago

I guess the original security concern existed because there was a lack of metadata that was able to decode the bytes given and show the user something more sensible about what they were signing.

I suppose my hope is that when things like https://github.com/paritytech/polkadot-sdk/issues/291 exist, it would be possible to pass a metadata "proof" + signer payload to some extension/signing device, and the extension/device would be able to use the metadata to decode and present full details of the thing to users, while ensuring that the node would not accept it if the metadata provided was actually a lie.

A long term advantage of accepting bytes + metadata is that there would be much more flexibility in what can be signed; currently the SignerPayloadJSON is a little hard coded to certain chains (it knows about "tip" and mortality info, but not about any other signed extensions that other chains might create for instance).

But anyway, perhaps that'll happen down the road a bit; food for thought though maybe :)

Most likely we will wait until something like this is supported more broadly at the ecosystem level and in the polkadot libraries.

Makes sense! I think a broader push to this new model will be needed once it's available!

Tbaut commented 1 year ago

I guess the original security concern existed because there was a lack of metadata that was able to decode the bytes given and show the user something more sensible about what they were signing.

No there was metadata all along. The concern is that when you ask users to sign raw bytes, it's expected to be some ascii messages. The signing part won't decode it. Now the security concern that led to this is to prevent being able to ask for raw signing of actual extrinsics:

where malicious apps can get transactions signed since it is "just bytes"

So the users would think that they're just signing some message, but they end up signing an extrinsic that didn't get decoded. The wrapping prevents the ability to actually craft a valid extrinsic.

jsdw commented 1 year ago

That makes sense; I guess the ideal API would differentiate from "sign raw bytes" (and display those as ASCII, wrapped in <bytes> or whatever, and "sign transaction bytes" which would attempt to decode the payload with the help of metadata to show the user something sensible?

chidg commented 2 weeks ago

Closing as stale/low priority