Page MenuHomePhabricator

Security review of bjeavons/zxcvbn-php
Open, LowestPublic

Event Timeline

Tgr created this task.Dec 9 2018, 12:16 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 9 2018, 12:16 AM
Paladox added a subscriber: Paladox.Dec 9 2018, 3:53 AM
Reedy added a subscriber: Reedy.Dec 9 2018, 1:30 PM

Based on https://github.com/bjeavons/zxcvbn-php/pull/32 (seems a mismatch with the dropbox js version as detailed in https://github.com/bjeavons/zxcvbn-php/issues/15 ) the upstream might not be very active, and it has actually been forked... https://github.com/mkopinsky/zxcvbn-php and https://packagist.org/packages/mkopinsky/zxcvbn-php

Tgr added a comment.Dec 9 2018, 8:01 PM

Although the discussion there gives the impression that the bjeavons version actually does a better job. And the last commit to the Dropbox repo was over a year ago and the last functional change two years ago, so it's not like there is a constant flood if upstream improvements the library would be expected to keep up with.

The mismatch is a problem if we intend to run zxcvbn on the client side as well, but per T32574#2836570 we probably don't.

Reedy and Tgr, Please decide upon the specific component to be used and update.

sbassett added a subscriber: sbassett.EditedJan 22 2019, 10:56 PM

Pinging @Reedy and @Tgr per @charlotteportero 's comment above.

Tgr added a comment.Jan 22 2019, 11:18 PM

I don't have a strong preference. If we have any plans of running zxcvbn in Javascript we should go with the mkopinsky version, but we are unlikely to run it client-side because it would involve downloading large dictionaries, and a standalone node.js login service is science fiction at this point, so I don't think JS compatibility is something we need to be concerned about. Other than that, they both seem stable, not very active, both have some password quality issues and neither seems obviously superior to the other... the bjeavons version is more widely used which maybe means more eyes on security issues (which is what we are really concerned about here) so if I had to choose I'd go with that.

sbassett triaged this task as Medium priority.EditedJan 23 2019, 4:51 PM

Thanks, @Tgr. Unless @Reedy has strong objections, we'll go with the bjeavons repo and get this scheduled for a security review.

Reedy closed this task as Resolved.Apr 1 2019, 4:20 PM

This can go ahead.

As the fork hasn't diverged much (the API is the same), if we have a strong need we can switch to the other version down the road

chasemp moved this task from Incoming to Our Part Is Done on the secscrum board.Mar 10 2020, 8:19 PM
Tgr reopened this task as Open.Mar 12 2020, 9:59 PM

zxcvbn-php had a stable 1.0 release recently (diff with 0.4, release notes) which included some fairly major changes to make it match the behavior of the original (JS) zxcvbn library (before it was more of a reimplementation of the idea than a proper PHP version of the same library - see bjeavons/zxcvbn-php#15 for more background).

It would be nice to use the 1.0 version, both because of the JS consistency (we probably won't use the JS library, but nice to have that option open), because the algorithm is probably superior, and because it is more modern (e.g. supports same PHP versions as MW master). Does that require a re-review?

sbassett raised the priority of this task from Medium to Needs Triage.Mar 13 2020, 8:01 PM
sbassett moved this task from Our Part Is Done to Incoming on the secscrum board.
Jcross moved this task from Incoming to Back Orders on the secscrum board.Mar 31 2020, 5:01 PM
Jcross added a subscriber: Jcross.Mar 31 2020, 5:03 PM

Hi @Tgr - it does appear to need a re-review. We are putting in our backlog and unassigning until someone can pick it up. We're doing our best but have limited resource hours right now, so please let us know if you have any questions or concerns and we'll be in touch as we move forward. Thanks!

Jcross removed Reedy as the assignee of this task.Mar 31 2020, 5:03 PM
Jcross triaged this task as Medium priority.Mar 31 2020, 5:06 PM
sbassett lowered the priority of this task from Medium to Lowest.Tue, May 26, 4:01 PM

Setting to lowest since there's been no action on this.