From 9dc788c0cec263a3000d3cd44eb0e8bb49163f40 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 27 Dec 2020 22:47:52 -0600 Subject: [PATCH] users: improve sockpuppet detection on signup. Require new accounts to verify their email address if any of the following conditions are true: * Their IP is a proxy. * Their IP is under a partial IP ban. * They're creating a new account while logged in to another account. * Somebody recently created an account from the same IP in the last week. Changes from before: * Allow logged in users to view the signup page and create new accounts. Creating a new account while logged in to your old account is now allowed, but it requires email verification. This is a honeypot. * Creating multiple accounts from the same IP is now allowed, but they require email verification. Previously the same IP check was only for the last day (now it's the last week), and only for an exact IP match (now it's a subnet match, /24 for IPv4 or /64 for IPv6). * New account verification is disabled for private IPs (e.g. 127.0.0.1, 192.168.0.1), to make development or running personal boorus easier (fixes #4618). --- app/controllers/users_controller.rb | 2 +- app/logical/danbooru/http.rb | 1 + app/logical/user_verifier.rb | 46 +++++++++++ app/policies/user_policy.rb | 10 +-- test/functional/users_controller_test.rb | 98 +++++++++++++++--------- 5 files changed, 115 insertions(+), 42 deletions(-) create mode 100644 app/logical/user_verifier.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index eb0e2f7b4..f28cd9390 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -59,7 +59,7 @@ class UsersController < ApplicationController end def create - requires_verification = IpLookup.new(CurrentUser.ip_addr).is_proxy? || IpBan.hit!(:partial, CurrentUser.ip_addr) + requires_verification = UserVerifier.new(CurrentUser.user, request).requires_verification? @user = authorize User.new( last_ip_addr: CurrentUser.ip_addr, diff --git a/app/logical/danbooru/http.rb b/app/logical/danbooru/http.rb index e1b76b16b..bbf30cafd 100644 --- a/app/logical/danbooru/http.rb +++ b/app/logical/danbooru/http.rb @@ -1,3 +1,4 @@ +require "danbooru/http/application_client" require "danbooru/http/html_adapter" require "danbooru/http/xml_adapter" require "danbooru/http/cache" diff --git a/app/logical/user_verifier.rb b/app/logical/user_verifier.rb new file mode 100644 index 000000000..e225924ae --- /dev/null +++ b/app/logical/user_verifier.rb @@ -0,0 +1,46 @@ +# Checks whether a new account seems suspicious and should require email verification. + +class UserVerifier + attr_reader :current_user, :request + + # current_user is the user creating the new account, not the new account itself. + def initialize(current_user, request) + @current_user, @request = current_user, request + end + + def requires_verification? + return false if is_local_ip? + + # we check for IP bans first to make sure we bump the IP ban hit count + is_ip_banned? || is_logged_in? || is_recent_signup? || is_proxy? + end + + private + + def ip_address + @ip_address ||= IPAddress.parse(request.remote_ip) + end + + def is_local_ip? + ip_address.loopback? || ip_address.link_local? || ip_address.private? || ip_address.try(:unique_local?) + end + + def is_logged_in? + !current_user.is_anonymous? + end + + def is_recent_signup?(age: 24.hours) + subnet_len = ip_address.ipv4? ? 24 : 64 + subnet = "#{ip_address}/#{subnet_len}" + + User.where("last_ip_addr <<= ?", subnet).where("created_at > ?", age.ago).exists? + end + + def is_ip_banned? + IpBan.hit!(:partial, ip_address.to_s) + end + + def is_proxy? + IpLookup.new(ip_address).is_proxy? + end +end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index fa1dff0f4..1aaaaa84f 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -1,6 +1,10 @@ class UserPolicy < ApplicationPolicy def create? - user.is_anonymous? && !sockpuppet? + true + end + + def new? + true end def update? @@ -31,10 +35,6 @@ class UserPolicy < ApplicationPolicy user.is_admin? || record.id == user.id || !record.enable_private_favorites? end - def sockpuppet? - User.where(last_ip_addr: request.remote_ip).where("created_at > ?", 1.day.ago).exists? - end - def permitted_attributes_for_create [:name, :password, :password_confirmation, { email_address_attributes: [:address] }] end diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index fe4353683..38216a1a6 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -243,6 +243,11 @@ class UsersControllerTest < ActionDispatch::IntegrationTest get new_user_path assert_response :success end + + should "render for a logged in user" do + get_auth new_user_path, @user + assert_response :success + end end context "create action" do @@ -256,11 +261,6 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_no_enqueued_emails end - should "not allow logged in users to create a new account" do - post_auth users_path, @user, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} - assert_response 403 - end - should "create a user with a valid email" do post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email: "webmaster@danbooru.donmai.us" }} @@ -289,45 +289,71 @@ class UsersControllerTest < ActionDispatch::IntegrationTest end end - should "mark users signing up from proxies as requiring verification" do - skip unless IpLookup.enabled? + context "sockpuppet detection" do + setup do + @private_ip = "192.168.0.1" + @valid_ip = "187.37.226.17" # a random valid, non-proxy public IP + @proxy_ip = "51.15.128.1" + end - self.remote_addr = "51.15.128.1" - post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + should "mark accounts created by already logged in users as requiring verification" do + self.remote_addr = @valid_ip - assert_redirected_to User.last - assert_equal(true, User.last.requires_verification) - end + post_auth users_path, @user, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} - should "mark users signing up from a partial banned IP as requiring verification" do - skip unless IpLookup.enabled? - self.remote_addr = "187.37.226.17" + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + end - @ip_ban = create(:ip_ban, ip_addr: self.remote_addr, category: :partial) - post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + should "mark users signing up from proxies as requiring verification" do + skip unless IpLookup.enabled? + self.remote_addr = @proxy_ip - assert_redirected_to User.last - assert_equal(true, User.last.requires_verification) - assert_equal(1, @ip_ban.reload.hit_count) - assert(@ip_ban.last_hit_at > 1.minute.ago) - end + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} - should "not mark users signing up from non-proxies as requiring verification" do - skip unless IpLookup.enabled? - self.remote_addr = "187.37.226.17" - post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + end - assert_redirected_to User.last - assert_equal(false, User.last.requires_verification) - end + should "mark users signing up from a partial banned IP as requiring verification" do + self.remote_addr = @valid_ip - context "with sockpuppet validation enabled" do - should "not allow registering multiple accounts with the same IP" do - assert_difference("User.count", 0) do - @user.update(last_ip_addr: "127.0.0.1") - post users_path, params: {:user => {:name => "dupe", :password => "xxxxx1", :password_confirmation => "xxxxx1"}} - assert_response 403 - end + @ip_ban = create(:ip_ban, ip_addr: self.remote_addr, category: :partial) + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + assert_equal(1, @ip_ban.reload.hit_count) + assert(@ip_ban.last_hit_at > 1.minute.ago) + end + + should "not mark users signing up from non-proxies as requiring verification" do + skip unless IpLookup.enabled? + self.remote_addr = @valid_ip + + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(false, User.last.requires_verification) + end + + should "mark accounts registered from an IPv4 address recently used for another account as requiring verification" do + @user.update!(last_ip_addr: @valid_ip) + self.remote_addr = @valid_ip + + post users_path, params: { user: { name: "dupe", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(true, User.last.requires_verification) + end + + should "not mark users signing up from localhost as requiring verification" do + self.remote_addr = "127.0.0.1" + + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} + + assert_redirected_to User.last + assert_equal(false, User.last.requires_verification) end end end