From 9becc41df989bfccff091852d45925d41f0a13d8 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Mon, 29 Nov 2021 15:57:05 -0600 Subject: [PATCH] Add authenticate_by when using has_secure_password This method is intended to replace code like the following, which returns early when a user with a matching email is not found: ```ruby User.find_by(email: "...")&.authenticate("...") ``` Such code is vulnerable to timing-based enumeration attacks, wherein an attacker can determine if a user account with a given email exists. After confirming that an account exists, the attacker can try passwords associated with that email address from other leaked databases, in case the user re-used a password across multiple sites (a common practice). Additionally, knowing an account email address allows the attacker to attempt a targeted phishing ("spear phishing") attack. `authenticate_by` addresses the vulnerability by taking the same amount of time regardless of whether a user with a matching email is found. --- activerecord/CHANGELOG.md | 26 +++++++ activerecord/lib/active_record.rb | 1 + activerecord/lib/active_record/base.rb | 2 +- .../lib/active_record/secure_password.rb | 54 +++++++++++++++ .../test/cases/secure_password_test.rb | 67 +++++++++++++++++++ activerecord/test/models/user.rb | 3 + activerecord/test/schema/schema.rb | 2 + 7 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 activerecord/lib/active_record/secure_password.rb create mode 100644 activerecord/test/cases/secure_password_test.rb diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 87ba34e21a..386b7189e7 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,29 @@ +* Add `authenticate_by` when using `has_secure_password`. + + `authenticate_by` is intended to replace code like the following, which + returns early when a user with a matching email is not found: + + ```ruby + User.find_by(email: "...")&.authenticate("...") + ``` + + Such code is vulnerable to timing-based enumeration attacks, wherein an + attacker can determine if a user account with a given email exists. After + confirming that an account exists, the attacker can try passwords associated + with that email address from other leaked databases, in case the user + re-used a password across multiple sites (a common practice). Additionally, + knowing an account email address allows the attacker to attempt a targeted + phishing ("spear phishing") attack. + + `authenticate_by` addresses the vulnerability by taking the same amount of + time regardless of whether a user with a matching email is found: + + ```ruby + User.authenticate_by(email: "...", password: "...") + ``` + + *Jonathan Hefner* + * Remove deprecated `ActiveRecord::DatabaseConfigurations::DatabaseConfig#spec_name`. *Rafael Mendonça França* diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index 10582eb71b..82e0043ad2 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -68,6 +68,7 @@ module ActiveRecord autoload :SchemaDumper autoload :SchemaMigration autoload :Scoping + autoload :SecurePassword autoload :SecureToken autoload :Serialization autoload :SignedId diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 0b2f6f0890..460b0875a1 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -315,7 +315,7 @@ module ActiveRecord # :nodoc: include Callbacks include Timestamp include Associations - include ActiveModel::SecurePassword + include SecurePassword include AutosaveAssociation include NestedAttributes include Transactions diff --git a/activerecord/lib/active_record/secure_password.rb b/activerecord/lib/active_record/secure_password.rb new file mode 100644 index 0000000000..cb857e8ead --- /dev/null +++ b/activerecord/lib/active_record/secure_password.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require "active_support/core_ext/hash/except" + +module ActiveRecord + module SecurePassword + extend ActiveSupport::Concern + + include ActiveModel::SecurePassword + + module ClassMethods + # Given a set of attributes, finds a record using the non-password + # attributes, and then authenticates that record using the password + # attributes. Returns the record if authentication succeeds; otherwise, + # returns +nil+. + # + # Regardless of whether a record is found or authentication succeeds, + # +authenticate_by+ will take the same amount of time. This prevents + # timing-based enumeration attacks, wherein an attacker can determine if a + # passworded record exists even without knowing the password. + # + # Raises an ArgumentError if the set of attributes doesn't contain at + # least one password and one non-password attribute. + # + # ==== Examples + # + # class User < ActiveRecord::Base + # has_secure_password + # end + # + # User.create(name: "John Doe", email: "jdoe@example.com", password: "abc123") + # + # User.authenticate_by(email: "jdoe@example.com", password: "abc123").name # => "John Doe" (in 373.4ms) + # User.authenticate_by(email: "jdoe@example.com", password: "wrong") # => nil (in 373.9ms) + # User.authenticate_by(email: "wrong@example.com", password: "abc123") # => nil (in 373.6ms) + # + # User.authenticate_by(email: "jdoe@example.com") # => ArgumentError + # User.authenticate_by(password: "abc123") # => ArgumentError + def authenticate_by(attributes) + passwords = attributes.select { |name, value| !has_attribute?(name) && has_attribute?("#{name}_digest") } + + raise ArgumentError, "One or more password arguments are required" if passwords.empty? + raise ArgumentError, "One or more finder arguments are required" if passwords.size == attributes.size + + if record = find_by(attributes.except(*passwords.keys)) + record if passwords.count { |name, value| record.public_send(:"authenticate_#{name}", value) } == passwords.size + else + self.new(passwords) + nil + end + end + end + end +end diff --git a/activerecord/test/cases/secure_password_test.rb b/activerecord/test/cases/secure_password_test.rb new file mode 100644 index 0000000000..db08eea533 --- /dev/null +++ b/activerecord/test/cases/secure_password_test.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require "cases/helper" +require "models/user" + +class SecurePasswordTest < ActiveRecord::TestCase + setup do + # Speed up tests + @original_min_cost = ActiveModel::SecurePassword.min_cost + ActiveModel::SecurePassword.min_cost = true + + @user = User.create(password: "abc123", recovery_password: "123abc") + end + + teardown do + ActiveModel::SecurePassword.min_cost = @original_min_cost + end + + test "authenticate_by authenticates when password is correct" do + assert_equal @user, User.authenticate_by(token: @user.token, password: @user.password) + end + + test "authenticate_by does not authenticate when password is incorrect" do + assert_nil User.authenticate_by(token: @user.token, password: "wrong") + end + + test "authenticate_by takes the same amount of time regardless of whether record is found" do + # Benchmark.realtime returns fractional seconds. Thus, summing over 1000 + # iterations is equivalent to averaging over 1000 iterations and then + # multiplying by 1000 to convert to milliseconds. + found_average_time_in_ms = 1000.times.sum do + Benchmark.realtime do + User.authenticate_by(token: @user.token, password: @user.password) + end + end + + not_found_average_time_in_ms = 1000.times.sum do + Benchmark.realtime do + User.authenticate_by(token: "wrong", password: @user.password) + end + end + + assert_in_delta found_average_time_in_ms, not_found_average_time_in_ms, 0.5 + end + + test "authenticate_by finds record using multiple attributes" do + assert_equal @user, User.authenticate_by(token: @user.token, auth_token: @user.auth_token, password: @user.password) + assert_nil User.authenticate_by(token: @user.token, auth_token: "wrong", password: @user.password) + end + + test "authenticate_by authenticates using multiple passwords" do + assert_equal @user, User.authenticate_by(token: @user.token, password: @user.password, recovery_password: @user.recovery_password) + assert_nil User.authenticate_by(token: @user.token, password: @user.password, recovery_password: "wrong") + end + + test "authenticate_by requires at least one password" do + assert_raises ArgumentError do + User.authenticate_by(token: @user.token) + end + end + + test "authenticate_by requires at least one attribute" do + assert_raises ArgumentError do + User.authenticate_by(password: @user.password) + end + end +end diff --git a/activerecord/test/models/user.rb b/activerecord/test/models/user.rb index a1eb844964..a2f90c6968 100644 --- a/activerecord/test/models/user.rb +++ b/activerecord/test/models/user.rb @@ -3,6 +3,9 @@ require "models/job" class User < ActiveRecord::Base + has_secure_password validations: false + has_secure_password :recovery_password, validations: false + has_secure_token has_secure_token :auth_token, length: 36 diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index c564ea5d82..471d89a3f8 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -1274,6 +1274,8 @@ ActiveRecord::Schema.define do create_table :users, force: true do |t| t.string :token t.string :auth_token + t.string :password_digest + t.string :recovery_password_digest end create_table :test_with_keyword_column_name, force: true do |t|