Page MenuHomePhabricator

Security review of bjeavons/zxcvbn-php
Closed, DeclinedPublic

Event Timeline

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

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.

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.

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

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.

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.May 26 2020, 4:01 PM

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

Is there any plan to use this if we re-review it? Is anyone likely to work on implementing it/using it?

Obviously if we do re-review, it needs prioritising etc.

JBennett added a subscriber: JBennett.

The Security team is updating their readiness review SOP to reflect a new change that any request that has aged 90 days without being in a reviewable state will be declined. We do this to help keep our work area current, accurate and reflective of actual work. If the status of your project changes please re-tag us and we will get this work scheduled.

@Jcross: Please do not remove correct metadata as it makes it harder to find tasks. This is and was a security readiness review request (which has been declined). Thanks.