Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#25174 closed task (blessed) (fixed)

Expand zxcvbn user_input blacklist

Reported by: iandunn's profile iandunn Owned by:
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.7
Component: Security Keywords: has-patch
Focuses: Cc:

Description

The current blacklist only contains the username, but there are other known data about the current user/site that we should discourage using in passwords, because they'll lower the entropy.

I've attached a rough first pass. It needs more work, but I'd like to get some feedback.

  • There's probably a better location for zxcvbn_user_input_blacklist()
  • Are there performance concerns with zxcvbn_user_input_blacklist() ? There are a lot of function calls and processing, and there may be a more elegant ways to get the same results.
  • Any more suggestions for additional generic words to blacklist?
  • Are there any security/privacy issues, since all of the data returned by zxcvbn_user_input_blacklist() will be revealed in the page source? Probably not in the typical usage, since it's only shown on user-edit.php (and therefore is already behind a current_user_can() check). There could be issues if it were (mis)used by plugins, though.
  • Any other issues?

Note that there's currently a bug in the zxcvbn implementation where user_input is being ignored, so this patch won't actually affect the returned score until Jon's latest patch is committed.

Attachments (5)

25174.diff (4.8 KB) - added by iandunn 11 years ago.
25174.2.diff (3.9 KB) - added by iandunn 11 years ago.
25174.3.diff (4.0 KB) - added by iandunn 11 years ago.
25174.4.diff (4.4 KB) - added by nacin 11 years ago.
25174.5.diff (4.8 KB) - added by iandunn 11 years ago.

Download all attachments as: .zip

Change History (18)

@iandunn
11 years ago

#1 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.7

#2 @nacin
11 years ago

Could most of this (for the user being edited or created) simply be collected via JS?

#3 @iandunn
11 years ago

Yeah, and that'd have an extra benefit of making sure we're blacklisting the most current values for those fields, in cases where the user updates usermeta at same time as the password.

We could also get the site url and title from the DOM, but we'd miss the following items:

  • Site description
  • admin_email
  • If the current user is editing another user, we wouldn't have the current user's:
    1. user_login,
    2. user_nicename
    3. user_email
    4. user_url
    5. first_name
    6. last_name
    7. description

So, is it worth the tradeoff? I'd personally err on the side of making the entropy score more accurate, even at the expense of a little bit of performance. Passwords are possibly the weakest link in the security chain, so educating users about what makes one strong is very important; and since changing a password is an infrequent occurrence, the performance impact won't be felt very often (if it's even noticeable). I can see the other side, though.

Or should we go with a hybrid approach? The PHP side could do the minimal amount of work to collect the data that only it can get, then pass that off to the client side. Then a JS function could add in all of the data that it has access to, and do all of the processing to clean up the array before it gets used. That way we'd get both the previous usermeta values, and the current ones.

#4 @desrosj
11 years ago

  • Cc desrosj@… added

#5 @nacin
11 years ago

I think we could do this entirely via JS, by building an array using not only the current value of the inputs, but also the original value of them, using defaultValue.

Then all we'd need to worry about is passing the current user's information to JS. No security concerns there.

home URL and site title are fine, but I would probably ignore admin_email.

Let's try to clean up a lot of the todo's here.

#6 @nacin
11 years ago

Realistically, let's work from the inside out. So, let's start with using not only the username of the user being edited, but also other information about that user, both defaultValue and current values. This should be 100% JS, I think. That'd be a good start no matter what, and it'd be committable. Anything else after that is just a bonus.

#7 @nacin
11 years ago

  • Type changed from enhancement to task (blessed)

@iandunn
11 years ago

#8 @iandunn
11 years ago

25174.3.diff is an all-JS solution. I haven't tested thoroughly yet, but I think it's most of the way there. It works fine on the current user's profile, another user's profile, the install, and the unit tests.

The returned array ends up with some extra words like "profile", "installation", "admin", etc, but I don't think filtering them out is really necessarily. It would make the code a bit more complex and slower, and those words are gonna be heavily penalized by the dictionary checks anyway.

Note that zxcvbn's user_input check is case-sensitive, which significantly reduces the effectiveness of the blacklist. It's still a worthwhile improvement, though.

@iandunn
11 years ago

@nacin
11 years ago

#9 @nacin
11 years ago

25174.4.diff introduces wp.passwordStrength to collect these two methods. Passes the tests.

#10 @iandunn
11 years ago

25174.5.diff has a few small changes:

I ran the unit tests on bunch of browsers/devices and found no problems. I also did some manual testing on various browsers without incident. So, I think it's good to commit.

@iandunn
11 years ago

#11 @nacin
11 years ago

In 25637:

Expand the zxcvbn password meter blacklist, based on user input.

props iandunn.
see #25174.

#12 @nacin
11 years ago

  • Resolution set to fixed
  • Status changed from new to closed

I had left this open for review and further testing, but things seem good.

#13 @SergeyBiryukov
11 years ago

  • Version changed from trunk to 3.7
Note: See TracTickets for help on using tickets.