Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support signing raw "signer payload" bytes? #924

Open
jsdw opened this issue Jul 10, 2023 · 7 comments
Open

Support signing raw "signer payload" bytes? #924

jsdw opened this issue Jul 10, 2023 · 7 comments
Labels
enhancement New feature or request

Comments

@jsdw
Copy link

jsdw commented Jul 10, 2023

Talisman currently seems to support signing:

  • Arbitrary bytes (which are internally wrapped in <bytes>...</bytes>).
  • A payload that looks like:
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 jsdw added the bug A bug label Jul 10, 2023
@jsdw
Copy link
Author

jsdw commented Jul 10, 2023

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

@0xKheops
Copy link
Contributor

Hey James,

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

@0xKheops 0xKheops added enhancement New feature or request and removed bug A bug labels Jul 10, 2023
@jsdw
Copy link
Author

jsdw commented Jul 10, 2023

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
Copy link
Contributor

chidg commented Jul 14, 2023

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
Copy link
Author

jsdw commented Jul 14, 2023

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 paritytech/polkadot-sdk#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
Copy link

Tbaut commented Jul 20, 2023

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
Copy link
Author

jsdw commented Jul 20, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants