Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Complexify bans passwords containing banned words #31

Open
Download opened this issue Jun 22, 2015 · 10 comments
Open

Complexify bans passwords containing banned words #31

Download opened this issue Jun 22, 2015 · 10 comments

Comments

@Download
Copy link

Please go to the complexify homepage and type in this password:

Id0ntTh!nkTh1sB4gSh04ldGet4Fr33Pass

Notice how its complexity is 0%.
Now remove the last letter:

Id0ntTh!nkTh1sB4gSh04ldGet4Fr33Pas

Notice how its complexity is now 100%.

The docs on banMode state that:

banMode:

Either:

  • strict: If a password is contained in the banned list, or contained in any item of the banned list, the password will fail. This means that "123456" will fail as it is in the banned list, but "123" and "345" will also fail as they are substrings of a password in the list.
  • loose: If a password exactly matches one in the banned list, the password will fail.

My password is not in the banned list, nor is it a substring of any item in the banned list. It contains an item from the banned list, but as I read the docs that should be ok?

@msigley
Copy link
Contributor

msigley commented Jun 23, 2015

I think this is an error in the documentation. It would make no sense to have the banned password check reject parts of the actually banned passwords. If this was the case a password of "45ss6gt7" would be banned because the banned password of "password" has "ss" in it.

Strict mode actually rejects passwords if they used in a substring of the password in addition to the exact match ban. An example of a rejection in strict mode would be the password "strong456password!" because the banned password "password" is a substring of it.

@Download
Copy link
Author

It would make no sense to have the banned password check reject parts of the actually 
banned passwords. If this was the case a password of "45ss6gt7" would be banned because 
the banned password of "password" has "ss" in it.

Not sure if we understand each other / the docs correctly.

What I think you are saying is that a password would be rejected if it contains a part of a banned password (so pwd contains substring of banned)

What I read in the docs is that if the password is a substring of a banned password it would be rejected:

So, given banlist = ['password']

pass -> rejected (is substring of banned 'password')
passage -> ok (contains substring of banned 'password')
password -> rejected (is banned 'password')
passwords -> ok (contains banned 'password')

Now I can see why you would say that passwords would not be ok... It's not what I read in the docs but I can understand why you would say it contains a banned password so it's rejected. This seems to be the current behavior at least.

However, personally I think it's strange. It makes the banlist unusable as far as I'm concerned. Especially with shorter banned strings it gives a lot of banned combinations. Many of which will be impossible to explain to users. Looking at the example I gave I think it's really weird that typing that extra 's' at the end makes the complexity drop from 100% to 0%. There is no real truth to that. The password with the extra 's' in it is stronger than the one without, whichever way you look at it. I can't (am not willing to try to) explain this to my users so currently I have disabled the banlist because of it.

With the current behavior having short passwords in the banlist will give you lots of rejections. And having this banlist:

