openstreetmap / openstreetmap-website

The Rails application that powers OpenStreetMap
https://www.openstreetmap.org/
GNU General Public License v2.0
2.2k stars 912 forks source link

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

Closed matkoniecz closed 5 years ago

matkoniecz commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

matkoniecz commented 5 years ago

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 https://github.com/openstreetmap/operations/issues/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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.