Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

certain bad passwords make it through the filter #51

Open
7ep opened this issue Dec 19, 2019 · 7 comments
Open

certain bad passwords make it through the filter #51

7ep opened this issue Dec 19, 2019 · 7 comments

Comments

@7ep
Copy link

7ep commented Dec 19, 2019

Try this password:

Arvest#1
Expected result: insufficient entropy
Actual result: sufficient

Then try this one:

Arvest#2

Expected result: insufficient entropy
Actual result: is correct - insufficient entropy

You are welcome to use https://github.com/7ep/demo to see this for yourself, in the unit tests at RegistrationUtilsTests at testShouldHaveInsufficientEntropyInPassword

image

@Tostino
Copy link
Collaborator

Tostino commented Dec 20, 2019

Hmm, that's interesting. Will take a look tomorrow. Appreciate the report.

@Tostino
Copy link
Collaborator

Tostino commented Jan 10, 2020

Well, it's finally tomorrow ;). Had a chance to look, and it seems like the "findGoodEnoughCombination" function is returning different results for those two examples, because the 1 gets transformed into a word using l33t transformations, while the 2 does not give the same word.
And using the findGoodEnoughCombination is what is used to determine if a password is random or not, and thus will skip using the findBestCombination method if the password is determined to be random.

So for Arvest#1, the matches it returns are unoptimal, and it is determining that the password is random.

I am working on a fix for this, fingers crossed it won't be too bad.

@minii-dev
Copy link

Also, the password "qwer" returns better score (1) than qwert, qwerty, qwertyu... (0)
If the check thinks qwer is score 1, is it a good idea to use the checker or it just allows us to think "I made things right" still being insecure?
What is the main purpose of this library?

@Tostino
Copy link
Collaborator

Tostino commented May 19, 2020

@minii-dev That is a perfectly acceptable thing for the library to do, through all the algorithms, using weighted dictionaries, we are calculating that an attacker is more likely to try qwert, qwerty, or qwertyu than they are to try qwer.

That is because qwerty is extremely high in the list of "bad passwords", and even though qwer is shorter, an attacker is more likely to try qwerty first because it appears in practically every leaked password dictionary right near the top.

The "score" is a simplified representation of the entropy, if you look at the Result you'd see why the assigned the entropy like it did. A score of 1 is still unacceptable, even though it may be higher than qwerty.

So that is a totally different situation to the issue mentioned above, which I did actually do some work on the issue @7ep raised, but it introduced other edge cases... I haven't had enough time with how busy work has been to do any more on that side since January though =(.

@minii-dev
Copy link

minii-dev commented May 20, 2020

Thank you for your answer, it sounds very good. It is strange to me if you say the attacker will try qwert, qwerty, qwertyu with the same chance, and it is much higher than qwer, because produces different score (for so short word, which costs nothing to brute force). I would fully accept your answer in the case the score was the same, but it differs. Do you really think the score 1 for qwer and 0 for qwertyu is correct. Or maybe the correct score should be 0 for both?
Thank you for the project!

@Tostino
Copy link
Collaborator

Tostino commented May 20, 2020

Maybe looking at the output would make more sense.
When we get the entropy for: qwer
image

We can see that the lowest entropy match it could find for the word qwer ended up being a spacial match. I also manually looked and did not find any matches for exactly "qwer" in any of our dictionaries.

Now lets look at: qwert
image

When we look at qwert, we can see that it appears exactly at position 420 within the default password dictionary.

Nbvcxz, nor any library will be able to get perfect results, we are trying to use knowledge about how attackers operate to anticipate how they go about attacking passwords. Using word lists and word permutations is extremely common for attackers, so we model that into our simulation to give an estimate on how many attempts an attacker may take on any given password. It uses all the algorithms, rules, etc to guide us to a best guess, but in the end, that's all it is, as best a guess as we can muster on how easy a password would be to attack.

If one of these was returning a vastly different result, that would be something to look at a changing behavior for. That is not what's happening here though. You are paying way too much attention to the "score" (which is why I originally didn't implement that feature).

Look at the time to crack instead, both of these are instant or near instant on all offline attacks using bcrypt, and extremely short time to crack for online attacks.

A score of 1 vs 0 is pretty arbitrary, one of them barely passed the threshold to get marked as "1" while the other didn't. In the end they are both woefully insecure passwords that don't meet the default minimum entropy for the library.

@minii-dev
Copy link

It is a good explanation using the entropy and the threshold and now the difference in score for qwer and qwert is clear to me. Thanx.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants