Check passwords against the HIBP online service #4438

wolframroesler · 2020-03-14T12:50:40Z

Type of change

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

Description and Context

Add "HIBP" to the "Database -> Reports" panel to check all passwords in the database against the HIBP online service, and display the findings in a table. The "worst" passwords (found the most times in the HIBP database) are listed at the beginning of the table (just as in Health Check).

Offline checks (against a previously downloaded HIBP database file) will be added in another PR.

Lots of temporary commits, feel free to squash or fixup.

Fixes #1083 (together with the upcoming PR for offline checks).

Screenshots

Testing strategy

Tested manually.

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]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

droidmonkey · 2020-03-15T14:12:38Z

I squashed and rebased all the commits. Only 1 visual comment here:

This dialog is way too wordy and steals focus. I would much prefer this to be in the actual HIBP report widget. For example, before permission is granted, a similar message to this is displayed instead of the results table with only one button "Perform Password Analysis". The technical details written in this dialog are irrelevant to the vast majority of users. Those should be hidden behind a "learn more" link that reveals them.

Otherwise this performs REALLY REALLY well! I got great results from my personal database. The only issue I saw was my credit card entries where I put my CVC code in the password field. That is fine because it was obvious to me and entry templates will help with that problem in the future.

droidmonkey · 2020-03-15T14:23:03Z

I am checking the code right now, and where it does work there are many efficiencies that can be had. Why do you jump from QString to StdString to C_Str? All operations can be smoothly done in QString. Additionally, you only use the HIBPDownloader once per password then reconstruct it, that is wasteful and the network manager can be loaded up with requests and it will do 5 at a time in batches. This will speed up lookups significantly.

src/core/HibpDownloader.cpp

src/gui/reports/ReportsDialog.cpp

src/gui/reports/ReportsWidgetHibp.cpp

wolframroesler · 2020-03-21T17:31:50Z

Reworked as requested, please re-review.

One more thing. I tried to put the following at the beginning of HibpDownloader.hpp:

#ifndef WITH_XC_NETWORKING
#error This file requires KeePassXC to be built with network support.
#endif

to make sure it was included only when KeePassXC was built with network support. To my surprise, the #error fires all the time, even when building with cmake -DWITH_XC_ALL=ON. In the .cpp files, #ifdef WITH_XC_NETWORKING works as expected, but in the .h files, WITH_XC_NETWORKING never seems to be defined (same in ReportsWidgetHibp.cpp/.h). Any explanation for that?

droidmonkey · 2020-03-21T18:11:16Z

You need to #include "config-keepassx.h" to use the predefines. We should probably be injecting those as defines in the build call instead of stuffing them in an include file. This is a carry over from KeePassX.

droidmonkey · 2020-03-28T15:53:05Z

@wolframroesler for your awareness i have made significant code enhancements locally and will be pushing them soon. I optimized as much as I could and standardized to Qt and code base.

droidmonkey · 2020-03-29T01:23:35Z

Pushed my code changes, here are some screenshots:

wolframroesler · 2020-03-29T10:58:24Z

src/gui/reports/ReportsWidgetHibp.h


 private:
    void makeHibpTable();
    void startValidation();
+    QString countToText(int count);


Is there a reason why this is a non-static, non-const member function?

wolframroesler · 2020-03-29T11:07:19Z

@droidmonkey Thanks for the rework, learned a lot from it :) I should have guessed that something like toHex exists in an all-bells-and-whistles-included library like Qt.

I particularly like the reworked warning message that appears when you switch to the HIBP pane, it's much better than my modal dialog. It looks like it's made to add a second button, "Perform Offline Analysis", after which you select your downloaded HIBP file in case you don't want to go online. Will implement that on a new branch after this one has been merged.

About the warning message, it says

your database passwords will be cryptographically hashed and the first five characters sent securely to this service

Will everyone understand that this is referring to the first five characters of the hash and not of the password?

droidmonkey · 2020-03-29T11:42:40Z

Great point!

* Fixes keepassxreboot#1083 * Add online HIBP checker dialog to the database reports widget. Permission is requested from the user prior to performing any network operations. * The number of times a password has been found in a breach is shown to the user. * If no passwords are breached then a positive message is presented. * Source of HIBP icon: https://github.com/simple-icons/simple-icons/blob/develop/icons/haveibeenpwned.svg

wolframroesler · 2020-03-29T11:58:40Z

Fixed two details: fetchFailed had a wrong signature (produced a message on stdout), and it failed to build without networking support.

droidmonkey · 2020-03-29T12:26:24Z

I did actually intend to remove the password QString from fetchFailed since it is unused and doesn't really matter. I completed my original change in the last commit and fixed the caution message. This should be good to go.

droidmonkey · 2020-03-29T13:39:17Z

Claim your bounty good sir!

wolframroesler · 2020-03-29T14:15:54Z

Thanks @droidmonkey :) will claim the bounty when offline checks are done too.

droidmonkey · 2020-03-29T14:16:57Z

Don't wait for that, I think there are some time restrictions on this bounty. I can't recall if I added a 3 month limit or not.

wolframroesler · 2020-03-29T14:24:34Z

https://www.bountysource.com/issues/50410294-check-passwords-against-hacked-password-databases says "You will be able to claim this bounty as soon as the original issue is marked as closed" so I suppose #1083 needs to be closed first.

droidmonkey · 2020-03-29T14:30:58Z

It is closed 😬

danielporto · 2020-04-20T10:47:45Z

I'm so looking forward for this! Thanks a lot!!

Added - Custom Light and Dark themes [#4110, #4769, #4791, #4796, #4892, #4915] - Compact mode to use classic Group and Entry line height [#4910] - View menu to quickly switch themes, compact mode, and toggle UI elements [#4910] - Search for groups and scope search to matched groups [#4705] - Save Database Backup feature [#4550] - Sort entries by "natural order" and move lines up/down [#4357] - Option to launch KeePassXC on system startup/login [#4675] - Caps Lock warning on password input fields [#3646] - Add "Size" column to entry view [#4588] - Browser-like tab experience using Ctrl+[Num] (Alt+[Num] on Linux) [#4063, #4305] - Password Generator: Define additional characters to choose from [#3876] - Reports: Database password health check (offline) [#3993] - Reports: HIBP online service to check for breached passwords [#4438] - Auto-Type: DateTime placeholders [#4409] - Browser: Show group name in results sent to browser extension [#4111] - Browser: Ability to define a custom browser location (macOS and Linux only) [#4148] - Browser: Ability to change root group UUID and inline edit connection ID [#4315, #4591] - CLI: `db-info` command [#4231] - CLI: Use wl-clipboard if xclip is not available (Linux) [#4323] - CLI: Incorporate xclip into snap builds [#4697] - SSH Agent: Key file path env substitution, SSH_AUTH_SOCK override, and connection test [#3769, #3801, #4545] - SSH Agent: Context menu actions to add/remove keys [#4290] Changed - Complete replacement of default database icons [#4699] - Complete replacement of application icons [#4066, #4161, #4203, #4411] - Complete rewrite of documentation and manpages using Asciidoctor [#4937] - Complete refactor of config files; separate between local and roaming [#4665] - Complete refactor of browser integration and proxy code [#4680] - Complete refactor of hardware key integration (YubiKey and OnlyKey) [#4584, #4843] - Significantly improve performance when saving and opening databases [#4309, #4833] - Remove read-only detection for database files [#4508] - Overhaul of password fields and password generator [#4367] - Replace instances of "Master Key" with "Database Credentials" [#4929] - Change settings checkboxes to positive phrasing for consistency [#4715] - Improve UX of using entry actions (focus fix) [#3893] - Set expiration time to Now when enabling entry expiration [#4406] - Always show "New Entry" in context menu [#4617] - Issue warning before adding large attachments [#4651] - Improve importing OPVault [#4630] - Improve AutoOpen capability [#3901, #4752] - Check for updates every 7 days even while still running [#4752] - Improve Windows installer UI/UX [#4675] - Improve config file handling of portable distribution [#4131, #4752] - macOS: Hide dock icon when application is hidden to tray [#4782] - Browser: Use unlock dialog to improve UX of opening a locked database [#3698] - Browser: Improve database and entry settings experience [#4392, #4591] - Browser: Improve confirm access dialog [#2143, #4660] - KeeShare: Improve monitoring file changes of shares [#4720] - CLI: Rename `create` command to `db-create` [#4231] - CLI: Cleanup `db-create` options (`--set-key-file` and `--set-password`) [#4313] - CLI: Use stderr for help text and password prompts [#4086, #4623] - FdoSecrets: Display existing secret service process [#4128] Fixed - Fix changing focus around the main window using tab key [#4641] - Fix search field clearing while still using the application [#4368] - Improve search help widget displaying on macOS and Linux [#4236] - Return keyboard focus after editing an entry [#4287] - Reset database path after failed "Save As" [#4526] - Use SHA256 Digest for Windows code signing [#4129] - Improve handling of ccache when building [#4104, #4335] - macOS: Properly re-hide application window after browser integration and Auto-Type usage [#4909] - Auto-Type: Fix crash when performing on new entry [#4132] - Browser: Send legacy HTTP settings to recycle bin [#4589] - Browser: Fix merging browser keys [#4685] - CLI: Fix encoding when exporting database [#3921] - SSH Agent: Improve reliability and underlying code [#3833, #4256, #4549, #4595] - FdoSecrets: Fix crash when editing settings before service is enabled [#4332]

wolframroesler changed the title ~~WIP: Check passwords against the HIBP database~~ Check passwords against the HIBP database Mar 14, 2020

wolframroesler changed the title ~~Check passwords against the HIBP database~~ Check passwords against the HIBP web service Mar 14, 2020

wolframroesler changed the title ~~Check passwords against the HIBP web service~~ Check passwords against the HIBP online service Mar 14, 2020

wolframroesler force-pushed the feature/hibp branch from e74f100 to b510b7f Compare March 14, 2020 15:20

droidmonkey force-pushed the feature/hibp branch from b510b7f to df10047 Compare March 15, 2020 14:14

droidmonkey requested changes Mar 15, 2020

View reviewed changes

droidmonkey added new feature feature: Reports labels Mar 15, 2020

droidmonkey added this to the v2.6.0 milestone Mar 15, 2020

droidmonkey assigned wolframroesler Mar 15, 2020

wolframroesler force-pushed the feature/hibp branch from 106e0aa to 6b8d02d Compare March 21, 2020 10:52

wolframroesler requested a review from droidmonkey March 22, 2020 20:55

droidmonkey force-pushed the feature/hibp branch from 7453ec2 to 41a2738 Compare March 29, 2020 01:25

wolframroesler commented Mar 29, 2020

View reviewed changes

wolframroesler added 7 commits March 29, 2020 13:56

Use a function instead of ?:?:?: to get the count text

2448421

Use Qt instead of C++ types/fcts for the item list

88f0e85

Don't need Group* in m_rowToEntry, use entry->group()

cf174df

Don't pass Group* to edit signal handler, use entry->group()

460d974

Don't pass sender to signal handler, use QObject::sender()

fc85fbd

Use QSharedPointer::data instead of &*sharedpointer

28ebf7b

wolframroesler and others added 8 commits March 29, 2020 13:56

Use Qt instead of C string functions to search the hash

8e77bf0

Loop over passwords in HIBP downloader, not in reports widget

761dccf

make format

16550f9

Rework re-validating editied passwords against HIBP

8abd26e

Fix forgotten slot name

6a7305c

Massive code improvements

324ada0

HIBP: Use proper signature of fetchFailed

524ff97

HIBP: Allow building without networking support

fc85af3

wolframroesler force-pushed the feature/hibp branch from 41a2738 to fc85af3 Compare March 29, 2020 11:56

Finished previous work

c11a0fc

droidmonkey removed the PR: Needs Work label Mar 29, 2020

droidmonkey approved these changes Mar 29, 2020

View reviewed changes

droidmonkey merged commit 83ed9a8 into keepassxreboot:develop Mar 29, 2020

purarue mentioned this pull request Jul 8, 2020

Consider upgrading to keepassxc 2.6.0 purarue/keepassxc-pwned#17

Closed

jonafato mentioned this pull request Nov 8, 2020

keepassxc: Enable networking by default NixOS/nixpkgs#103146

Merged

10 tasks

bwbroersma mentioned this pull request Oct 15, 2024

Werkgroep BIO actiepunt 300624-2 - mogelijke herformulering o-maatregel 5.17.02 MinBZK/Baseline-Informatiebeveiliging-Overheid#167

Closed

phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check passwords against the HIBP online service #4438

Check passwords against the HIBP online service #4438

wolframroesler commented Mar 14, 2020 •

edited

Loading

droidmonkey commented Mar 15, 2020

droidmonkey commented Mar 15, 2020

wolframroesler commented Mar 21, 2020

droidmonkey commented Mar 21, 2020 •

edited

Loading

droidmonkey commented Mar 28, 2020

droidmonkey commented Mar 29, 2020

wolframroesler Mar 29, 2020

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020 •

edited

Loading

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

danielporto commented Apr 20, 2020

Check passwords against the HIBP online service #4438

Check passwords against the HIBP online service #4438

Conversation

wolframroesler commented Mar 14, 2020 • edited Loading

Type of change

Description and Context

Screenshots

Testing strategy

Checklist:

droidmonkey commented Mar 15, 2020

droidmonkey commented Mar 15, 2020

wolframroesler commented Mar 21, 2020

droidmonkey commented Mar 21, 2020 • edited Loading

droidmonkey commented Mar 28, 2020

droidmonkey commented Mar 29, 2020

wolframroesler Mar 29, 2020

Choose a reason for hiding this comment

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020 • edited Loading

wolframroesler commented Mar 29, 2020

droidmonkey commented Mar 29, 2020

danielporto commented Apr 20, 2020

wolframroesler commented Mar 14, 2020 •

edited

Loading

droidmonkey commented Mar 21, 2020 •

edited

Loading

droidmonkey commented Mar 29, 2020 •

edited

Loading