kukai-wallet / kukai

Web wallet for the Tezos blockchain
Apache License 2.0
180 stars 100 forks source link

Unable to sign a custom payload for layer 2 solutions #400

Closed Gau-thier closed 1 year ago

Gau-thier commented 2 years ago

Describe the bug (current behavior)

When trying to use Kukai-wallet to sign operations for our Layer 2 solution Deku, I face:

Error

An unknown error occured. Please try again or report it to a developer.

I am able to login on the wallet and request Sign and Operations permissions:

const Tezos = new TezosToolkit("https://mainnet.tezos.marigold.dev/");
const myWallet = new BeaconWallet({
    name: "decookies",
    preferredNetwork: NetworkType.MAINNET,
    // we should use NetworkType.CUSTOM, but if we do, we face:
    // Network Error
    // The wallet does not support this network. Please select another one.
});

Tezos.setWalletProvider(myWallet);
dispatch(saveWallet(myWallet));

// later
await wallet.requestPermissions({
    network: {
        type: NetworkType.MAINNET,
        rpcUrl: "https://mainnet.tezos.marigold.dev/"
    }
});

It looks good on Kukai-wallet side:

permissions-kukai permissions-ok-kukai decookies-kukai

Then, later in my dApp, the code used to sign my custom payload:

signature = await signer.client.requestSignPayload({
    signingType: SigningType.RAW,
    payload: stringToHex(fullPayload)
}).then(val => val.signature);

This is working great using AirGap-wallet.

I already open an issue on Temple-Wallet because they drop the first char of the payload to sign.

To reproduce

You can use the beaconSDK official documentation to reproduce the exact same error:

beaconSDK-kukai

Feel free to let me know if any other information is needed. Please note, this bug makes the usage of Kukai-wallet impossible for our Layer 2 solution.

Cheers!

Gauthier

klassare commented 2 years ago

Kukai currently supports signing of Micheline expressions (0x05 prefix) and message signing (noop not support yet). If you need something custom beyond that we are happy to add support for it. Do you have a spec or anything? We would for example insist on domain separation between L1 and L2.

klassare commented 2 years ago

I also want to point out that there's a reason why we only support signing of hex strings prefixed with 0x03 or 0x05. Operations and Micheline expressions are well defined, so we can parse them and make it more human understandable to the user what they are signing. Raw can have an arbitrary meaning and is therefore more dangerous to sign. So it's more of a feature than a bug imo.

Gau-thier commented 2 years ago

Hi @klassare 👋! Thanks, for the answer 😄. I am not sure to fully understand what you mean... We only want Kukai-Wallet to sign a payload, not to forge/create it. Hence, I don't think it is a "feature". From my point of view, it is:

allowing all the operations available within Beacon-SDK with Kukai-wallet

Moreover, user still signs an hex string in the case of RAW payload, so he/she can decodes it and sees the content.

Trusting our dApp looks as safe as trusting Kukai-wallet or any other dApp...

klassare commented 2 years ago

I suggest that you reserve a magic byte that can be prepended to your L2 operations before signing (or do something equivalent). Would make it easier for us to support your use-case.

tomjack commented 2 years ago

@klassare I agree. I intend to create a TZIP documenting the existing watermarks / magic bytes, which could later act as a registry for application-specific watermarks not used by Octez. (Later, perhaps a link to that TZIP could be placed in a comment near the watermark type in Octez...)

I looked into Deku's signatures before, when I noticed they were intending to reuse Tezos keys.

This is the first instance I have seen of an application "going rogue" and doing signatures with Tezos keys without using a known watermark. :smile:

