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

Set general requirements and process for including wallets #670

Merged
merged 11 commits into from Dec 15, 2014

Conversation

saivann
Copy link
Contributor

@saivann saivann commented Dec 6, 2014

Preview: https://github.com/bitcoin/bitcoin.org/tree/walletguidelines#wallets

As suggested in #663, I am writing a list of requirements and process for inclusion used thus far. As a matter of fact I have been slowly accumulating this list from developers comments or real issues when testing wallets. Recent wallets were tested with these "standards".

This is in no way a comprehensive list and I still think the first point in the list (sufficient developers and users feedback) is the most important one. In an ideal world bitcoin.org would have someone dedicated to auditing wallets' code and all (which is obviously out of reach for now).

Additionally, this list is meant to change, similarly to scores which evolved with the new unique security models of multisig and hardware wallets. To the best of my knowledge, I've tried to observe very common, simple or non-obtrusive security practices being adopted by all wallets.

Suggestions are welcome if you want to improve this process and involve with the maintenance of this page.

Note: Xapo is the last web wallet to not support HSTS and MultiBit/KnC/Hive for Mac are the last wallets to not rotate change addresses. It may be good to consider these becoming a requirement soon.

@harding
Copy link
Contributor

harding commented Dec 6, 2014

@saivann I like it---thanks! My only suggestion is that we preface the list with something like:

Bitcoin.org is excited to see innovative wallets, so if your wallet has a good reason for not following some of the rules below, please submit it anyway and we'll consider updating the rules.

That way wallet authors know they can discuss an issue with us in case our policy isn't nuanced enough.

@saivann
Copy link
Contributor Author

saivann commented Dec 6, 2014

@harding Very good point, thanks!

@schildbach
Copy link
Contributor

First quick comment: Rather than punishing wallets for "concerning bugs" I'd try to look at how those bugs were communicated, how fast they could be fixed, and what were the learnings. I think the proposed rules incentivize to conceal and deny.

@harding
Copy link
Contributor

harding commented Dec 6, 2014

@saivann I saw @schildbach mention on IRC that "custodial" doesn't translate into German in an obvious way. Even as a native English speaker, I didn't quite get it---I assume it's from the legal definition of custody, as in "Coinbase has custody of your private keys, so they can spend your bitcoins at will." The more common, non-legal definition of custodial means someone who is responsible for cleaning or maintain a building, e.g. a janitor or handyman---and that's where my thoughts went first when I read "custodial". I'm guessing that's where @schildbach's translation went as well.

Maybe you want to define custodial before using it, or maybe (more radically) we shouldn't call them wallets. To me, "wallet money" == "money I control" and "bank money" == "money someone else controls on my behalf". I understand if making that distinction is out of the scope of this pull.

Regarding @schildbach's comment on bugs, I agree. I'd support a simple statement that focuses entirely on issue communication, so that even a super-buggy program could be included as long as it provided highly-visible warnings to users about each specific issue (and maybe an extra warning to generally indicate that the program is alpha/beta-quality).

@saivann
Copy link
Contributor Author

saivann commented Dec 6, 2014

@schildbach Yes that's a good point, any suggestion for updating one of the texts, or for an additional text?

@harding How about:

If user has exclusive access over its private keys
If user has no access over its private keys
If user has no access to some of the private keys in a multi-signature wallet

@harding
Copy link
Contributor

harding commented Dec 6, 2014

@saivann that sounds good to me.

@luke-jr
Copy link
Contributor

luke-jr commented Dec 6, 2014

The first version should reflect rules that existing wallets currently meet, and can be revised with a second PR later.

"For multi-signature wallets: " sounds like it ought to be "For multi-signature custodial wallets: "

For hardware wallets, the "Refuses unsigned firmware upgrades" needs to be changed - this requirement is incompatible with the GPL and user freedoms. I'd also suggest some requirement for a secure RNG if it ever generates its own seed.

"Level 1 - Full nodes" shouldn't be there - wallets should be independent software from nodes.

"Environment To get a good score, the wallet must run from an environment where no apps can be installed." - again, I dislike rewarding things which restrict user freedom.

"... and be compatible with Tor." is ambiguous and faulty: either it means it can take advantage of Tor for anonymity (which is impossible for Bitcoin, so n/a), or that it can be technically run over Tor (which is true of any application, so not particularly relevant).

A lot of references herein to "rotating addresses", but this is not sufficient - wallets should not reuse addresses at all, not even by rotating them.

@saivann
Copy link
Contributor Author

saivann commented Dec 6, 2014

Re "Refuses unsigned firmware upgrades": It is true that a conflict exists between the open-source model and security. I am not opposed to moving it into optional criterias, however thus far it seems that current hardware wallets have all choosen this option.

Re "Secure RNG": Any suggestion for a standard? This cannot be easily tested.

Re "Full nodes": Perhaps we can rename it to "Full validation"?

Re "Restricted environment": I think it should be kept as is - no software or operating system is forced to provide any specific feature. Here having few features (being specialized) is in itself a security feature.

Re "Rotating addresses": I see two options adopted by wallets thus far, rotating change addresses, and rotating the receiving addresses in the UI. I don't know how it could be more specific.. Unless you think we should prefix these with "Avoid address reuse by..."?

@saivann
Copy link
Contributor Author

saivann commented Dec 6, 2014

Regarding tor, I am not sure that I understand your point. Some wallets provide the option of using tor like a proxy, or it can easily be done in the browser, while some others don't. In any case when using tor, you can conceal your IP address. To be honest I wouldn't be against not listing this score if it causes issues; it is the only one that lists an optional feature that must be enabled by the user.

@schildbach
Copy link
Contributor

Re "Refuses unsigned firmware upgrades": I think its only incompatible to the GPLv3, not v2. Also I think Trezor allows unsigned firmware upgrades but it will remove the seed and also there is some safety mechanism I can't remember.

I think its very similar to how Android handles this. If you want to flash a custom firmware, you need to unlock the bootloader which removes all user data. After that, you're free to do what you want (including locking the bootloader again).

@harding
Copy link
Contributor

harding commented Dec 6, 2014

Regarding hardware wallets and restricted environment being incompatible with open source, I feel that's what the "if your wallet has a good reason for not following some of the rules below..." policy statement is for. If a wallet or environment is secure, that's good. If it's also easy to use, that's great. If it's secure and easy to use and also modifiable by expert users, that's fantastic.

@luke-jr
Copy link
Contributor

luke-jr commented Dec 6, 2014

Re "Refuses unsigned firmware upgrades": Even if all current hw wallets have this problem, I don't think we should reward it as a requirement. Such a restriction is compatible with the GPLv3 iff the user has a way to override it, but is always incompatible with the GPLv2 where signing keys are considered source code (GPLv3 specifically exempts this in the one case).

Re "Secure RNG": Any hardware RNG should suffice, I guess. Any pure-software pseudo-RNG should not.

