-
Notifications
You must be signed in to change notification settings - Fork 168
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
Cannot sign raw payloads #997
Comments
What version of Signer do you use? |
Latest version, 5.0.0. What I mean by raw payload is, as I recently found out, the function that the polkadot extension exposes in https://github.com/polkadot-js/extension/blob/master/packages/extension-base/src/background/RequestBytesSign.ts. What it does, is to wrap the provided payload in |
Oh, I see. You are attempting to send a "sign message" payload, but there are non-UTF8 symbols in the payload. "Send message" interface was designed for text messages and the Signer needs a way to visualize it. If you are indeed using this interface to send binary data, we need to figure out how to represent it and make sure that this does not result in blind signing (which is strictly forbidden) and that it does not result with potential attacker being able to sneak a transaction for signing while making the user believe that it is a binary payload. |
I thought that is why the extension wraps the payload in the [EDIT] |
That part is actually similar between transaction and messages. What differs is that messages do not have extensions like nonce and lifetime. So this works in most networks (there are some that do not have extensions, but those are special cases). If what you are trying to sign is not a valid transaction but has binary format indeed, can you encode it somehow uft-friendly? Like base64 of something like that. Users will see encoded transaction and could maybe compare it against something this way. Although I would strongly advise against using payloads that could not be rendered into something human-understandable, as this requires certain trust in a generally trustless system. With messages, this is not forbidden, but users will (hopefully) recognize the danger and I would expect them to have less overall willingness to use the system. Signing non-UTF payloads (with a warning) could be allowed if we just show binary content of payload (hex-encoded?) on failure to decode message as string. In my opinion, this is just weird (sloppy?) - why not include this functionality properly in metadata and make proper transactions? Or at least spell clearly what the thing signed by user means? - but I understand that there are unimaginable uses for the system and we can indeed consider adding this feature. Let me just show this to @kirushik for security review. |
This is what happens in websites I and my team have no control over, for instance Polkassembly. I guess what they ask the extension to sign is not UTF-8 encoded, as otherwise I am understanding it would be parsed by the signer app. |
Is this issue relevant? Does it make sense for me to try to find some spare time a give it a try to fix it? Never browsed the codebase before, so not sure I could even make an estimate. It is really a pity that I can't use the Signer everywhere I would like to, given it's really cool. |
Is it still present? #1014 seems to be related to this (possibly duplicate) |
So I am using the same benchmark as before (i.e., https://kilt.polkassembly.network/). |
As far as I understand, you are correct. This is improper payload formatting, for some reason they did not scale-encode the message properly and/or used an external signer raw payload interface that should not have been used as is for this purpose. |
So the proper way to create challenges that are "QRCodable" is by doing something like |
Right, but maybe without |
Ok, I will try to reach out to the Polkassembly people to investigate the issue. If they can get it fixed, I will close this issue. |
I'm saddened to inform you that reg. polkadot-js/extension#1053
|
It is indeed a big bummer. It is basically rendering the Parity Signer useless, if not for signing transactions. We have seen that more and more generic payload signing is an important feature for logins and similar use cases. Are we entirely sure there is no way around it than simply dropping support altogether? It seems quite limiting to me. |
On contrary, this was removed for fixing as far as I know. It was not supposed to be possible there in current state. I'm pretty sure support will come back once it is implemented properly, it's just that someone must go and implement it. |
Either way, this is just something that should be fixed outside of Signer. I'll keep this issue open to help building pressure on that front. |
I was not able to find a tracking issue for this, and would love to see one including a suggestion on how to proceed further and welcome external contributions 😁 ping @Tbaut |
Yup, someone needs to do it. I was the one bringing the QR signing to the extension, without scale encoding. I'm definitely happy to be the one to bring it back the right way, but I need to understand if there is a right way and how. I opened an issue in the extension repo polkadot-js/extension#1070 |
Ok, looks like we can adopt 2 formats of signing freeform messages in Signer simultaneously.
Distinguishing between the 2 formats is quite simple. We can give them slightly different representation. Most importantly - whole payload should be just signed as is and Signer should only allow limited number of permitted formats. We can keep adding allowed formats later if needed. I feel like there is a more general solution to distinguish a valid transaction from not-transaction-for-all-possible-networks, but given that there is no network identification in this kind of message, this is quite dangerous feature. @kirushik please check if this makes sense. |
Any news reg. this? It'd be great to have it in before the next release. |
Fixed in #1332 |
Nice! When will this be released? |
We expect a release in a month or two. |
What is the first version of the Polkadot Vault which supports this feature? The changelogs don't reveal this information. Using v6.0.2 does not seem to support it 😢 |
Polkadot Vault does support raw payload signing since v6. The problem is in the pjs extension |
Use Talisman @wischli it's implemented there and works well with Vault. |
I tried but none of my QR signer addresses are listed as a linkable options on Polkassembly. Fixed by building the extension locally and running in dev mode. |
Because you need to "connect" on Talisman the accounts with the Dapp. I just did it successfully. |
You are absolutely right. Thanks a lot for taking the time to educate me. Next Decoded or Sub0 event I'll get you a beer 🍺 |
Apparently the signer does not support or it is not fully compatible with raw payload signing.
I have had issues in two different cases, with my wallet-managed account linked to Polkadot extension as external:
page.signer.signRaw()
, which triggers the Polkadot extension, the payload cannot be parsed by the signer, even though both the website and the signer are using the latest chain metadata.I have had zero problems with extrinsic signing so far, so I guess the problem lays somewhere in the raw payload signing. Is this a known issue? If not, I could provide with additional screenshot/context about both errors.
The text was updated successfully, but these errors were encountered: