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

block extremely simple and common passwords like "12345678" on a registration #2285

Closed
matkoniecz opened this issue Jun 29, 2019 · 8 comments

Comments

@matkoniecz
Copy link
Contributor

Currently registration form requires 8 characters (what is good) and has no massive set of requirements (minimum X of ABC, Y of FGH etc) what is also great.

But it would be useful to blacklist some of the most common and weakest passwords.

Example set of the most common passwords from https://xato.net/today-i-am-releasing-ten-million-passwords-b6278bbe7495

password x19580
12345678 x13582
123456789 x11696
baseball x3565
football x3494
qwertyuiop x2860
1234567890 x2797
superman x2540
1qaz2wsx x2531
trustno1 x2213
jennifer x2155
sunshine x1901
iloveyou x1893
starwars x1718
computer x1688
michelle x1677
11111111 x1575
princess x1483
987654321 x1349
corvette x1327
1234qwer x1276
88888888 x1243
q1w2e3r4t5 x1201
internet x1196
samantha x1149
whatever x1139
maverick x1116
steelers x1058
mercedes x1050
123123123 x1016

(obviously in blocking a bit longer list may be used)

passwords = {}
File.foreach("10-million-combos.txt").each do |line|
  begin
    password = line.split[1]    
  rescue ArgumentError
    next
  end
  next if password.nil?
  next if password.length < 8

  passwords[password] ||= 0
  passwords[password] += 1
end

passwords.to_a.sort_by { |entry| -entry[1] }.each do |entry|
  password = entry[0]
  count = entry[1]
  next if count <= 1000

  puts "#{password} x#{count}"
end
@tomhughes
Copy link
Member

Once you start down that road where do you end? A more sensible idea would be to add a password strength meter rather than blacklisting some arbitrary set of passwords.

@rugk
Copy link
Contributor

rugk commented Jul 5, 2019

Nope, it's not. NIST official password guidelines now recommend blocking these very simple password. Actually, in practice you often just use the list/service of https://haveibeenpwned.com/.

@woodpeck
Copy link
Contributor

woodpeck commented Jul 5, 2019

What is the threat we wish to protect ourself or our users against here?

@matkoniecz
Copy link
Contributor Author

matkoniecz commented Jul 5, 2019

My intention was to help users in choosing passwords of acceptable quality (not password or 12345678) what in turn should reduce risk of someone taking over their account.

What prompted me to create this issue was that I was making a test account (related to for example openstreetmap/operations#311 ) and was really surprized that system accepted password as password.

I am not expert on this topic, so maybe implementing this is not worth the effort.

@woodpeck
Copy link
Contributor

woodpeck commented Jul 5, 2019

Why would someone take over someone else's OSM account? I can delete all your edits with an account I create afresh, why would I want to take over yours?

@matkoniecz
Copy link
Contributor Author

matkoniecz commented Jul 5, 2019

Deleting/editing diary entries, editing/blanking posts on a forum.openstreetmap.org or other linked services, impersonation, taking over a nice username or account with high edit count. Maybe harassing specific user.

@gravitystorm
Copy link
Collaborator

Why would someone take over someone else's OSM account? I can delete all your edits with an account I create afresh, why would I want to take over yours?

I've no interest in taking over @matkoniecz user account. But as for yours, @woodpeck - well, moderator privileges make a juicer target! Even more so for an admin account. We can't currently make any account-security checks before handing out elevated privileges, and there's a bunch of stuff which is hard to undo if a moderator or admin account with weak access gets hacked. Even a normal account has who-knows-what in the private messaging system, and "well password complexity is entirely up to the user to worry about" isn't something I want to hear.

So I'm supportive of this suggestion. But I would strongly suggest that it waits until we move our account signup process over to Devise. Implementation would be best then as a devise-compatible extension, or using existing extensions like https://github.com/devise-security/devise-security

@tomhughes
Copy link
Member

As I said I see no problem with installing some sort of general purpose password validator like that but I don't think we should be in the business of managing a hard coded block list or of deciding what is or isn't good enough.

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

5 participants