Make WordPress Core

Opened 9 years ago

Closed 4 months ago

#31772 closed defect (bug) (reported-upstream)

Browser unresponsive with long password

Reported by: bevanr's profile BevanR Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.7
Component: Users Keywords: has-patch needs-unit-tests
Focuses: javascript, performance Cc:

Description (last modified by SergeyBiryukov)

Steps to reproduce

  1. Login
  2. Navigate to profile
  3. Click in the text input for "New Password"
  4. Paste in a long password
    • E.g. 600 characters:
      123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
      
  5. Press the "tab" key
    • Expected behaviour:
      • The cursor moves to the "Repeat New Password" field.
      • "Strength indicator" changes to "Strong" or "Weak" (probably should be "weak").
    • Actual behaviour:
      • The cursor does not move.
      • The cursor stops blinking.
      • "Strength indicator" does not change.
      • The browser is not responsive.
      • After several seconds or a minute, the cursor eventually starts blinking, moves to the "Repeat New Password" field and the strength indicator changes to "Strong" (probably should be "weak").
  6. Press the "shift+tab" keys
    • Expected behaviour: The focus moves to the "New Password" field and its content is selected.
    • Actual behaviour:
      • The cursor does not move.
      • The cursor stops blinking.
      • The browser is not responsive.
      • After several seconds or a minute, the focus eventually moves to the "New Password" field and the content is selected.
  7. Press the delete/backspace key
    • As expected, the password is deleted and the browser is immediately responsive.

My testing was as an administrator using Chrome on Mac OS X. I think this issue applies to any role and any browser on any OS. I expect the delay is longer with slower hardware.

Solutions

Certainly the strength checker need not run on focus events. It should only be necessary on change.

It could also run after a short timeout (~500 ms) if no other change event has occured, so that it only runs when the user stops typing.

An elaborate solution might be to make the password strength checking code more efficient. Or only run it on the first N characters.

Or alternatively, seek a third party library that checks password strengths more performantly.

A simple solution could be to set the maxlength="" attribute on password <input>s. Rough testing on Chrome on an 18-month old MacBook Air suggests The limit would probably need to be less than about 64-256 in order to keep the browser responsive (without modifying the unperformant code).

The disadvantage of this approach is that long passwords can not be input by the keyboard, or will be truncated if pasted in. This might be an issue for users that use password managing software like OnePassword. We would need to investigate how long the passwords generated by such software is.

For reference and comparison;

Attachments (4)

31772.diff (490 bytes) - added by MikeHansenMe 9 years ago.
31772.patch (784 bytes) - added by BevanR 9 years ago.
31772.2.patch (790 bytes) - added by BevanR 9 years ago.
Improved documentation.
31772-with-tests.patch (2.2 KB) - added by BevanR 9 years ago.

Download all attachments as: .zip

Change History (28)

#1 @chriscct7
9 years ago

I haven't tested the reported issue but a 600 character just numeric password is extremely secure. Even if a brute force program knew you password was completely numeric, a 32 character numeric password would take 32 quintillion years to brute force using BSDI DES. The number of combinations to test in a only numeric password is 10(number of characters).

Last edited 9 years ago by chriscct7 (previous) (diff)

#2 @chriscct7
9 years ago

One easy fix, if confirmed, would be to run the test only when the number of chars is under 32 and just return very strong on anything at or above that

#3 @SergeyBiryukov
9 years ago

  • Description modified (diff)
  • Focuses ui removed

@MikeHansenMe
9 years ago

#4 @MikeHansenMe
9 years ago

  • Keywords has-patch added

31772.diff checks the length and returns strong if it is greater than 32. This avoids running zxcvbn altogether, which was getting hung up a lot on long passwords.

#5 @jorbin
9 years ago

The problem with just checking if the string is a certain length is that we also need to check repeating values. A password of

000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

Which is 0 repeated 960 times is that it is still crackable in 2.064 seconds according to zxcvbn.

Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#6 @BevanR
9 years ago

What if we check only the first 32 characters, when the password is more than 32 characters long?

#7 @MikeHansenMe
9 years ago

  • Keywords has-patch removed

Maybe this should be submitted to zxcvbn? They could determine what approach they would like to take on it. I agree the 000 repeated is not acceptable.

#8 @BevanR
9 years ago

I posted the question on zxcvbn's issue queue: https://github.com/dropbox/zxcvbn/issues/69

#9 @chriscct7
9 years ago

Repeating values of 0's or any other character are perfectly secure if there's alot of characters in the password (like > 20)

Let's say I don't know your password, and I have no preconcieved notions of what it could be. I don't know it, but your password is 32 repeating 0's. So how do I figure out your password? Brute force. The entropy required to bruteforce a string where it can contain upper/lower characters, special characters and numbers is incredibly high.

Even if I knew your password only contained digits, it would still take me 32 quintillion years using the most powerful commonly accepted publically known brute force algorithims. So it's secure, even if to a user it doesn't seem secure. Repeating characters are only a concern when the number of digits is small or constricted.

