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

⚠️ ☠️ WARNING: Risk of losing funds, please do not use this project ☠️ ⚠️ #724

Closed
blockchainunchained opened this issue Feb 13, 2021 · 24 comments · Fixed by #791 or #793

Comments

@blockchainunchained
Copy link

blockchainunchained commented Feb 13, 2021

Hello,

Sorry to put pressure on like this but I feel if I do not somebody is going to lose funds. Parity have a bad reputation for this already and we cannot afford to take the risk of making it worse. I want Polkadot to succeed. It doesn't succeed if nobody can store DOT safely. Currently this app is recommended on the Wiki, the Website and elsewhere and yet it is not secure?

It is currently impossible to know what you are signing when you sign stuff with this app. It does not display any info about the transaction on the offline device. This means your security is only as good as the online device being used to sign transactions. It also means users are at extreme risk of using a compromised polkadot.js app and losing all their funds.

This has happened before, recently, in Ethereum land where one of the founders of a De-Fi project was tricked into signing a transaction that sent all his funds to an attacker. This is not just a theoretical issue. It is a very viable attack vector.

The development team agree this is an issue and that nobody should be using this app until it is resolved.

So where are the warning messages? Where are the workarounds? Why are we ignoring this issue and hoping that a fix will be deployed (no schedule) before someone gets hacked? That is reckless in the extreme.

Please understand as a developer myself I respect what you guys do but I also respect those that use my code. In this case it feels like the needs of the users are not being properly respected and we need to fix that.

Thanks a lot

@blockchainunchained
Copy link
Author

blockchainunchained commented Feb 13, 2021

Potential work arounds (not tested, please be careful):

  1. Use two computers (one airgapped and one online) running Polkadot.js and your Parity Signer offline phone.
  • Install a fresh operating system (verify checksums) on a computer (a raspberry Pi would work fine).
  • Update the OS, install Chrome. (or use the desktop app)
  • Use the instructions at Polkadot.js app Github repo to get a local instance running (e.g. via docker).
  • Disconnect the machine from the internet
  • In future you will generate any transactions you want to sign on this offline machine which will remain offline.
  • If you need to update chain definitions / Polkadot.js you will have to repeat these steps from scratch.
  • Scan a QR code of a transaction from the offline machine using Parity Signer
  • Scan the Parity Signer QR code of the signed transaction on the online machine (OS verification not important).

The above prevents a malicious person interfering with the transaction but it does nothing against a bug in Polkadot.js causing it to ask us to sign something weird and us not noticing because the purpose of the transaction isn't explained.

  1. We could verify using an airgapped computer so it can't broadcast the transaction before we've had the chance to check it.
  • Install a fresh operating system (verify checksums) on a computer (a raspberry Pi would work fine).
  • Install https://gitlab.com/polychainlabs/polkadot-tx-parser which lets us see what a transaction does
  • Generate the transaction on any machine with Polkadot.js on it.
  • Sign the transaction using Parity Signer
  • Scan this transaction into the airgapped machine, verify it does what you want it to do.
  • Scan the transaction from Parity Signer back onto the live machine to broadcast the transaction.
  1. As above but checking the unsigned transaction instead of signing it first. You wouldn't need an airgapped machine neccesarily although it might still be a good idea to prevent anyone tampering with your transaction verifying environment.

Update

  1. Follow the instructions here instead: https://blog.polychainlabs.com/polkadot/2021/02/09/polychains-approach-to-polkadot-account-security.html

@Tbaut
Copy link
Contributor

Tbaut commented Feb 13, 2021

These instructions for 1. won't work because Polkadot-js apps will only work if it's connected to a node. Also you can't craft a transaction without it, unless it's an immortal transaction, which is not recommended.

The 2. might work but not being knowledgeable about tx-parser.. I can't tell. It is important to note that if the metadata are not up to date, decoding can fail. This is precisely the reason why the details card got removed. Signer couldn't keep up with the runtime upgrades. This is a problem that is very tricky to tackle offline due to QR restrictions, and the size of Metadata. This is logged here: #318