['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

Would make it impossible to create any password with any lowercase letter in it....

The behavior as documented does make a lot of sense to me. Of course if 'password' is banned because it's too weak, then 'passwor', 'passwo', 'passw' etc will all also be too weak. That makes perfect sense. But doing a contains makes it much too strict imho. Even this would be considered as complexity 0%:

'Th1sOñeÌsVeryL0ngAnd!AlmostImpossibleToGu€ssB4tStillB4nnedBecauseItEndsWithPass'

It ends with 'Pass' so it's rejected.... Silly imho. Also to me it's really weird behavior that the password can be rejected at any keystroke... Once the password is 12 or so characters long it doesn't make any sens in my mind that adding an extra character would invalidate it...

In my humble opinion, the docs are (should be) correct and the current behavior of complexify should be changed to match the docs. At least that would make the banlist usable for me. If you change the docs to match the current behavior, then the specified behavior becomes much, much more strict. And as such not usable for me any more as I want to sign-up process to be simple and painless for my users.

@Download
Copy link
Author

I opened issues in the projects of the different ports of complexify to request feedback on whether they think the docs are correct or the current behavior.

@msigley
Copy link
Contributor

msigley commented Jun 23, 2015

Have you tried loose mode? It only matches banned passwords exactly. Loose mode is what I ended up with on my websites because I didn't like the way strict matching worked either.

-----Original Message-----
From: "Stijn de Witt" notifications@github.com
Sent: ‎6/‎23/‎2015 4:49 AM
To: "danpalmer/jquery.complexify.js" jquery.complexify.js@noreply.github.com
Cc: "Matthew Sigley" matthew.d.sigley@gmail.com
Subject: Re: [jquery.complexify.js] Complexify bans passwords containingbanned words (#31)

It would make no sense to have the banned password check reject parts of the actually banned passwords. If this was the case a password of "45ss6gt7" would be banned because the banned password of "password" has "ss" in it.
Not sure if we understand each other / the docs correctly.
What I think you are saying is that a password would be rejected if it contains a part of a banned password (so pwd contains substring of banned)
What I read in the docs is that if the password is a substring of a banned password it would be rejected:
So, given banlist = ['password']
pass -> rejected (is substring of banned 'password')
passage -> ok (contains substring of banned 'password')
password -> rejected (is banned 'password')
passwords -> ok (contains banned 'password')
Now I can see why you would say that passwords would not be ok... It's not what I read in the docs but I can understand why you would say it contains a banned password so it's rejected. This seems to be the current behavior at least.
However, personally I think it's strange. It makes the banlist unusable as far as I'm concerned. Especially with shorter banned strings it gives a lot of banned combinations. Many of which will be impossible to explain to users. Looking at the example I gave I think it's really weird that typing that extra 's' at the end makes the complexity drop from 100% to 0%. There is no real truth to that. The password with the extra 's' in it is stronger than the one without, whichever way you look at it. I can't (am not willing to try to) explain this to my users so currently I have disabled the banlist because of it.
With the current behavior having short passwords in the banlist will give you lots of rejections. And having this banlist:
['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']
Would make it impossible to create any password with any lowercase letter in it....
The behavior as documented does make a lot of sense to me. Of course if 'password' is banned because it's too weak, then 'passwor', 'passwo', 'passw' etc will all also be too weak. That makes perfect sense. But doing a contains makes it much too strict imho. Even this would be considered as complexity 0%:
'Th1sOñeÌsVeryL0ngAnd!AlmostImpossibleToGu€ssB4tStillB4nnedBecauseItEndsWithPass'
It ends with 'Pass' so it's rejected.... Silly imho. Also to me it's really weird behavior that the password can be rejected at any keystroke... Once the password is 12 or so characters long it doesn't make any sens in my mind that adding an extra character would invalidate it...
In my humble opinion, the docs are (should be) correct and the current behavior of complexify should be changed to match the docs. At least that would make the banlist usable for me. If you change the docs to match the current behavior, then the specified behavior becomes much, much more strict. And as such not usable for me any more as I want to sign-up process to be simple and painless for my users.

Reply to this email directly or view it on GitHub.

@Download
Copy link
Author

Yes, loose mode is available. However, strict is not behaving as I would expect, which is why I brought it up. If I look at the docs, the behavior described there makes sense to me and is what I would expect. So I think it's a bug in the implementation of strict mode.

@danpalmer
Copy link
Owner

Hey everyone. Thanks @Download for opening the issue.

What I think you are saying is that a password would be rejected if it contains a part of a banned password (so pwd contains substring of banned)

This is correct. It is the expected behaviour of "strict" mode. This was intended to prevent users using passwords like "password123", even if it didn't specifically appear in the banned password list.

What I read in the docs is that if the password is a substring of a banned password it would be rejected:

This is a bug in the documentation. I will try and get a fix out for it soon.

@RajUmadas
Copy link

I believe the way @Download interprets the docs is correct, and the functionality should match. He is entirely correct that banning shorter passwords would make very strong passwords fail. I have used this tool set before in a loose fashion and not run into this issue. I am going to start a roll out with strict on, and will edit the complexity measure to function the way @Download suggests.

@Download
Copy link
Author

@RajUmadas

I agree. @danpalmer is 'the boss' so it's his call. But I strongly feel that the docs are right and the implementation wrong.

This was intended to prevent users using passwords like "password123", even if it didn't specifically appear in the banned password list.

I understand why this would be desirable and if you write it down like this it seems to make sense. However, you did not address my concern:

having this banlist:

['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z']

Would make it impossible to create any password with any lowercase letter in it....

This cannot be the intended behavior right?? However it is what will happen if you try to prevent users from adding extra characters to banned words, you will ban all combinations containing a banned sequence. Hence, using short sequences in the banlist makes it very strict for users.
But short words are exactly the words I want to ban!

I feel the banlist is especially useful to guard against short, unsafe sequences.

This password is relatively safe, even though it is very short, because it uses characters from many different character groups:

s$€1_~!;

This password however, is very unsafe:

password

Not only does it hardly use any different character groups, it's also a common word. So yes we want to ban this.

However, make the passwords longer and the importance of using different character groups fades. It's actually a very valid strategy to use long passwords that are combinations of common words. They are more easy to remember and are generally stronger than short passwords, even if they don't contain characters from many different character groups. Please have a look at this cartoon which explains it perfectly well imo: https://xkcd.com/936/

Personally I use the shorter, complex passwords... Because I am lazy and don't want to type much. But that's a personal choice. Others may very well use the long ones. I don't want my software to force that choice on users. That's why I allow for very long passwords as well as short ones. Also I don't force the user to use e.g. numerals or punctuation marks. I do however demand from them that the password has a minimal complexity. Hate punctuation? Use a long password. Hate typing? Use numerals and punctuation symbols. The choice is yours.

However, the complexity algorithm fails to account for common words. It ranks

awdfsyuf

as being equally complex as

password

Which to us as humans is not correct. This is where the banlist comes in. It allows us to make sure that short passwords are not too easy, by preventing common words, ideally, we would simply ban all common words shorter than, say. 10 characters.

Making it also ban substrings of banned passwords helps in this respect. Preventing pass because it's a substring from password makes perfect sense.

However, the banlist as it currently is (in strict mode) prevents the use of long passwords that are common word combinations. Even though these are not only perfectly safe (often safer than cryptic short passwords) but actually recommended by many.

If you insist on changing the spec to match the code (a mistake imho), then at least put some bounds on it. So that if the password length (or complexity perhaps?) becomes greater than some threshold, it stops banning. The current behavior just feels so wrong! Please have a look at my example passwords which are outrageously long and complex but will still be banned...

@Download
Copy link
Author

Sorry for the long rant :) but I still have something to add.

For me the rule should be this:

Making your password longer, makes it stronger

The current behavior of strict mode breaks this rule. It regards

thisismypas

as stronger than

thisismypass

even though it's shorter. It means a user will see the bar indicating his password strength going up with each keystroke as he types this, only to suddenly fall to 0 as he types the last 's'. That feels so wrong!

Please reconsider Dan.

@RajUmadas
Copy link

I ended up using https://github.com/dropbox/zxcvbn

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

No branches or pull requests

4 participants