Wrap all raw signed bytes inside <Bytes>

[jacogr]

This closes a huge gaping hole where malicious apps can get transactions signed since it is "just bytes". Always perform wrapping of the raw data when the raw signer is used before performing the signing.

cc @Tbaut (Hopefully this doesn't break the work on your QR integrations)
cc @Swader (Not sure if you use this somewhere, but in-case since I believe you have asked)
cc @niklabh (I believe that Polkasemmbly uses signing in the initial flow, if not, ignore)
cc @joelamouche (I'm assuming your team is referred to below, unaware of your specific usage details)
cc @xlc (I heard a rumor you use signing, have not checked as of yet)

[athei]

This is nice. I have a few questions, though:

• Doesn't this break the currently running native crowd loan campaigns which rely on on a verifier hash?
• Can we have some UI changes that show when something is wrapped so that we can confidently assume that this is no transaction?

[jacogr]

Teams will need to make the required changes to their apps if they use this interface.

From this point forward (which means the 0.39 release) everything in the raw bytes signing interface will be wrapped. (Built-in accounts and the new QR functionality, Ledger does not allow raw signing at all - since it is a risk, e.g. the same risk we try and remove here)

[Tbaut]

Thanks, I will make sure to check it out before the end of the day and adapt Stylo in case it's needed.

[niklabh]

Thanks checking

[Tbaut]

Just checked the code briefly, and I understand that any message will now be wrapped with <Bytes>{message}</Bytes> and this means the signer will sign this additional data.

The best path forward for anyone using the signing functionality:

• Wrap the message themselves (otherwise it will be done by the extension and the signature won't match)
• For an offline signer eventually remove the <Bytes></Bytes> before displaying a message

pinging @Slesarew for QR signer in rust you're working on.

[ntduan]

So do I need to change it to verify the signature as it is now?
Before:
const msg = ‘message'
signatureVerify(msg, signature, address)
Now:
const msg = <Bytes>message</Bytes>
signatureVerify(msg, signature,address)

[jacogr]

@ntduan Indeed. A different form (assuming this is merged and the package available) via https://github.com/polkadot-js/extension/pull/743/files#diff-60639b27a793d918f655ef618cd1778eb219812f1200d48079560408ac4ae3f0R12 -

import { wrapBytes } from '@polkadot/extension-dapp';

const msg = wrapBytes(‘message');

signatureVerify(msg, signature, address)

(On the signing as well, as @Tbaut suggested it may be easier just start using to wrap anyway - since there won't be any double-wrapping, i.e. there is no delay then either way)

That assumes you have no signatures stored. In your case you do, so you would need to check both variants... which is not great, but not difficult.

Could actually release a patch version (i.e. extension won't yet hit the stores), so the utils are already available and usable on @polkadot/extension-dapp (it saves some work)

[jacogr]

Merging. Will release a patch of the libs tomorrow so (if needed) the wrappers can be used. (After merge, it will obviousl already be available in the beta versions)

Will look at late next week for a extension release if no crisis pops up...

[niklabh]

Wrapped all signMessages with <Bytes> on polkassembly as well paritytech/polkassembly#1153

[Tbaut]

Stylo was not really affected, worst case it would have shown the "Bytes".. which is not a big deal. It's fixed in the latest version.
Karura with their crowdloan page definitely need to cater for this (unless they win the first auction in which case this page will certainly be useless)

[joelamouche]

Ok so let me try to reformulate to make sure I understand:

• this is only relevant when trying to sign raw bytes (NOT for an extrinsic for example)
• This wrapper is required around the bytes and prevents a dapp from having a user sign an extrinsic without their knowledge
• Typically the use-case for this is user authentication

Correct?

[jacogr]

@joelamouche 100% - so if you use the raw signing (where just a message is passed), it needs a change. So uses I've seen in the last 2 days -

• auth for users
• getting users to accept T&C's
• I understand it is used for the Moonriver signature-for-verification (at least seen a screenshot where that seems to be the case)

Any transaction signing is 100% unaffected - assuming it is not passed to the signer raw signing interface.

TL;DR Only the signRaw function on the signer interface is affected

[polkadot-js-bot]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.