For example 0000 on an iPhone is insecure because I can brute force it quickly but also I know your password has to be numbers, and there has to be exactly 4 of them (if you use the number pin on older editions), and also 0000 is a common password.

If I have no clue what your password length is, particularly when its long, 512 0's is no more secure than this randomly generated 512 character password

yK&pF=W4B&w!XMrf:nuuMg8@c*=F)t.t[B;F*zj~Cf::B>%gxj(@YK>p-v6RDQRV%T{2](.Tq7sg^;]{z}Y{e<P/FB/<GvM4TX5VH$zz$Xk<Bj,HbUu7fc\vY\$!2cCK}b{RWs~Hx);dUz)N?-J]uk@?R'$%,P,f`LnA@Gj"_mx9`eEm&u]mn4-vw_T~gpEJ%LAkW6~(uDk(^&'6L]&Ud3>]a~\qA`x-te&x&eR,Z;/27y=H<GLT#uS~=>^)Jdr*t5cA[,>E7m:qq/q)=:U#~bZ!c+\3AH&RyWMJW_%Zzg<Z$^]JS:.Q)SKDJ'k~b?Tu;zEP=Z}8}#g/]<s!%=V:>JhfsdC{T!\^@L2}%nas5rD:q<UP$a^WJFr5zkKm~6$YA&8_$bpp>W$w\4X{z$*<r-rBv/}X^R-H!_{}3jqR7?Ub%feQa(^]`q[@~<9N=/u[h6VCG~=Vg&/snH~zmq<W~ZehzEb}W{hy<)A3X4(L7;tX>6etr+ubN3)C>E=wNc%]

Above roughly 20 characters, the entropy required to brute force makes repeating characters or even using English words in passwords irrelevant. If my password is the word "cat" repeated 55 times, its still going at least require tens of billions of years to break.

Last edited 9 years ago by chriscct7 (previous) (diff)

#10 @chriscct7
9 years ago

  • Keywords has-patch added
  • Version changed from 4.1.1 to 3.7

Version set to version bug introduced (introduction of zxcvbn in WordPress 3.7)

#11 @BevanR
9 years ago

@chriscct7; Do your calculations consider the use of rainbow tables as opposed to brute force?

#12 @chriscct7
9 years ago

@BevanR It doesn't need to. Here's why:
Let's assume they're using Rainbow tables for a minute. Know how many combinations you'd need to store in a table for a 32 character password? 95^32, or 1.9 * 10^63 roughly 1.9 vagillion. So first, you'd need to generate that table. Now, let's just assume I can make that table instantly, or I already have it (which irl is not the case at all). Now, GPU based rainbow table is the fastest method, and let's assume we can do 1.720 billion comparisons per second (which is a really high number). So the time to compute is 95^32 / 1.720 billion * seconds or 1.126×10^54 seconds or 3.569×10^46 years or roughly 2.6×10^36 * the estimated age of our universe. Sure, rainbow-tables help, but not as much as you’d think.

#13 @BevanR
9 years ago

If building a rainbow table for the purpose of hacking password data, one would probably not build it from every possible character combination, but from a dictionary of common password words. As a pessimistic example, lets assume the following are true;

  • The dictionary has only 1000 words.
  • The first word is "0000", the default pin for many devices.
  • The rainbow table is built from each of the 1000 words, then every 2-word combination of the 1000 words, then every 3 word combination, etc. up to combinations of at least 8 words.
  • The hacker has access to a botnet of a million devices.
  • Each device makes an average of a billion comparisons per second.
  • The rainbow table already exists.
  • Data transfer time is negligible.

Therefore;

  • The botnet can make 1015 comparisons per second.
  • The index in the rainbow table of "0000" repeated 8 times (32 characters) would be a bit over 1021 (1000+102×3+103×3+104×3+...107×3).
  • "0" repeated 32 times will be identified as the password after 106 clock seconds—on the twelth day.

Of course this is pessimistic. But I think it may still be realistic.

I am not saying that we should check passwords that are 32 characters long. I just think we need to consider both optimistic and pessimistic scenarios.

#14 @BevanR
9 years ago

Web Workers was suggested as a solution in the issue queue for zxcvbn. To me this seems like the best solution yet:

  • Long passwords can still be strength checked, without blocking the user interface.
  • Strength-checking processes that take too long to be useful (the user already changed the password or submitted the form) can be terminated.
  • A new state for the "Strength indicator will need to be introduced: "Checking".

Any objections? If not, I think the next step is to work out if the Web Worker feature should be implemented in zxcvbn or in WP.

#15 @BevanR
9 years ago

  • Keywords needs-patch added; has-patch removed

The maintainer thinks implementing Web Workers is overkill and will instead look to make zxcvbn more performant for long passwords. Since that will take some time, in the meantime they suggested only checking the first ~75 characters as the best solution. The disadvantages of this approach are all very minor and/or extremely unlikely edge cases.

I think this is ready for work again with a patch to only analyse the first 75 characters of the password for its strength.

@BevanR
9 years ago

#16 @BevanR
9 years ago

  • Keywords has-patch added; needs-patch removed

The patch is ready for review and further testing. I have tested it as per the ticket description.

There appears to be no automated test suite for JavaScript, so I believe no code tests can nor should be provided with this patch.

If there is a JavaScript test suite the test should call wp.passwordStrength.meter(long, [], long) with a 2000+ character password and check it returns within 100ms. Those numbers may need further testing and tweaking to be sure that the time taken and expected time (threshold) is sufficiently different on all hardware and test environments which the test suite is likely to run on.

@BevanR
9 years ago

Improved documentation.

#17 @jorbin
9 years ago

  • Keywords needs-unit-tests added

JavaScript tests are in tests/qunit and the password strength meter is one area that we have decent coverage. We can likely use sinon to add a spy into zxcvbn to make sure that only the first 128 characters are being checked.

Tests that rely upon certain time thresholds are harder to guarantee the results from since the machine running them can affect the results. We should avoid adding them. Our JS suite is fairly good at avoiding false negatives and this is one area.

Otherwise, this patch is looking good. If we can get the tests in the next day or two, I think we may be able to add this to 4.2.

#18 @BevanR
9 years ago

Thanks for pointing out the JS tests. I assumed there were no JS tests because this documentation only discusses PHPunit: https://make.wordpress.org/core/handbook/automated-testing/

I forgot to check in the repo in case the documentation was outdated. How can the documentation be corrected?

What exactly should be spied on? zxcvbn() itself? How would you recommend learning Sinon sufficiently to implement an appropriate test?

This ticket was mentioned in Slack in #docs by sam. View the logs.


9 years ago

#20 @BevanR
9 years ago

I found answers to some of my questions on WP's "Make" blog: https://make.wordpress.org/core/2013/09/13/javascript-unit-tests-for-core/.

I thought of a reliable way to test without spying. It compares scores of long passwords that are weak before the threshold. The tests check that WP's score for a password that is strong only after the threshold;

  • matches WP's score for a password that is weak both before and after the threshold.
  • matches Zxcvbn's score for the weak first part of the password.
  • is less than Zxcvbn's score for a password that is strong only after the threshold.

#21 @mattheweppelsheimer
9 years ago

+1 to addressing this. I was about to open a ticket for it. Glad you beat me to it, @BevanR. I routinely generate 200+ character length passwords and experience the same issue.

If an argument needs to be made for prioritizing this for 4.3, then: I see it as a UX problem deterring good security habits. It is not unlike if we were arbitrarily limiting password lengths. Speeding up the strength checking for arbitrarily long passwords will enable users to have better security habits. All power to those of you competent to fix it.

Last edited 9 years ago by mattheweppelsheimer (previous) (diff)

#22 @BevanR
9 years ago

@mattheweppelsheimer; Why and how do you create such long passwords?

I ask because if there are common and good reasons for such long passwords, that might affect the priority of this issue, and also the related might-be-bug; that WP fails silently when a new password is greater than its threshold of about 1 or 2 thousand characters. Note other systems have lower thresholds; Drupal's is about 500 for example. So if your password system or process generates passwords longer than a few hundred characters you are likely to encounter such issues.

FYI, the reason for these thresholds is to prevent DDoS attacks, for which limitless password-hashing would offer a prime attack vector. The limits are not the result of any limitation of the database or technology stack.

#23 @mattheweppelsheimer
9 years ago

WP fails silently when a new password is greater than its threshold of about 1 or 2 thousand characters

Oh, I wasn't aware of that. Good to know. The DDoS vector mitigation reason makes sense. That silent failure might concern me for reasons below… but that's a separate issue that I'll think on.

Why and how do you create such long passwords?

How: I generate them automatically with a KeePassX client — a dynamic generator and encrypted personal password store similar to 1Password or LastPass, but GPL. https://www.keepassx.org/

Why: It's a measure to virtually eliminate the chance of brute forcing or a rainbow table match.

I should say that I am not a security expert, so it's possible there is a flaw in what I've synthesized from reading about how these things work, but here's how I think about it:

200 characters is arbitrary, and probably ridiculous in that it's far beyond what is imaginably necessary — but that's kind of the the point. When using a system like KeePassX, the process of generating and using passwords is identical regardless of the password length. Given that, and considering how the time required to brute force crack hashes increases exponentially with each additional bit added to a password's length, from a user perspective you may as well use the longest possible password that the application will allow.

I doubt this practice is currently a very widespread, but ideally the only barriers to growing adopt are education — not technical UI limitations like our slow password strength checking script.

#24 @pbearne
4 months ago

  • Resolution set to reported-upstream
  • Status changed from new to closed

this was fixed upstream in zxcvbn
https://github.com/dropbox/zxcvbn/issues/69
closing here

Note: See TracTickets for help on using tickets.