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

Add support for Offline HIBP password checker #2707

Merged
merged 1 commit into from Jun 25, 2019

Conversation

@c4rlo
Copy link
Contributor

c4rlo commented Feb 16, 2019

The new keepassxc-cli hibpanalyze subcommand can check all passwords in the given database against a given list of SHA-1 password hashes (when --hibp is given). Such lists are available from the "Have I Been Pwned" (HIBP) project at https://haveibeenpwned.com/Passwords.

Example usage:

$ curl -Lo hashes.7z https://downloads.pwnedpasswords.com/passwords/pwned-passwords-sha1-ordered-by-hash-v4.7z
$ keepassxc-cli analyze --hibp <(7z x -so hashes.7z) passwords.kdbx
Insert password to unlock passwords.kdbx:
Password for entry 'foo' has been leaked 123 times!
Password for entry 'mygroup > bar' has been leaked 789 times!
Finished.

Type of change

  • New feature (non-breaking change which adds functionality)

Description and Context

Have I Been Pwned is a widely used collection of leaked credentials. It is used by several other password managers.

There are several other variations of the functionality implemented in this PR that may be worth considering:

  • Ability to access the functionality implemented in this PR from the GUI.
  • Ability to access this functionality without having to download the entire password hash database (9.8 GiB compressed currently) first. This is possible using the HIBP pwned passwords API. To use it, one must send the first 20 bits of the hash up to the server, getting back a list of all matching hashes (generally a few hundred). This is a compromise that leaks a small-ish amount of information about the password to the HIBP server, while keeping data transfer volumes low.
  • Ability to check all new or changed passwords against the HIBP API.

Testing strategy

The core logic (implemented in core/HibpOffline.cpp) has unit tests in tests/TestHibp.cpp.

What's still mostly missing is tests for the CLI-specific part.

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
src/cli/Command.cpp Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
src/crypto/CryptoHash.h Outdated Show resolved Hide resolved
tests/TestCli.cpp Outdated Show resolved Hide resolved
@louib

This comment has been minimized.

Copy link
Member

louib commented Feb 16, 2019

@c4rlo thanks for the contribution.

For the record, I opened a PR for that a few months ago, which I decided to not push through completion. Review comments from that PR might still be relevant (in particular the presence of the SHA1 algo in the standard crypto class).

Your PR would also address #1083.

One of the popular features currently requested is an integrated password analyzer, that would display information about duplicate passwords, expired passwords, passwords with low entropy, etc. Considering this, we could rename the command so that it allows us to add other metrics / stats. Something like keepassxc-cli analizer --hibp ?? Here's the password analyzer issue.

@c4rlo

This comment has been minimized.

Copy link
Contributor Author

c4rlo commented Feb 16, 2019

Thanks for the context, @louib -- I should have guessed this came up before :)

I'm happy to move the SHA-1 impl out of CryptoHash if that will help get this accepted.

As for the name of the subcommand, I'd be happy to go with analyze --hibp, especially if we think there will be further analysis modes added in future.

src/cli/Hibp.cpp Outdated Show resolved Hide resolved
src/cli/Hibp.cpp Outdated Show resolved Hide resolved
src/cli/Hibp.cpp Outdated Show resolved Hide resolved
src/cli/Hibp.cpp Outdated Show resolved Hide resolved
src/cli/Hibp.cpp Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
src/crypto/Crypto.cpp Outdated Show resolved Hide resolved
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Feb 16, 2019

Recommend not searching the Recycle Bin for passwords.

The actual implementation of the HIBP interface should not be contained in the CLI code. That should be added to Core. The interface class should have a meaningful name such as "HIBPInterface".

@droidmonkey droidmonkey added this to the v2.4.1 milestone Feb 17, 2019
@c4rlo

This comment has been minimized.

Copy link
Contributor Author

c4rlo commented Feb 23, 2019

I believe I've addressed most comments now; see individual commits. Still left to do:

  • Look into removing class Sha1Hash with its uchar m_data[20] and just using QByteArray instead. I might run some benchmarks first.
  • Improve CLI unit tests (in addition to the lower-level unit tests we now have for the underlying logic).
  • Add documentation.
@c4rlo c4rlo changed the title CLI: add 'hibp' subcommand CLI: add 'analyze' subcommand with HIBP checker Feb 23, 2019
@c4rlo

This comment has been minimized.

Copy link
Contributor Author

c4rlo commented Feb 24, 2019

Completed above items. This PR is ready for another round of review.

Btw TeamCity seems to have some issues...

@louib louib requested a review from phoerious Mar 4, 2019
src/core/HibpOffline.cpp Outdated Show resolved Hide resolved
@droidmonkey droidmonkey modified the milestones: v2.4.1, v2.5.0 Mar 24, 2019
@droidmonkey droidmonkey changed the title CLI: add 'analyze' subcommand with HIBP checker Add support for HIBP password checker Mar 24, 2019
@droidmonkey droidmonkey changed the title Add support for HIBP password checker Add support for Offline HIBP password checker Mar 24, 2019
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 24, 2019

