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

Limit password entropy calculation using zxcvbn to 128 characters #7748

Conversation

libklein
Copy link
Contributor

Fixes #7712.

Essentially changes the behavior of password strength updates in two ways:

  1. Performs password strength calculation asynchronously if the password is sufficiently large:
  • Displays an estimate based on a password truncated to 40 characters
  • Updates the estimate once the calculation resolves
  1. Debouces password updates to avoid spawning superfluous calculations.
  • Implemented as a single shot timer that triggers the calculation, reset on each update request.

Testing strategy

Refactored existing tests and added new ones checking an exceedingly long password.

Type of change

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

Discussion

  • I think it would be helpful to keep a copy of the requested password in the PasswordHealth class
  • Should we display some hint when displaying an entropy estimate? If so, i'd suggest "Entropy: > x bits". Any ideas on how to change the current string "Entropy: x bits" while preserving existing translations?

Issues

  • Tests are very time sensitive and may need tuning on CI.

@droidmonkey
Copy link
Member

droidmonkey commented Mar 31, 2022

I'd prefer to not make this asynchronous, just limit the use of zxcvbn to 128 characters and use simple log estimation after that. AsyncTask has the nasty side effect of compounding the stack because it creates an event loop on top of an event loop.

@libklein libklein force-pushed the feature/async-password-entropy-check branch from 77531b8 to 776bd8f Compare April 3, 2022 15:31
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.29%. Comparing base (a4d4adb) to head (92928f5).
Report is 434 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #7748   +/-   ##
========================================
  Coverage    64.29%   64.29%           
========================================
  Files          339      339           
  Lines        43431    43437    +6     
========================================
+ Hits         27923    27927    +4     
- Misses       15508    15510    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@droidmonkey droidmonkey changed the title Feature/async password entropy check Limit password zxcvbn to 128 characters May 1, 2022
@droidmonkey droidmonkey changed the title Limit password zxcvbn to 128 characters Limit password entropy calculation using zxcvbn to 128 characters May 1, 2022
@droidmonkey droidmonkey added bug pr: backport pending Pull request yet to be backported to a previous release labels May 1, 2022
@droidmonkey droidmonkey added this to the v2.7.2 milestone May 1, 2022
@droidmonkey droidmonkey self-requested a review May 1, 2022 14:14
@libklein libklein force-pushed the feature/async-password-entropy-check branch from 6788a67 to fd17468 Compare May 13, 2022 21:26
@droidmonkey droidmonkey force-pushed the feature/async-password-entropy-check branch from fd17468 to 9720030 Compare May 28, 2022 15:25
@droidmonkey
Copy link
Member

droidmonkey commented May 28, 2022

I basically rewrote this entire PR. Instead of just changing this in the password generator, I changed it at the source of Zxcvbn usage, PasswordHealth. This has the positive benefit of reducing CPU usage no matter where we calculate entropy! Additionally, I upped the limit to 256 bytes, removed the chunked calculation, and replaced it with a byte-average calculation (when > 256 bytes). The results are within a 1-2% difference from pure zxcvbn calcs in my testing:

Length Pure Zxcvbn Zxcvbn + Estimation
257 1418 1417.56
512 2734.62 2670.76
999 5261.79 5279.93

Copy link
Contributor Author

@libklein libklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, IMHO it's probably a waste of resources to provide an estimate for password beyond 256 characters, so i think that the modifications are fine as it (except the byte split thing). To play the devil's advocate, here are some concerns regardless:

How did you measure the average error? ZXCVBN is pretty sophisticated and the runtime/entropy estimation depends very much on the format of the password. It somewhat suprises me that there is so little difference between estimations.
See https://www.usenix.org/conference/usenixsecurity16/technical-sessions/presentation/wheeler.

I did not modify PasswordHealth directly as i figured there may be cases (say for database audits) where we want an accurate estimate of password strength. Perhaps this should be an alternative implementation of the PasswordHealth interface?

{
auto entropy = 0.0;
auto pwdBytes = pwd.toUtf8();
entropy += ZxcvbnMatch(pwdBytes.left(ZXCVBN_ESTIMATE_THRESHOLD), nullptr, nullptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work with multi-byte UTF-8 chars? Maybe it's better to split based on character count and not on bytes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically you'd only have multibyte unicode characters if you pasted them into the password field. We don't support generating passwords with unicode. But yes, we can do the limit check on the string, then convert it to utf8 bytes for zxcvbn.

@droidmonkey
Copy link
Member

The error is irrelevant for any meaningful analysis. It also remains small if the remaining bytes in the string have the same amount of entropy. If you have a non random first 256 bytes and a random remaining you'd see the most divergence (and vice versa). However, at that point you are overflowing hash algorithms and your password, no matter its randomness, is equivalent to guess the hash itself.

Plus we bin your passwords by rating, excellent is reached well before you hit 256 bytes.

Limit the use of zxcvbn based password entropy estimation to 256 bytes. After this threshold, the average per-byte entropy from the zxcvbn calculation is added for each additional byte. In practice, this produces a slightly higher entropy calculation for purely randomized passwords than zxcvbn would normally calculate. However, the time to calculate is capped leading to a much better user experience and removing unnecessary calculations.

Fixes keepassxreboot#7712
@droidmonkey droidmonkey force-pushed the feature/async-password-entropy-check branch from 9720030 to 92928f5 Compare May 28, 2022 19:23
@droidmonkey droidmonkey merged commit 65a1d1b into keepassxreboot:develop May 30, 2022
@droidmonkey droidmonkey added pr: backported Pull request backported to previous release and removed pr: backport pending Pull request yet to be backported to a previous release labels Jun 27, 2022
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: backported Pull request backported to previous release pr: bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit size of password sent to zxcvbn to prevent lock ups
4 participants