Anyone reading this should avoid the "Check for breaches based on password" menu item until this flaw has been fully fixed!

It would also be wise to wait until the security of this addon has been verified by multiple outside sources.

to Andrew or whoever works on this issue

I'm not sure what the best way to proceed patching this is, but we might need to get with Troy from haveibeenpwned.com and ask him to use a better hashing algorithm (Argon?). However, unless you collaborate on the salt (or in this case I guess it would be a pepper), I don't see a secure way to do this. Perhaps the better way is to take the SHA1 hashes (yes, that gigantic file) and download them, then hash and compare locally.

Anyway, lots more thinking needs to go into this, and checking it over with the folks at https://security.stackexchange.com would be pretty helpful as well.

andrew-schofield commented 6 years ago

Yes, the only truly secure way is to configure the plugin in such a way as to download the 11GB+ password files locally.

Short of adding an explicit warning if you try and use this functionality I'm not sure much can be done given the API available.

andrew-schofield commented 6 years ago

Also I'd welcome anyone to audit the code involved. Password data is used in this checker (obviously) and to calculate the password-last-modified-date in the other checkers.

Narvey commented 6 years ago

A warning is the bare minimum, and probably isn't sufficient, but let's start there, go ahead and add the warning.

The thing is, Troy says himself that you should never transmit passwords that are actually in use over the internet on his passwords lookup page. So while I'm not sure what the purpose of that page is (after you have changed your password, look it up there to see how many breaches it was in?), I can say for sure that the way this addon uses it is not the intended scope or usage.

andrew-schofield commented 6 years ago

Yes, I think you're right, I think only acceptably secure solution is to remove the "online" check aspect of this mode, and resort to getting the user to download the SHA1 tables before running the check. Even though the request is encrypted (via TLS in the case of HIBP), and even if I changed the request to include the hash in the payload rather than the URL, it still relies on trusting HIBP explicitly.

Judging by the blog post that accompanies the launch of the password checker, Troy seems to suggests the only legitimate use case for the online version is to check that a password used by an account that has been breached (i.e. you're going to change it anyway), actually appears in a list of breached passwords, reinforcing the idea that if you have re-used that password it should be changed everywhere.

andrew-schofield commented 6 years ago

34 Completely disables this check mode pending a re-write for offline operation. It also adds a note to the README which mentions the problem with a link back to this issue for context.

helluvamatt commented 6 years ago

@andrew-schofield Would you accept a PR for offline reading of the pwn file? If so, I have some questions:

Would we need a configuration mechanism for the location of the pwn file? I haven't looked into how KeePass plugins keep configuration, or if KeePass provides this mechanism (I don't believe it does, I think plugins have to keep their own configs.)
Do we want an "auto-update" function that allows us to fetch the latest pwn file?
Follow up to number 2, do we handle the unzipping (7-zip? Nice.) of the pwn file?

I recently had an intrusion into a credit card online account due to a weak password, and I am looking for more ways to secure my online accounts, including 2FA (that isn't SMS based) and strong passwords.

helluvamatt commented 6 years ago

Update:

I have downloaded the HIBP passwords files. Uncompressed, they are 12.5GB and 320 million lines. It takes 48 seconds just to count the lines using a StreamReader and 17 seconds using wc (via MSYS2).

Progress reporting will be completely useless, as I'm seeing several minutes to scan the files for a single hash. I'm looking at a way to scan the files once and check each line against a collection of hash/entry tuples. (List<Tuple<PwEntry, string>>) but it's going to take a ton of time to do this, especially if the password database is large. It may require a warning message before running with the option to cancel.

Please, let me know what you think. I can commit what I have to my own fork for your enjoyment.

andrew-schofield commented 6 years ago

Feel free to open a PR. Would be better to discuss code solutions via that mechanism anyway.

helluvamatt commented 6 years ago

I have opened #38 to discuss changes. Thanks!

ghost commented 6 years ago

There are some changes now troyhunt.com/ive-just-launched-pwned-passwords-version-2 and you can send partial hashes. Won't this fix the issue?

dotnvo commented 6 years ago

@FredrikLindseth Yep, pretty sure this could be implemented now with the changes to the API, but I'm not an expert.

It looks like the new version uses K-Anonymity. This might be the solution to make this work.

andrew-schofield / keepass2-haveibeenpwned

(weakly-hashed) passwords are sent over the internet #33

Anyone reading this should avoid the "Check for breaches based on password" menu item until this flaw has been fully fixed!

to Andrew or whoever works on this issue

34 Completely disables this check mode pending a re-write for offline operation. It also adds a note to the README which mentions the problem with a link back to this issue for context.