I completely missed that this requires to download the entire 9.8 GB password hash database. I find this to be completely untenable for the average user. I would much rather implement the online version instead of this.

@c4rlo

This comment has been minimized.

Copy link
Contributor Author

c4rlo commented Mar 25, 2019

I definitely think that supporting an online check would be very nice as well -- certainly more convenient in most cases. But the offline check has a few advantages also:

  • Does not require Internet access, so works for people running keepassxc on an airgapped computer.
  • Does not leak any information about the passwords. I don't really want to get into a discussion of whether leaking a few bits of a SHA-1 to HIBP is a problem or not, but some people may certainly feel uncomfortable with it.

As regards having to download a 9.8 GB file, I don't see that as a big problem as it's not something that would have to be done often.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 26, 2019

Ok, I'll do a deep dive on this pr once I'm satisfied with 2.4.1

@t4777sd

This comment has been minimized.

Copy link

t4777sd commented Apr 4, 2019

For me personally I would like to have the option to download the data and run it offline.

This new subcommand checks all passwords in the given database against a given list of SHA-1 password hashes. Such lists are available from the "Have I Been Pwned" project at https://haveibeenpwned.com/Passwords.

Note that this support offline checking only. The HIBP project also provides a web API for checking specific hash ranges; this is not currently supported.
@droidmonkey droidmonkey force-pushed the c4rlo:hibp branch from a7e5d06 to 522a0ce Jun 25, 2019
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 25, 2019

I made the necessary updates to merge this PR. Works great, but the HIBP file is HUGE (24 GB uncompressed) and takes about 5 minutes to crunch through on my laptop.

@droidmonkey droidmonkey merged commit 0e0cba6 into keepassxreboot:develop Jun 25, 2019
4 checks passed
4 checks passed
CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
phoerious added a commit that referenced this pull request Oct 26, 2019
Added

- Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)]
- Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)]
- Add offline user manual accessible via the 'Help' menu [[#3274](#3274)]
- Add support for importing 1Password OpVault files [[#2292](#2292)]
- Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)]
- Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)]
- Add group sorting feature [[#3282](#3282)]
- Add feature to download favicons for all entries at once [[#3169](#3169)]
- Add word case option to passphrase generator [[#3172](#3172)]
- Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)]
- Add UNIX man page for main program [[#3665](#3665)]
- Add 'Monospaced font' option to the notes field [[#3321](#3321)]
- Add support for key files in auto open [[#3504](#3504)]
- Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)]
- Complete usernames based on known usernames from other entries [[#3300](#3300)]
- Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)]
- Allow abbreviation of field names in entry search [[#3440](#3440)]
- Allow setting group icons recursively [[#3273](#3273)]
- Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)]
- Drop to background after copying a password to the clipboard [[#3253](#3253)]
- Add 'Lock databases' entry to tray icon menu [[#2896](#2896)]
- Add option to minimize window after unlocking [[#3439](#3439)]
- Add option to minimize window after opening a URL [[#3302](#3302)]
- Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)]
- Browser: Add initial support for multiple URLs [[#3558](#3558)]
- Browser: Add entry-specific browser integration settings [[#3444](#3444)]
- CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)]
- CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)]
- CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)]
- CLI: Add XML import [[#3572](#3572)]
- CLI: Add CSV export to the 'export' command [[#3278](#3278)]
- CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)]
- CLI: Add `--dry-run` option for merging databases [[#3254](#3254)]
- CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)].
- CLI: Add interactive shell mode command `open` [[#3224](#3224)]

Changed

- Redesign database unlock dialog [ [#3287](#3287)]
- Rework the entry preview panel [ [#3306](#3306)]
- Move notes to General tab on Group Preview Panel [[#3336](#3336)]
- Enable entry actions when editing an entry and cleanup entry context menu  [[#3641](#3641)]
- Improve detection of external database changes  [[#2389](#2389)]
- Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)]
- Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)]
- Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)]
- Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)]
- Merge custom data between databases only when necessary [[#3475](#3475)]
- Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)]
- Add 'New Entry' context menu when no entries are selected [[#3671](#3671)]
- Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)]
- Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)]
- Browser: Show database name when pairing with a new browser [[#3638](#3638)]
- Browser: Show URL in allow access dialog [[#3639](#3639)]
- CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)]
- CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)]
- CLI: Rename command `extract` to `export`. [[#3277](#3277)]

Fixed

- Improve accessibility for assistive technologies [[#3409](#3409)]
- Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)]
- Fix password generator issues with special characters [[#3303](#3303)]
- Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)]
- Fix password visibility toggle button state on unlock dialog [[#3312](#3312)]
- Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)]
- Fix hard-coded background color in search help popup [[#3001](#3001)]
- Fix font choice for password preview [[#3425](#3425)]
- Fix handling of read-only files when autosave is enabled [[#3408](#3408)]
- Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)]
- Enable HighDPI icon scaling on Linux [[#3332](#3332)]
- Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))]
- Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)]
- Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)]
- Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)]
- Fix crash when removing custom data [[#3508](#3508)]
- Fix placeholder resolution in URLs [[#3281](#3281)]
- Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)]
- Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)]
- Browser: Fix password entropy calculation [[#3107](#3107)]
- Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.