I don't think there is a good workaround. The PR are already there and as I mentioned earlier in a comment #680 (comment) this will be addressed in the next days.

@kianenigma
Copy link

I agree with the concerns and find them as sensible. Although, I would still say that each tool has its own trade-off, and it is worth telling Signer's position as well.

Signer does not need to be updated frequently, but cannot decode transactions in return. To the best of my knowledge, Ledger decodes transactions, but you might need to wait a week or two for your update to arrive per runtime upgrade.

Having said that, I can see people preferring either, and both tools having their own use cases.

Lastly, I believe you can set an on-chain time delayed proxy and have your signer account be the proxy. Then you can check the queued call in multiple explorers (not sure how well they decode and show it -- should be possible easily) and be almost certainly sure about what you have just singed.

@blockchainunchained
Copy link
Author

blockchainunchained commented Feb 15, 2021

Surely it is better to need to update Signer more regularly and have it actually be capable of decoding transactions? We can do updates securely via the App Store (admittedly its a bit of a faff having to delete the keys). Alternatively more work can be done on reading sequences of QR codes if necessary (or high data density QR codes) which allows us to update infrequently but still understand what we are signing.

Does Ledger display what the transaction is doing on the device itself? I've not used it yet but if it doesn't clearly show (which will be hard on a tiny screen) what you are signing then it is also of extremely limited utility. You are only as secure as your online computer that's communicating the Ledger without this check.

"Having said that, I can see people preferring either, and both tools having their own use cases."

I'd argue both tools are deficient and we are letting users down.

In your last example with the time delay proxy can you cancel the queued call before it executes? Surely it would be better to know what you are signing before you sign it? Although I appreciate potential work arounds.

@blockchainunchained blockchainunchained changed the title WARNING: Risk of losing funds, please do not use this project ⚠️ WARNING: Risk of losing funds, please do not use this project Feb 16, 2021
@blockchainunchained blockchainunchained changed the title ⚠️ WARNING: Risk of losing funds, please do not use this project ::warning: WARNING: Risk of losing funds, please do not use this project Feb 16, 2021
@blockchainunchained blockchainunchained changed the title ::warning: WARNING: Risk of losing funds, please do not use this project ⚠️ WARNING: Risk of losing funds, please do not use this project Feb 16, 2021
@blockchainunchained blockchainunchained changed the title ⚠️ WARNING: Risk of losing funds, please do not use this project ⚠️ WARNING: Risk of losing funds, please do not use this project ☠️ Feb 16, 2021
@blockchainunchained blockchainunchained changed the title ⚠️ WARNING: Risk of losing funds, please do not use this project ☠️ ⚠️ ☠️ WARNING: Risk of losing funds, please do not use this project ☠️ ⚠️ Feb 16, 2021
@maciejhirsz
Copy link
Contributor

@blockchainunchained this is being worked on. It will take some time (days), we don't want to rush a security feature without testing it first.

Surely it is better to need to update Signer more regularly and have it actually be capable of decoding transactions? We can do updates securely via the App Store (admittedly its a bit of a faff having to delete the keys). Alternatively more work can be done on reading sequences of QR codes if necessary (or high data density QR codes) which allows us to update infrequently but still understand what we are signing.

Signer should be used offline (in airplane mode) to be secure, which means not being able to do App Store updates. Updating metadata with QR codes requires multiple QR codes currently and it's a bit unwieldy, and it also requires a trusted source from which you are getting those QRs (e.g. metadata would have to be signed by parity using a public key hardcoded into the app). These are all solvable problems, but they are not easy, and certainly not something we can hack over a knee in an hour.

Does Ledger display what the transaction is doing on the device itself? I've not used it yet but if it doesn't clearly show (which will be hard on a tiny screen) what you are signing then it is also of extremely limited utility. You are only as secure as your online computer that's communicating the Ledger without this check.

The hack that happened that you are refering to in the OP happened by a targeted tx emitted by a compromised MetaMask, which was signed by a Ledger.

@blockchainunchained
Copy link
Author

It will take some time (days), we don't want to rush a security feature without testing it first.

I couldn't agree more. In the interim however why is the warning I created the only warning a user will see? Are we just hoping nobody stumbles into this landmine before we defuse it? I'm not suggesting we rush diffusing the landmine but I am saying we could put a sign or a fence up so someone doesn't step on it.

Signer should be used offline (in airplane mode) to be secure

Absolutely, but if you look on this repo there are instructions for how to update Signer. Either those instructions are wrong and causing a security issue in and of themselves or you are forgetting that I suggested (as the guide did) to delete the keys before reconnecting the device to the internet. Which is it?

https://github.com/paritytech/parity-signer/blob/master/docs/wiki/Security-And-Privacy.md#how-to-update-parity-signer-securely

Updating metadata with QR codes requires multiple QR codes currently and it's a bit unwieldy

As is accidentally signing something you didn't mean to, if we can't solve this problem the wallet isn't fit for purpose.

These are all solvable problems, but they are not easy, and certainly not something we can hack over a knee in an hour.

Agreed, in which case I feel it is important to warn users of these deficiencies. My issue isn't that these issues exists it's that users are being encouraged to use this application (by Parity website, Polkadot wiki etc) without being correctly informed of its deficiencies. It doesn't matter if you have an unsafe car on your driveway. It does matter if you let someone drive it without telling them. It matters even more if you encourage them to do so.

The hack that happened that you are refering to in the OP happened by a targeted tx emitted by a compromised MetaMask, which was signed by a Ledger.

Yes, the user should have checked the address on the hardware wallet's screen. He didn't because it was a great big jumble of encoded contract data that was non-human readable. You can of course see how this issue is different but very very similar. We don't even display the data to the user before asking them to blindly sign it. This is the exact same class of issue.

We owe it to the users of this tool to at least warn them of the risks and explain any work arounds. Before an issue happens and without being delayed for an indeterminate length of time waiting for a fix.

What is the advantage to not informing users?

@blockchainunchained
Copy link
Author

Also its a bit unfair to say I'm suggesting we pull a fix from our posterior in a matter of days considering I reported this issue a full 6 months ago and nothing has been done about it.

#680

@maciejhirsz
Copy link
Contributor

@blockchainunchained I'm not saying you are suggesting that, I'm saying it's being worked on. We should have a working balance tx inspection soon, but I also want to set expectations for the proper fix coming later.

Nobody is disagreeing with you that this is a problem.

@blockchainunchained
Copy link
Author

Then why have we not warned users? Why is the website and the wiki still encouraging them to use this tool? Why is there not a warning in big letters on both the release page and the wiki?

I was told 6 months ago this is being worked on and for 6 months people's security has been put at risk from a total lack of any and all official warnings. How is that acceptable?

I'm not sure you saw my question:

What is the advantage to not informing users? Why are we choosing not to?

@maciejhirsz
Copy link
Contributor

maciejhirsz commented Feb 16, 2021

@blockchainunchained opened an issue on the wiki, if you have suggestions where the warnings should be added please comment there.

@blockchainunchained
Copy link
Author

blockchainunchained commented Feb 16, 2021

That issue is closed but the warning is crap. It explains that Parity Signer can't view transactions but it doesn't explain why this is a problem. It tells users to be careful but it doesn't tell them how to be careful. Is it better than nothing? Sure. Could we do much better? You bet your ass. Imagine being a user that lost funds to this issue. Would you feel like the team did everything they could to avoid you being a victim? Or would you feel the team cared more about how they looked than ensuring people were safe?

@blockchainunchained
Copy link
Author

