gui: Check the strength of an encryption password #17950
Conversation
| @@ -6,6 +6,7 @@ | |||
| #define BITCOIN_QT_ASKPASSPHRASEDIALOG_H | |||
|
|
|||
| #include <QDialog> | |||
| #include <QLineEdit> | |||
There was a problem hiding this comment.
Move to .cpp and just forward declare QLineEdit.
There was a problem hiding this comment.
It is required in this file because of QLineEdit is a parameter of updatePasswordStrengthIndicator
There was a problem hiding this comment.
It's a pointer to QLineEdit, the compiler doesn't need the definition so a forward declaration is enough.
There was a problem hiding this comment.
I agree with @promag here, it's already done this way with WalletModel in the same files.
There was a problem hiding this comment.
But the updatePasswordStrengthIndicator needs access to the ui variable. I could give it over a parameter the current solution is more elegant IMO
There was a problem hiding this comment.
Right, so instead of this:
this->updatePasswordStrengthIndicator(ui->passEdit2);just do
// receive password as const QString&
this->updatePasswordStrengthIndicator(ui->passEdit2->text());or
// access ui->passEdit2 directly
this->updatePasswordStrengthIndicator(); and in either cases you should add #include <QLineEdit> in src/qt/askpassphrasedialog.cpp.
|
Concept ACK |
2207a87 to
b771102
Compare
|
Concept ACK |
|
Concept ACK, but I strongly disagree with the chosen policies about passwords. For example, a sufficiently long passphrase of chosen lowercase-only words can be extremely high in entropy, but would be classified as "weak" here. It'd would be highly unfortunate to discourage good practices. |
src/qt/askpassphrasedialog.cpp
Outdated
| { | ||
| std::string pass = lineedit->text().toUtf8().constData(); | ||
| bool mixedcase = (boost::to_upper_copy<std::string>(pass) != pass) && (boost::to_lower_copy<std::string>(pass) != pass); | ||
| bool specialChars = pass.find_first_not_of("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxzy1234567890") != std::string::npos; |
There was a problem hiding this comment.
should we add digit checks too? Seems like right now we only consider lowercase, uppercase, special characters and length.
There was a problem hiding this comment.
Thanks will add this later
Thanks for the feedback. I will re-think the strength measure the next days |
b771102 to
f94c70c
Compare
|
This actual solves #5278 (an issue from 2014). |
f94c70c to
a5fa237
Compare
|
Agreed with @sipa There are dedicated libraries for this purpose, so I'm not sure it can be reasonably simplified such that we want to roll our own crypto here... Maybe we should just link CrackLib? |
Like many C code bases born in the 1990's CrackLib doesn't really live up to today's standards for secure or robust coding. See the "assumed length of password" discussions in the comments of CrackLib's |
|
@practicalswift I don't see what discussions you're referring to (or any discussions at all at the link). In any case, the input is being provided by the user himself, so this isn't a real concern...? |
|
This topic needs discussion. I personally think we should only use a library with a permissive license + a very small one (only one file). I don't think it would be worth maintaining another subtree just for a small feature in the GUI |
|
Dependencies shouldn't be bundled at all, just picked up by configure and linked to when available. But yes, the LGPL impact on gitian builds might need discussion. I wonder if there are any alternatives to CrackLib. |
|
@luke-jr There's a JS library called zxcvbn from Dropbox. It's also MIT |
Sorry for my sloppy wording: I should have said "reasoning about string length in My point is that CrackLib (1993) was written in an era when robustness was not a priority. We can't really blame the authors: the code was written three years before the widely read seminal article on buffer overflows ("Smashing the stack for fun and profit" (1996)), and I guess most input was assumed to be local and thus somewhat trusted back then. So while we can't blame the authors we can avoid depending on C code from this dark era when possible :) And perhaps more importantly and regardless of code quality: what was consider a non-weak password in 1993 is not necessarily non-weak in 2020 :) Meta point: I don't think we should ever tell a use that their password is strong. The absence of proof of weakness do not necessarily imply strongness :) |
You mean that it only looks at the first 1024 characters? tbh, that seems more than sufficient...?
In our case, it is local. And skimming the linked file, I don't see anything alarming either... It's doing sanity checks on string sizes and such just fine...?
It's not like it's been unmaintained since 1993... Last update was in 2019 from the looks of it. |
As far as I can tell the fixes since 1993 have been of the type "plug this implementation flaw" rather than modernising the definition of what constitutes a weak/non-weak password. Let me know if you find any significant counterexample. Aside: The thread Instead of receiving a simple "thank you for taking your time to report this vulnerability and thus making our software more robust" the security researcher receives a reply containing words/phrases such as "clever-clogs" (security researchers?), "nitpicking" (security research?) and "shooting your mouth off to prove how very clever you are" (non-embargoed disclosure?):
Wow! We should be very glad we live in an era where security researchers are given bounties instead of bullying in exchange for vulnerability reports :) |
|
It doesn't look like bullying in context... but we're getting off on a tangent here |
|
On another note: wallet encryption is significantly different from other cases where one has a password. It mainly only protects against people sitting at your PC for a few minutes - not against malware or remote crackers. The adversary is real humans, not automated password-cracking software. With that in mind, it makes more sense to use a simple, easy to remember password, NOT one with high entropy. So this kind of strength meter is giving people a false sense of security at best (they're more likely to think encryption protects against crackers), and helping them lose their wallet at worst (by forgetting their passphrase)... So I think it's a Concept NACK from me. |
In any case, 1993 standards are probably still better than any simple algorithm we throw together here. Also stumbled across libpwquality, which is BSD-licensed and slightly newer (2011?). (None of this overcomes the Concept NACK issues, though) |
|
I will have a look at these libraries but I'm a bit curious about how we should include them.
Anyway I still think that these two feature might be a bit to overkill for a small GUI feature though. |
Exactly. weak concept-ACK, but what exactly constitutes a strong password is a common topic of heated discussion and bound to be controversial. I also do not like the idea of adding a dependency just for this. |
I agree on this point. I plan on re-doing the password strength measure this weekend with more advanced checks. I'm still curios if we should add a dict (using the OS dict file) to check for words in passwords |
|
If we're doing this, a library should be a dependency, but can be an optional feature omitted when not available. It's possible no library exists for the criteria we want, though, since we're outside the norm in that regard. An ideal wallet passphrase probably should use only dictionary words, for example. |
|
Todays IRC meeting discussions on this topic: [20:35:16] <wumpus> #topic the library for #17950 (emilengler)
[20:35:18] <gribble> https://github.com/bitcoin/bitcoin/issues/17950 | gui: Check the strength of an encryption password by emilengler · Pull Request #17950 · bitcoin/bitcoin · GitHub
[20:35:33] <wumpus> I *really* do not like introducing a dependency for this
[20:35:35] <emilengler> thanks
[20:35:42] <emilengler> I agree with wumpus
[20:35:59] <jonasschnelli> Yes. Please no dependency for a gimmick feature
[20:36:05] <wumpus> it's somewhat nice to display a measure of password strength (if people can ever agree on one), but it's not worth large changes to our build process for
[20:36:09] <sipa> i feel that anything that self-written is going to be too ad-hoc to be useful
[20:36:09] <jonasschnelli> It is already handholding...
[20:36:09] <wumpus> exactly
[20:36:24] <sipa> so either it's depending on a well-maintained library, or we don't do it at all
[20:36:24] <jeremyrubin> I think i'd rather just *suggest* a strong password
[20:36:25] <luke-jr> there's conceptual problems in the first place
[20:36:48] <wumpus> maybe it's a bad idea in the first place, thinking of it, we don't want to encourage a specific kind of password scheme, this only reduces security
[20:36:52] <luke-jr> this shouldn't be a "strong" password, it should be a memorable one
[20:36:56] <jeremyrubin> e.g., here are 12 random words
[20:37:04] <wumpus> that, too
[20:37:13] <luke-jr> encrypted wallets won't stop malware, just little brother
[20:37:30] <luke-jr> the risk of losing access outweighs the benefits of a string passphrase here
[20:37:31] <jonasschnelli> Can we just have a short text to help people do the right thing? Or a link (less likely)?
[20:37:32] <wumpus> it's not a brainwallet, not the entire internet can attack it, the security needed depends on how secure the wallet file is kept
[20:37:46] <gwillen> it is very hard to make a password-strength indicator that is not at least sometimes very misleading
[20:37:56] <wumpus> making it too strong might cause people to forgt it
[20:38:01] <MarcoFalke> I think more people have lost coins due to forgetting too strong passwords than first getting their wallet stolen, but not their password, and then got their password cracked offline
[20:38:02] <wumpus> which is much worse
[20:38:11] <jonasschnelli> Indeed
[20:38:11] <sipa> gwillen: zxcvbn seems pretty sophisticated already
[20:38:15] <sipa> *actually
[20:38:16] <wumpus> MarcoFalke: agree
[20:38:30] <sipa> it's very hard because users probably don't have a good intuition for what the requirements are
[20:38:30] <wumpus> what would be nice is a feature that makes people write down their HD seed
[20:38:42] <wumpus> aid recovery, not make it worse
[20:38:45] <jeremyrubin> (I'm actually recovering a wallet for someone who forgot their password, so I agree)
[20:38:45] <MarcoFalke> yeah
[20:39:00] <wumpus> a lot of people lose their coins either by losing their wallet or paspphrase
[20:39:01] <sipa> if the wallet.dat file leaking is an attack vector you want to protect against, the password needs to be *far* stronger than common recommendations for website login passwords
[20:39:02] <jonasschnelli> wumpus: you mean adding BIP39 support?
[20:39:06] <jeremyrubin> * attempting that is, let's hope the fragments are good enough
[20:39:28] <gwillen> sipa: as such things go, it's pretty sophisticated, but it does not know your dog's name, or your mother's maiden name, or your birthday, or your favorite book you're quoting from, or any of a number of stupid things users do that lower effective entropy
[20:39:40] <gwillen> while not lowering apparent entropy relative to the tool's model
[20:39:41] <luke-jr> sipa: but if the wallet.dat file leaks, you probably have a keylogger on your PC anyway, so..
[20:39:45] <wumpus> jonasschnelli: yes I suppose
[20:39:53] <sipa> gwillen: of course it can only give an upper bound
[20:40:00] <gwillen> it's never presented that way, though
[20:40:12] <sipa> anyway
[20:40:25] <sipa> i'm in favor of just not pursuing that feature
[20:40:31] <jeremyrubin> luke-jr: disagree with those priors
[20:40:35] <sipa> it's too hard to do right
[20:40:35] <luke-jr> jeremyrubin: ?
[20:40:49] <jeremyrubin> [11:39] <luke-jr> sipa: but if the wallet.dat file leaks, you probably have a keylogger on your PC anyway, so..
[20:40:49] <MarcoFalke> Yeah, we should recommend users use a shorter password, if anything
[20:40:51] <sipa> luke-jr: wallet.dat files get backed up
[20:40:59] <wumpus> luke-jr: they might copy it to a cloud service or something
[20:41:00] <sipa> MarcoFalke: i disagree with that as well
[20:41:01] <luke-jr> sipa: hopefully encrypted!
[20:41:04] <jeremyrubin> I think it's relatively likely you leak your file but don't get a keylogger
[20:41:13] <sipa> luke-jr: right, but in that case, the passphrase needs to be strong
[20:41:24] <luke-jr> sipa: no, I mean encrpying the file itself
[20:41:26] <jeremyrubin> e.g., keeping backups on thumb drives
[20:41:36] timothy (~tredaelli@redhat/timothy) left IRC (Remote host closed the connection)
[20:42:01] <sipa> luke-jr: it's hard to assume that people will use a strong password for an encrypted backup, but then not one inside the file?
[20:42:15] <luke-jr> perhaps we should put a suggestion to that effect somewhere
[20:42:16] <wumpus> in any case, there's no disagreement about whether the wallet encryption is useful or not, that's not the topic
[20:42:17] <sipa> i disagree that in general we should advise weak passwords
[20:42:34] <wumpus> no, we shouldn't advise that
[20:42:46] <MarcoFalke> ok, we shouldn't advise on weak passwords, but we might want to explain the tradeoffs
[20:42:53] <sipa> MarcoFalke: yes
[20:42:59] <wumpus> that would make sense, yes
[20:43:02] <wumpus> add an explanation
[20:43:03] <MarcoFalke> I.e. what the password protects against and what it does not protect against
[20:43:07] <sipa> "Losing this password will make your funds irrecoverably lost"
[20:43:28] <jeremyrubin> I think also saying "writing down the password in a notebook is probably better than not having one"
[20:43:36] <jonatack> https://www.xkcd.com/936/
[20:43:37] <jeremyrubin> Or something to that effect
[20:43:37] <jonasschnelli> I'm all for informing (text based) rather then applying rules that only works for certain use cases
[20:43:48] <luke-jr> jeremyrubin: it really depends on your risk exposure
[20:43:53] <jeremyrubin> I think people don't know that the password is not a seed
[20:44:14] <jeremyrubin> if you just write down the password but they don't have the wallet.dat it's fine
[20:44:45] <wumpus> jeremyrubin: yep, some people are confused by that
[20:44:45] <jeremyrubin> luke-jr: if someone remote compromises your computer but you have a sticky note with a long password on your screen you're "fine"
[20:45:06] <wumpus> because most wallets work with seeds nowadays
[20:45:07] <jeremyrubin> (until you type it in, but let's assume read only compromise)
[20:45:09] <jonasschnelli> The wallet encryption should be better explained. I would not wonder if some users encrypt their watch only wallets in the hope to not leak metadata to computer wide text pattern searches, etc.
[20:45:21] <wumpus> agree with jonasschnelli on adding explanation text
[20:45:37] <luke-jr> you can't encrypt watch-only I thought?
[20:45:44] <emilengler> I think it may be a good way to add a way to encrypt the wallet in the intro dialog
[20:45:44] <jonasschnelli> Can't you?
[20:45:46] <hebasto> explanation is good
[20:46:05] <wumpus> only private keys are encrypted, so encrypting watch-only would be a no-op
[20:46:07] <luke-jr> jonasschnelli: what would it even do?
[20:46:07] <sipa> jonasschnelli: of course not
[20:46:10] <jonasschnelli> luke-jr: I guess you can because its mostly a mixed situation
[20:46:13] <sipa> what would there be to encrypt?
[20:46:24] <jonasschnelli> sipa: thats exactly the problem
[20:46:31] <jonasschnelli> people expect things are encrypted
[20:46:40] <jonasschnelli> while we only encrypt the keys
[20:46:44] <wumpus> the metadata is never encrypted
[20:46:52] <jonasschnelli> Yes. But we don't tell that to users
[20:47:01] <sipa> jonasschnelli: a no-key wallet can't be encrypted, i think?
[20:47:04] <luke-jr> it's obvious?
[20:47:11] <jonasschnelli> So,.. IRS grabs wallet.dat file and reads transaction comments
[20:47:16] <wumpus> this is why software needs documentation I guess
[20:47:22] <sipa> luke-jr: don't assume things are obvious
[20:47:22] <jeremyrubin> luke-jr: how is it obvious?
[20:47:31] <jeremyrubin> That they can open the wallet and see stuff and only pw to sign?
[20:47:34] <jonasschnelli> It's not obvious to most users
[20:47:37] <luke-jr> you open Bitcoin Core and see your metadata, without entry of passphrase
[20:47:51] <wumpus> jonasschnelli: well it doesn't ask th password at startup, only when you send
[20:47:51] <jonasschnelli> encryption means for most users data can't be read by someone no knowing the secret
[20:47:53] <jeremyrubin> Actually that sounds like a 2 birds one stone thing
[20:48:08] <jonasschnelli> wumpus: sure. But novice users don't understand that either
[20:48:15] <jeremyrubin> If people have to put their password in more often maybe they're less likely to forget it ;)
[20:48:23] <luke-jr> jeremyrubin: hmm!
[20:48:59] <jonasschnelli> I think adding more explanations on how the encryption work would be great in general
[20:49:04] <jonasschnelli> works
[20:49:21] <MarcoFalke> Opened an issue #18085
[20:49:22] <gribble> https://github.com/bitcoin/bitcoin/issues/18085 | doc: Explain what the wallet password does · Issue #18085 · bitcoin/bitcoin · GitHub
[20:49:27] <jonasschnelli> Nice
[20:49:37] <wumpus> thanks
[20:49:47] <jeremyrubin> I think there's also a lot of room for improvements in what users have available, e.g. shamir's secret shares
[20:49:52] <jonasschnelli> We don't encrypt the wallet, we encrypt the keys
[20:50:29] <jonatack> I sense new options/config args in our future here
[20:50:32] <jeremyrubin> Even though we know multisig is better, user's are really struggling to do anything better than a plaintext wallet
[20:51:12] <luke-jr> not sure it makes sense to put any effort into pre-Taproot multisig at this point?
[20:51:16] <sipa> jeremyrubin: that's not really an option in a model where a wallet is a file and not a seed
[20:51:17] <wumpus> we should be careful only to add features that are actually used and useful though, don't want to end up with some GPG-like tool that does a zillion things but with a lot of pitfalls
[20:51:30] <jeremyrubin> Maybe organizing some discussion at coredev.tech would be good about conducting some user research to improve things.
[20:51:52] <kanzure> i'll add that to the list then.
[20:51:57] <kanzure> empirical user testing would be interesting
[20:52:00] <sipa> luke-jr: multisig support at this point means improving PSBT integration, I think |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
If this is followed-up on, I'm going to suggest re-opening in the new gui repo: https://github.com/bitcoin-core/gui. I'm in agreement with others that adding a new dependency for this doesn't seem like a great idea. As was also alluded too above, this is the kind of change that is open for plenty of bike-shedding in regards to password strength etc, so ideally we wouldn't be spending lots of cycles discussing it here. |
Fixes: #5278
Video Demo
This PR checks the strength of a password when encrypting or changing the passphrase of a wallet. If someone has some suggestions in terms of password security let me so I can add them to the different levels of strength