search

keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.39k stars 1.48k forks source link

Excluded characters tooltip is incorrect #11044

Closed mhnestler closed 1 month ago

mhnestler commented 4 months ago

Overview

The tooltip text shown when hovering over the "Exclude look-alike characters" checkbox in the Password Generator widget says: Excluded characters: "0", "1", "l", "I", "O", "|", "﹒", but the actual implemented exclusion set rather surprisingly also includes B, G, 6, and 8.

https://github.com/keepassxreboot/keepassxc/blob/afe634d4f2a0f32e96ea720289262a4be90afc0f/src/gui/PasswordGeneratorWidget.ui#L719

Actual exclusion implementation is done in the 5 references to ExcludeLookAlike in generatePassword() in PasswordGenerator.cpp, including:

https://github.com/keepassxreboot/keepassxc/blob/afe634d4f2a0f32e96ea720289262a4be90afc0f/src/core/PasswordGenerator.cpp#L145 https://github.com/keepassxreboot/keepassxc/blob/afe634d4f2a0f32e96ea720289262a4be90afc0f/src/core/PasswordGenerator.cpp#L158

Steps to Reproduce

  1. Open Password Generator.
  2. Click Advanced if the advanced pane is not already open.
  3. Hover mouse pointer over "Exclude look-alike characters" and observe the tooltip.
  4. Tick the checkbox "Exclude look-alike characters" and observe the generated password. For an easy test, try enabling only the Numbers group, set Length to maximum, and note whether there are any 6's or 8's in the password (answer: no).

Expected Behavior

The tooltip should accurately describe what the checkbox does.

I propose, in addition to listing the excluded characters, it should say something like: "Excludes these characters regardless of what character types are enabled".

Correcting this UI is the minimum fix, but perhaps the exclusion behavior should really be changed. I was surprised in 2 ways by this feature: 1st, that "6" and "8" are considered "look-alike"; 2nd, that the checkbox has any effect when only a single character group is selected (numeric). An all-numeric password loses a lot of entropy by only having six characters (when 0, 1, 6, 8 are excluded) instead of all ten digits.

Actual Behavior

The tooltip actually says Excluded characters: "0", "1", "l", "I", "O", "|", "﹒" which implies these are the only excluded characters.

The characters "B", "G", "6", "8" are excluded from all generated passwords when the "Exclude look-alike characters" box is checked.

Context

KeePassXC - Version 2.7.9 Revision: 8f6dd13

Qt 5.15.11 Debugging mode is disabled.

Operating system: macOS 14.5 CPU architecture: arm64 Kernel: darwin 23.5.0

Enabled extensions:

Cryptographic libraries:

Finally, thank you so much for your work on KeePassXC. I have used it for years. This is my first bug report.

droidmonkey commented 4 months ago

You are looking at the look alike wrong. 6 looks like G and 8 looks like B. Implemented here: https://github.com/keepassxreboot/keepassxc/pull/6187

Tool tip just needs to be adjusted.

However, I do agree that if only one category is selected, then these exclusions shouldn't apply. Although you are already in extremely low entropy territory with just one pick group.