I believe the signed messages for Deku are currently JSON arrays. (That's before the final BLAKE2B, as usual.)

So I have three ideas:

  1. We could reserve [ and { as magic bytes for generic JSON signatures. This would allow Deku to keep doing what they're doing. But... it doesn't really seem to solve the problem... it would be approximately equivalent to:

  2. Deku could switch to using 0x05 + a Micheline encoded JSON string. This would already be supported by Kukai I guess? But... it seems this would start to make Kukai's support for this format look much more questionable! Is it really enough to just show the JSON to the user? I think it depends on the JSON...

  3. Finally, we could pick a watermark specifically for Deku. (I guess this is what you were suggesting @klassare?) Either a magic byte just for Deku (because we're not running out yet,) or maybe a single "application" magic byte followed by some kind of identifier for Deku. This would hopefully make it common knowledge that the signatures are Deku signatures, and it should mean that no reasonable wallet will perform Deku signatures carelessly (i.e. without intending to perform Deku signatures or at least strongly warning the user.)

(A fourth alternative would be to use failing_noop, but I see no reason to do that, and a few reasons not to. Happy to discuss it if someone wants to. It could be made to work if combined with some convention -- perhaps like the 4-byte tag convention apparently invented by the developers of failing_noop, but never publicly documented to my knowledge.)

It's not clear to me whether any of these alternatives completely solves the problem for Kukai here. The closest is maybe # 3? Kukai could detect the Deku watermark and at least show something roughly like "WARNING this is a Deku sidechain operation and I don't know what it means, it might steal any assets you own in Deku, but here you can look at the JSON"?

klassare commented 2 years ago

@tomjack Thank you for elaborating on different ideas. I think #⁠2 is wrong to do since it would be to use a watermark for a purpose it was not intended for. For the same reason, I don’t really like how message signing is done right now as basically a subset of Micheline. But that’s a separate question.

As a wallet we want to easily identify a clear purpose for what is being signed. Just doing it on a very high level like “you’re about to sign a Deku operation” (+ json or hex string) can be acceptable. We can always improve the abstraction/interpretation over time. Therefore, I think #⁠3 is the best option out of the 4.

Another option to consider would be to create domain separation on a key derivation level. Avoiding key reuse would have some security and privacy benefits, but also affect (in a positive way imo) the UX in wallets that supports Deku more directly.

tomjack commented 2 years ago

Another option to consider would be to create domain separation on a key derivation level. Avoiding key reuse would have some security and privacy benefits, but also affect (in a positive way imo) the UX in wallets that supports Deku more directly.

Yes, I was surprised to see that Deku would reuse the tzN base58 prefixes, and more surprised by this issue which represents (indirectly) a desire to reuse the Tezos SLIP-0044 coin code.

But I don't expect to be able to change their minds :smile:

Gau-thier commented 2 years ago

Hi @klassare and @tomjack,

I think I start to understand what you meant. Your goal is to display a warning popup (matching on the used prefix) saying something like:

dAppName is asking you to sign this payload, which will be sent to Deku Layer2

Am I right?

If yes, I guess a global warning for any usage of SigningType.RAW would be a good first step, don't you think? (maybe also print a warning if the Network is CUSTOM?)

And in a second time, we could see directly with BeaconSDK to handle SigningType.DEKU.

What do you think?

tomjack commented 2 years ago

If yes, I guess a global warning for any usage of SigningType.RAW would be a good first step

No, I agree with @klassare that allowing SigningType.RAW with arbitrary payloads is a bad idea. It's only OK if no one really uses it. Why add a feature if no one is supposed to use it?

Perhaps a wallet could have a secret "developer mode" switch which is hard to find and warns you that if you don't know what it means but some dapp asks you to turn it on, they are probably scamming you. Then dangerous behaviors could be hidden under that switch with warnings.

But developers can just use tezos-client so I don't see the point really.

This is working great using AirGap-wallet.

I already open an https://github.com/madfish-solutions/templewallet-extension/issues/736 because they drop the first char of the payload to sign.

That is terrifying. I wonder what they show the user...

Gau-thier commented 2 years ago

Hi there!

@oteku had opened a TZIP-26 proposal which seems to be a good compromise => https://gitlab.com/tezos/tzip/-/merge_requests/195

klassare commented 1 year ago

Support for 0x80 watermark / magic byte added in 12469f5b131b8cbb7f9a047f67c7bb84c38779d2