Skip to content

Commit

Permalink
Use secure compare as well.
Browse files Browse the repository at this point in the history
  • Loading branch information
josevalim committed Feb 15, 2011
1 parent ae75186 commit dd721f1
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 3 deletions.
11 changes: 11 additions & 0 deletions lib/devise.rb
Expand Up @@ -374,6 +374,17 @@ def self.configure_warden! #:nodoc:
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 }

This comment has been minimized.

Copy link
@shoenig

shoenig Dec 18, 2013

In what way is this "constant-time"? It loops over every byte, and it doesn't even return as soon as there is a mismatch.

This comment has been minimized.

Copy link
@josevalim

josevalim Dec 18, 2013

Author Contributor

@shoenig it is constant time in the sense the average case is the worst case, so timing attacks are not possible. It is not constant time in the O(1) sense.

res == 0
end
end

require 'warden'
Expand Down
6 changes: 4 additions & 2 deletions lib/devise/models/database_authenticatable.rb
Expand Up @@ -31,9 +31,11 @@ def password=(new_password)
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
Expand Down
2 changes: 1 addition & 1 deletion lib/devise/models/encryptable.rb
Expand Up @@ -36,7 +36,7 @@ def authenticatable_salt

# 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
Expand Down
3 changes: 3 additions & 0 deletions test/controllers/helpers_test.rb
Expand Up @@ -136,11 +136,13 @@ def setup
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
Expand Down Expand Up @@ -224,6 +226,7 @@ def setup

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"
Expand Down

0 comments on commit dd721f1

Please sign in to comment.