diff --git a/lib/devise.rb b/lib/devise.rb index 7b93ee0c..028eb9a8 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -374,6 +374,17 @@ module Devise def self.friendly_token ActiveSupport::SecureRandom.base64(15).tr('+/=', 'xyz') end + + # constant-time comparison algorithm to prevent timing attacks + def self.secure_compare(a, b) + return false unless a.present? && b.present? + return false unless a.bytesize == b.bytesize + l = a.unpack "C#{a.bytesize}" + + res = 0 + b.each_byte { |byte| res |= byte ^ l.shift } + res == 0 + end end require 'warden' diff --git a/lib/devise/models/database_authenticatable.rb b/lib/devise/models/database_authenticatable.rb index e960f529..23f720f1 100644 --- a/lib/devise/models/database_authenticatable.rb +++ b/lib/devise/models/database_authenticatable.rb @@ -31,9 +31,11 @@ module Devise self.encrypted_password = password_digest(@password) if @password.present? end - # Verifies whether an incoming_password (ie from sign in) is the user password. + # Verifies whether an password (ie from sign in) is the user password. def valid_password?(password) - ::BCrypt::Password.new(self.encrypted_password) == "#{password}#{self.class.pepper}" + bcrypt = ::BCrypt::Password.new(self.encrypted_password) + password = ::BCrypt::Engine.hash_secret("#{password}#{self.class.pepper}", bcrypt.salt) + Devise.secure_compare(password, self.encrypted_password) end # Set password and password confirmation to nil diff --git a/lib/devise/models/encryptable.rb b/lib/devise/models/encryptable.rb index 89eff9a7..3913d168 100644 --- a/lib/devise/models/encryptable.rb +++ b/lib/devise/models/encryptable.rb @@ -36,7 +36,7 @@ module Devise # Verifies whether an incoming_password (ie from sign in) is the user password. def valid_password?(incoming_password) - password_digest(incoming_password) == self.encrypted_password + Devise.secure_compare(password_digest(incoming_password), self.encrypted_password) end protected diff --git a/test/controllers/helpers_test.rb b/test/controllers/helpers_test.rb index c5ef7ba4..c28a4938 100644 --- a/test/controllers/helpers_test.rb +++ b/test/controllers/helpers_test.rb @@ -136,11 +136,13 @@ class ControllerAuthenticatableTest < ActionController::TestCase end test 'sign out without args proxy to sign out all scopes' do + @mock_warden.expects(:user).times(Devise.mappings.size) @mock_warden.expects(:logout).with().returns(true) @controller.sign_out end test 'sign out everybody proxy to logout on warden' do + @mock_warden.expects(:user).times(Devise.mappings.size) @mock_warden.expects(:logout).with().returns(true) @controller.sign_out_all_scopes end @@ -224,6 +226,7 @@ class ControllerAuthenticatableTest < ActionController::TestCase test 'sign out and redirect uses the configured after sign out path when signing out all scopes' do swap Devise, :sign_out_all_scopes => true do + @mock_warden.expects(:user).times(Devise.mappings.size) @mock_warden.expects(:logout).with().returns(true) @controller.expects(:redirect_to).with(admin_root_path) @controller.instance_eval "def after_sign_out_path_for(resource); admin_root_path; end"