blockchainunchained commented Feb 16, 2021

How about this:

Please do not use Parity Signer right now, there is no way to use it securely as you cannot check what you are signing. If you blind sign transactions using Parity Signer you are entirely reliant on the security of the network connected computer that generated the transaction. This class of attack has been actively exploited before, this is not a theoretical threat

@burdges
Copy link

burdges commented Feb 28, 2021

I'm curious, what currently goes into the QR codes Parity signer scans? Are they merely "sign this hash"? Or does parity signer hash the actual call itself?

If they latter, then one very short term solution could be displaying the QR code contents using another application, even just some python script running on another laptop.

If not, then another very short term solution would be providing the call data and then uses running the call data through some native code that produces the QR codes.

I know these sound pretty ghetto but if anyone wants an immediate stopgap then they'd suffice.

@maciejhirsz
Copy link
Contributor

I'm curious, what currently goes into the QR codes Parity signer scans? Are they merely "sign this hash"? Or does parity signer hash the actual call itself?

Signer hashes the call itself, the decoding logic is already in the signer (for balance transfers at least). The UI isn't final yet, and there is the future-proofing the app issue mentioned above.

@japisd
Copy link

japisd commented Aug 14, 2021

Why is this still open?

@blockchainunchained
Copy link
Author

This issue is not fixed, you should not be blind signing stuff but that is exactly what you have to do with Parity Signer right now. Someone else started work on a tool called Stylo which is a fork of this repository. That lets you see what you are signing before you boldly go off and sign it.

@Slesarew
Copy link
Contributor

Why is this still open?

Because we didn't have a release since this was posted. It's long ago fixed in staging but testing is not done. There is also an ongoing 3rd party UX audit ongoing to close all similar concerns.

@m00seym00
Copy link

is it safe to use Parity Signer as long term holds? I have no plans to sell anything for awhile.

@blockchainunchained
Copy link
Author

There shouldn't be any problems generating an account and using it to store funds. The problem is if you want to do anything with those funds. You are unable to verify what the hot computer is telling the cold signer to sign. Imagine someone giving you a cheque and not being able to read the recipient or amount boxes but still signing the signature.

This will be solved with an upcoming pull release that @Slesarew has done some incredible work on. I know the others are getting crabby with him for making such a large pull request as it is difficult to review but equally I didn't see anyone else stepping up to resolve this problem. Huge respect to them for doing so.

@Slesarew
Copy link
Contributor

is it safe to use Parity Signer as long term holds? I have no plans to sell anything for awhile.

Remember, that any blind-signed transaction could include proxy delegation command - even empty account signing anything blindly might be considered compromised unless you track down what exactly was signed on blockchain; other than that, holding with old signer is fine; however, I would recommend storing keys (as seed phrase and derivations) on paper or something equivalent as ultimately the best long-term method, in some cases bested only by things like banana split. You will need Signer only to create keys, export public ones to hot wallet to accept funds, and later to perform operations on these funds; in the meantime, keeping backups (which should be normal practice anyway!) is sufficient to hold. Also, you will have to restore from paper backup when we release new version and for every subsequent upgrade.

@m00seym00
Copy link

Thank you. Guess I'll try Stilo while it all gets sorted out.

@Slesarew
Copy link
Contributor

Thank you. Guess I'll try Stilo while it all gets sorted out.

I would suggest still using the older Signer over Stylo for storage accounts if you do not sign anything - it currently has much better test coverage and allows derivations; creating derived accounts is better practice than keeping lots of seed phrases.

@Tbaut
Copy link
Contributor

Tbaut commented Sep 20, 2021

and allows derivations

That's inaccurate. Stylo actually has support for derivations for a while now. It also has many additional features compared to current version of Signer, such as being able to change an account's network or sign arbitrary messages.

@rogerpocz
Copy link

I'm with problem, can't sign transaction with parity signer through qr code, show a error, there some way to fix this??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants