Commit 9964c43b authored by Jean-Philippe Lang's avatar Jean-Philippe Lang Committed by Eric Davis

Adds random salt to user passwords (#7410).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@4936 e93f8b46-1217-0410-a6f0-8f06a7374b81
parent c224ec51
......@@ -81,7 +81,9 @@ class User < Principal
def before_save
# update hashed_password if password was set
self.hashed_password = User.hash_password(self.password) if self.password && self.auth_source_id.blank?
if self.password && self.auth_source_id.blank?
salt_password(password)
end
end
def reload(*args)
......@@ -119,7 +121,7 @@ class User < Principal
return nil unless user.auth_source.authenticate(login, password)
else
# authentication with local password
return nil unless User.hash_password(password) == user.hashed_password
return nil unless user.check_password?(password)
end
else
# user is not yet registered, try to authenticate with available sources
......@@ -198,13 +200,21 @@ class User < Principal
update_attribute(:status, STATUS_LOCKED)
end
# Returns true if +clear_password+ is the correct user's password, otherwise false
def check_password?(clear_password)
if auth_source_id.present?
auth_source.authenticate(self.login, clear_password)
else
User.hash_password(clear_password) == self.hashed_password
User.hash_password("#{salt}#{User.hash_password clear_password}") == hashed_password
end
end
# Generates a random salt and computes hashed_password for +clear_password+
# The hashed password is stored in the following form: SHA1(salt + SHA1(password))
def salt_password(clear_password)
self.salt = User.generate_salt
self.hashed_password = User.hash_password("#{salt}#{User.hash_password clear_password}")
end
# Does the backend storage allow this user to change their password?
def change_password_allowed?
......@@ -470,6 +480,20 @@ class User < Principal
end
anonymous_user
end
# Salts all existing unsalted passwords
# It changes password storage scheme from SHA1(password) to SHA1(salt + SHA1(password))
# This method is used in the SaltPasswords migration and is to be kept as is
def self.salt_unsalted_passwords!
transaction do
User.find_each(:conditions => "salt IS NULL OR salt = ''") do |user|
next if user.hashed_password.blank?
salt = User.generate_salt
hashed_password = User.hash_password("#{salt}#{user.hashed_password}")
User.update_all("salt = '#{salt}', hashed_password = '#{hashed_password}'", ["id = ?", user.id] )
end
end
end
protected
......@@ -486,6 +510,12 @@ class User < Principal
def self.hash_password(clear_password)
Digest::SHA1.hexdigest(clear_password || "")
end
# Returns a 128bits random salt as a hex string (32 chars long)
def self.generate_salt
ActiveSupport::SecureRandom.hex(16)
end
end
class AnonymousUser < User
......
class AddUsersSalt < ActiveRecord::Migration
def self.up
add_column :users, :salt, :string, :limit => 64
end
def self.down
remove_column :users, :salt
end
end
class SaltUserPasswords < ActiveRecord::Migration
def self.up
say_with_time "Salting user passwords, this may take some time..." do
User.salt_unsalted_passwords!
end
end
def self.down
# Unsalted passwords can not be restored
raise ActiveRecord::IrreversibleMigration, "Can't decypher salted passwords. This migration can not be rollback'ed."
end
end
......@@ -231,7 +231,7 @@ sub RedmineDSN {
my ($self, $parms, $arg) = @_;
$self->{RedmineDSN} = $arg;
my $query = "SELECT
hashed_password, auth_source_id, permissions
hashed_password, salt, auth_source_id, permissions
FROM members, projects, users, roles, member_roles
WHERE
projects.id=members.project_id
......@@ -426,11 +426,12 @@ sub is_member {
$sth->execute($redmine_user, $project_id);
my $ret;
while (my ($hashed_password, $auth_source_id, $permissions) = $sth->fetchrow_array) {
while (my ($hashed_password, $salt, $auth_source_id, $permissions) = $sth->fetchrow_array) {
unless ($auth_source_id) {
my $method = $r->method;
if ($hashed_password eq $pass_digest && ((request_is_read_only($r) && $permissions =~ /:browse_repository/) || $permissions =~ /:commit_access/) ) {
my $method = $r->method;
my $salted_password = Digest::SHA1::sha1_hex($salt.$pass_digest);
if ($hashed_password eq $salted_password && ((request_is_read_only($r) && $permissions =~ /:browse_repository/) || $permissions =~ /:commit_access/) ) {
$ret = 1;
last;
}
......
......@@ -4,7 +4,9 @@ users_004:
status: 1
last_login_on:
language: en
hashed_password: 4e4aeb7baaf0706bd670263fef42dad15763b608
# password = foo
salt: 3126f764c3c5ac61cbfc103f25f934cf
hashed_password: 9e4dd7eeb172c12a0691a6d9d3a269f7e9fe671b
updated_on: 2006-07-19 19:34:07 +02:00
admin: false
mail: rhill@somenet.foo
......@@ -20,7 +22,9 @@ users_001:
status: 1
last_login_on: 2006-07-19 22:57:52 +02:00
language: en
hashed_password: d033e22ae348aeb5660fc2140aec35850c4da997
# password = admin
salt: 82090c953c4a0000a7db253b0691a6b4
hashed_password: b5b6ff9543bf1387374cdfa27a54c96d236a7150
updated_on: 2006-07-19 22:57:52 +02:00
admin: true
mail: admin@somenet.foo
......@@ -36,7 +40,9 @@ users_002:
status: 1
last_login_on: 2006-07-19 22:42:15 +02:00
language: en
hashed_password: a9a653d4151fa2c081ba1ffc2c2726f3b80b7d7d
# password = jsmith
salt: 67eb4732624d5a7753dcea7ce0bb7d7d
hashed_password: bfbe06043353a677d0215b26a5800d128d5413bc
updated_on: 2006-07-19 22:42:15 +02:00
admin: false
mail: jsmith@somenet.foo
......@@ -52,7 +58,9 @@ users_003:
status: 1
last_login_on:
language: en
hashed_password: 7feb7657aa7a7bf5aef3414a5084875f27192415
# password = foo
salt: 7599f9963ec07b5a3b55b354407120c0
hashed_password: 8f659c8d7c072f189374edacfa90d6abbc26d8ed
updated_on: 2006-07-19 19:33:19 +02:00
admin: false
mail: dlopper@somenet.foo
......@@ -70,7 +78,7 @@ users_005:
status: 3
last_login_on:
language: en
hashed_password: 7feb7657aa7a7bf5aef3414a5084875f27192415
hashed_password: 1
updated_on: 2006-07-19 19:33:19 +02:00
admin: false
mail: dlopper2@somenet.foo
......
......@@ -157,7 +157,6 @@ class UserTest < ActiveSupport::TestCase
user = User.try_to_login("admin", "hello")
assert_kind_of User, user
assert_equal "admin", user.login
assert_equal User.hash_password("hello"), user.hashed_password
end
def test_name_format
......@@ -179,6 +178,22 @@ class UserTest < ActiveSupport::TestCase
assert_equal nil, user
end
context ".try_to_login" do
context "with good credentials" do
should "return the user" do
user = User.try_to_login("admin", "admin")
assert_kind_of User, user
assert_equal "admin", user.login
end
end
context "with wrong credentials" do
should "return nil" do
assert_nil User.try_to_login("admin", "foo")
end
end
end
if ldap_configured?
context "#try_to_login using LDAP" do
context "with failed connection to the LDAP server" do
......@@ -523,6 +538,23 @@ class UserTest < ActiveSupport::TestCase
should 'be added and tested'
end
end
def test_salt_unsalted_passwords
# Restore a user with an unsalted password
user = User.find(1)
user.salt = nil
user.hashed_password = User.hash_password("unsalted")
user.save!
User.salt_unsalted_passwords!
user.reload
# Salt added
assert !user.salt.blank?
# Password still valid
assert user.check_password?("unsalted")
assert_equal user, User.try_to_login(user.login, "unsalted")
end
if Object.const_defined?(:OpenID)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment