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

FIX: Password generator skips special characters #3303

Merged

Conversation

@ba32107
Copy link
Contributor

ba32107 commented Jun 21, 2019

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Description and Context

Fixes #3064
Fixes #2749
This issue was introduced by #1841. I found three problems with the code:

  • Even though the generator/SpecialChars setting was saved properly, it was never loaded.
  • The generator/AdvancedMode setting was never saved.
  • The code that initializes the password generation options uses the isVisible method of a UI component to decide which initial character sets to include. When this code first runs (on application startup), the UI is not ready yet at all, therefore the correct code path never runs. Any subsequent action on the UI invokes this code again (this time the visibility check runs correctly), which fixes all settings, as reported in both issues.

I added both settings to the appropriate load/save functions. I also moved the password generator initialization logic to the proper event handler from the constructor.

Testing strategy

  • Tested manually against the bugs reported in #3064 and #2479.
  • Checked config file to verify that the generator/SpecialChars and generator\AdvancedMode setting is saved and loaded correctly.

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]
@ba32107 ba32107 force-pushed the ba32107:hotfix/pass_gen_spec_char_3064 branch from 0b5b4c6 to 9880a9f Jun 21, 2019
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 21, 2019

Sweet you beat me to it!

@droidmonkey droidmonkey added this to the v2.5.0 milestone Jun 21, 2019
@droidmonkey droidmonkey added the bug label Jun 21, 2019
@droidmonkey droidmonkey self-requested a review Jun 21, 2019
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 21, 2019

We need to evaluate the special character groupings that are currently available. Right now I rarely am able to pick a grouping that satisfies stringent site requirements. Usually there are "safe" special characters like !@#$%& that almost all sites accept. That should be part of the basic buttons. The advanced buttons can add to that group allowing punctuation, extended ascii, extended symbols, etc.

We also have a request to add an include free-form text edit that would manually add to any buttons selected. That might be overkill if the buttons are better aligned with what "most people" would use.

@ba32107

This comment has been minimized.

Copy link
Contributor Author

ba32107 commented Jun 21, 2019

I almost never use the advanced options, I usually just pick the special characters on the simple panel - works well on most sites I use.

Not sure I understand the second improvement you mentioned. Is there an issue for it?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 21, 2019

See #3271 and #2313

@ba32107 ba32107 force-pushed the ba32107:hotfix/pass_gen_spec_char_3064 branch from 9880a9f to 054ebde Jun 23, 2019
@ba32107

This comment has been minimized.

Copy link
Contributor Author

ba32107 commented Jun 23, 2019

The new commit I made only resolved the conflicts in the CHANGELOG file (I squashed all changes into one commit, but didn't change anything now apart from the CHANGELOG), so the Windows build failing should be independent. I think there might have been changes in the develop branch that cause the test to fail. Is it perhaps this commit? I'm only assuming because TOTP-related changes are in it:
3022ca9

I am on Ubuntu, all tests pass for me.

EDIT: all green

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jun 24, 2019

I wonder if "Pick characters from every group" should default to off and we replace it with another more visible but temporary switch in case you are trying to generate a password for a website with stupid guidelines. I don't like non-random processes interfering with strong pseudo-random password generation by default.

@ba32107

This comment has been minimized.

Copy link
Contributor Author

ba32107 commented Jun 24, 2019

Just to confirm that I understand you correctly: do you mean that if the "Pick characters from every group" is enabled by default, that adds a certain predictability to the password? If so, yes I agree. And I presume the new switch you're proposing would be called exactly the same, but would be disabled by default.

I like this suggestion. Not sure if it should be done as part of this PR though - it's a pretty nasty bug that has bit many people. Would be great to roll it out as soon as possible.

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jun 24, 2019

It does make the random password less random, yes. Still quite secure, but less random than it could be.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 24, 2019

Most sites with stupid password rules require at least 1 (sometimes 2) characters from each group. Security is impacted by the site itself, not us.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 29, 2019

@ba32107 is this ready or are there more corrections you would like to add?

@ba32107

This comment has been minimized.

Copy link
Contributor Author

ba32107 commented Jun 29, 2019

Sorry, I didn't realize you were waiting for me to make further changes. I considered this PR as ready, because it addresses an important bug. I assumed that we'll tackle @phoerious's suggestion in a separate issue.

@droidmonkey droidmonkey force-pushed the ba32107:hotfix/pass_gen_spec_char_3064 branch from 054ebde to aa88ad2 Jun 29, 2019
@droidmonkey droidmonkey merged commit 11dabfa into keepassxreboot:develop Jun 29, 2019
1 check passed
1 check passed
CodeFactor No issues found.
Details
@ba32107 ba32107 deleted the ba32107:hotfix/pass_gen_spec_char_3064 branch Jun 30, 2019
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
3 participants
You can’t perform that action at this time.