Re "Full nodes": Same thing. Maybe we can score wallets better which can easily be operated with a full node installed locally (eg, ElectruRe "Restricted environment": m would not meet this criteria because it's difficult to setup)?

Re "Restricted environment": We're not forcing OSs to support apps by removing this requirement, just not mandating they NOT support apps...

Re "Rotating addresses": Bitcoin Core, at least, never reuses addresses even in rotation. Maybe we're thinking of the same thing? I see "rotating" as implying a pattern like A, B, C, A, B, C.

Re Tor: What good is there in hiding your IP when you're still revealing the same information to the other nodes?

@luke-jr
Copy link
Contributor

luke-jr commented Dec 6, 2014

Another possible requirement for hw wallets might be to allow some secure export, for backup.

I'd really rather see the word "rotating" go for clarity - maybe a straightforward "Never use the same address twice, except at user explicit request" or something?

BTW, I'd prefer to not be tagged in commit messages. This causes me to get an email for each one any time someone forks the repository... :/

@saivann
Copy link
Contributor Author

saivann commented Dec 6, 2014

I have updated texts to allow unsigned firmware updates if the seed is protected and made it clear that rotating addresses is meant to avoid address reuse.

Re "Secure RNG": I am no expert here, but I remember reading a lot (including from jgarzik) that trully secure RNGs should never rely on hardware RNGs alone.

Re "Full nodes": This is actually not a requirement for any score, this only affects the positioning of the wallet so full nodes get good visibility, both because they provide better trustless validation and privacy, and because they are essential to protecting the network as a whole.

Re "Restricted environment": Like @harding, I think this could be revisited when a hardware wallet actually adopt this approach. Allowing custom code to run on the device have important implications.

Re tor: Well if nodes can know you own a certain set of addresses, it's still harder to identify who you are without having your IP address. Again, I am not against dropping this score altogether.

Re "export seed": This is already covered (If user has exclusive access over its private keys ... Allows backup of the wallet)

@luke-jr noted, I'll stop adding credits in commits.

@btcdrak
Copy link
Contributor

btcdrak commented Dec 7, 2014

Maybe we can have some privacy tags for wallets, where rotating change addresses was a tickbox. This can help users assess the differences between wallets for their taste.

@saivann
Copy link
Contributor Author

saivann commented Dec 7, 2014

@btcdrak Filtering wallets by features could be applied to many features in general, and would probably be a nice layout, I'm concerned that this information wouldn't be kept up to date however. But for the specific point mentioned, as far as I know, all wallets are slowly migrating to rotating change addresses.

@saivann
Copy link
Contributor Author

saivann commented Dec 10, 2014

@schildbach I have just pushed a commit that rewrites and adds some points to better cover some good coding practices and address your comment, does that sound better to you?

@gurnec
Copy link
Contributor

gurnec commented Dec 11, 2014

I have two proposed changes regarding strong passwords I'd like to bring up for discussion.

The first (which I'm in favor of but only weakly so) is dropping the strong password requirement for logins with read-only access. Although a breach of privacy isn't a good thing, I think users should have the option of taking that risk in exchange for a more convenient password if they so choose, e.g.:

Refuses weak passwords (short passwords and/or common passwords) used to secure access to any funds.

The second is offering an alternative to a strong password requirement along these lines:

Provides an aggressive account lock-out feature in response to failed login attempts along with a strict account recovery process.

The intent here is to cover GreenAddress's PIN login method which doesn't require a good password but is nonetheless quite strong IMO (without having to actually describe it in the guidelines). "Strict account recovery" would mean something safer than your typical email-a-recovery-link, and would probably need to be evaluated on a case-by-case basis.

Any thoughts to these?

@schildbach
Copy link
Contributor

@saivann Yes, LGTM. Thanks!

@saivann
Copy link
Contributor Author

saivann commented Dec 11, 2014

@gurnec Thanks! Your suggestion sounds good to me, and also better cover Hive Web. I have included your changes and updated "If private keys are stored online" to "If private keys or encryption keys are stored online".

@gmaxwell
Copy link
Contributor

One thing to clarify is that these requirements will change and strenghten over time as the industry evolves. I don't ever want to be in a situation where collectively we agree Vendor X is doign something harmful and they're arguing its fine because its not on the list. :)

@gmaxwell
Copy link
Contributor

Also, perhaps we should clarify that these are table stakes. A wallet metting all these may be acceptable to be listed, but listing doesn't constutite a recommendation. We're trying to be sensitive to a diversty of use cases and development time-frames, and so the bar may be lower than some would like to have in a perfect world.

@gmaxwell
Copy link
Contributor

I think we should elevate HSTS to mandatory. Is anyone aware of any reason other than some existing parties being non-conformant that we shouldnt? Paypal has been for many years.

@gmaxwell
Copy link
Contributor

Some extra thoughts:

We should also require listed should have a publically stated security response and bug reporting policy. I'm not sure if we want any requirements beyond there being a contact/policy-- there is some argument for requirements: One popular service was demanding invasive personal information like passport numbers just to report security issues. We also need this contact information for ourselves, we should never be in a situation where we're made aware of a problem with a wallet and our only available option is to delist it because we can't contact its creator / service provider. It's already the case that I have no secure (e.g. PGP) method to contact many of the wallet services are found to have a severe steal all the customer's funds vulnerability... though we may not need to go as far as fixing that with the requirements here.

In cases where the users assets are held by the service, we should probably require the service have some statement on how it would hacking, insider theft, etc. E.g. Does it have insurance which covers users assets? To what degree? Is it self insured? Does the insurance actually protect the users? Can it prove that it actually has the users funds and isn't secretly fractional reserve? Maybe this one could go in an upgraded requirement in the future.

@harding
Copy link
Contributor

harding commented Dec 12, 2014

I like all of @gmaxwell's suggestions, although I think the "table stakes" suggestion is already covered by the current scoring system. @luke-jr suggested that the first version of this doc try to reflect the current criteria, and that a second pull be opened for additional criteria. With that in mind, I suggest the following:

  • Add the note suggested by @gmaxwell indicating that the rules may change and be strengthened.
  • Call for ACKs/NACKs on this pull request with a planned merge date in a few days if no NACKs or other significant criticisms are received
  • After merging, open new pull request that will make HSTS and security contact information mandatory (and @ notify any wallet authors who will be delisted after that pull gets merged)
  • Either in that pull request or a separate pull request, discuss requiring or encouraging wallets which hold user assets to provide policy statements on loss/theft and possibly proof of reserves

@saivann
Copy link
Contributor Author

saivann commented Dec 12, 2014

@gmaxwell Thanks, your feedback makes a lot of sense. I pushed two additional changes.

@harding This sounds like a good plan! I was also thinking of using this process for applying other changes in requirements in the future.

I will take care of HSTS and security contact information soon.

Regarding the research on policy statements, I may spend time on it a bit later, if someone is reading this and interested by the task, you are welcome.

@saivann
Copy link
Contributor Author

saivann commented Dec 12, 2014

In the absence of critical feedback, this pull request will be merged on December 15th.

If you think this first version needs changes or has important issues, please leave a comment. Otherwise additional improvements can also be submitted after the initial version is merged.

@sinetek
Copy link

sinetek commented Dec 12, 2014

As an outsider, why is the GPL relevant at all here?

@gmaxwell
Copy link
Contributor

@sinetek because we wouldn't want to impose requirements on wallets which are incompatible with free sofware.

in particular, requiring that devices that would handle your bitcoins to be tamper proof black boxes could have pretty bad negative consequences for user freedom. Really what the drive should be for is tamper evidence. There would be a stronger argument if tamperproofness weren't so much stronger against users with casusal positive uses than it is against attackers.

E.g. say your hardware wallet has a nasty security bug, but the official update forces you to pay a maintance fee in your first transaction to the vendor, or perform some crazy AML process. You should be free to apply an open source security fix; instead of accepting the crazy update or throwing the device out.

There is some tradeoff, because it means you could also apply an insecure update, but an attacker with a soldering gun can very likely do that already with many hardware wallet designs (certantly for trezor.)

I'm not sure how much to weigh that point, but its an interesting consideration at least.

saivann added a commit that referenced this pull request Dec 15, 2014
Set general requirements and process for including wallets
@saivann saivann merged commit 9e6bf6a into master Dec 15, 2014
@saivann saivann deleted the walletguidelines branch December 15, 2014 20:14
@saivann saivann mentioned this pull request Mar 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants