Closed clamburger closed 6 years ago
These three are unfixable:
h123456
is detected as a SequenceMatch + SequenceMatch instead of SequenceMatch + DictionaryMatch (seems to affect any password with 12345 in it)on
or no
at the start or end of a password will sometimes cause a DictionaryMatch
to be returned instead of a ReverseDictionaryMatch
, or vice versaEach issue is caused by the same thing: when the upstream library sorts the matches coming out of the Matcher (before sending them into the Scorer), it only checks the i
and j
members of the match. If both are equal, it's arbitrary as to which one is chosen - it's an implementation detail of the browser as to which sorting algorithm is used. This means that browsers experience the same issue: for some matches, Firefox will return a different sequence to Chrome.
Luckily the fallout isn't that bad. guesses
and score
are still guaranteed to be the same everywhere. All we can really do is add a note to the docs.
edit: more investigation revealed that the majority of browsers perform a stable sort (the main exception being Chrome when there are more than ten elements). As such, I've used a stable sort in PHP as well so that it matches most of the time.
I've checked the first million rows of the rockyou password list. Haven't found any further issues.
Use this issue for tracking situations we need to resolve before release
Password1
, should return 0 (fixed, single character detection wasn't working)rockyou
returns 10,718 guesses, should return 45,900 guesses (fixed, minimum guesses wasn't being applied to all matches)098765
and09876
are detected asdvorak
instead ofqwerty
- this results in a different number of guesses (fixed, typo in SpatialMatch caused dvorak matches to return an incorrect number of guesses)marie1
, upstream detects aL33tMatch
from thefemale_names
dictionary, but we detect a normalDictionaryMatch
from thepasswords
dictionary (fixed, our L33tMatch algorithm didn't match upstream properly)ABC123
andPASSWORD1
are missing the 'All-uppercase is almost as easy...' suggestion (fixed, the 'all uppercase' regex was incorrect)getGuesses
to int, which upstream doesn't do (it's returns a float) - this has a maximum error of 1 guess, but can lead to greater effects when it's part of a set of multiple matches (fixed, now returns float)j123456
is detected as a Bruteforce + SequenceMatch instead of Bruteforce + DictionaryMatch (fixed, matchers were in a different order than upstream which led to SequenceMatch being chosen over DictionaryMatches with equal guesses)regex
to match upstream (instead ofyear
) (fixed)!QAZ1qaz
), the second match has 4 times as many guesses as it should (fixed, affected passwords where the first character was shifted)hitenmitsurugi
,soldemedianoche
,inthenameofgod
. In all three cases we return slightly less guesses than upstream.