Adds random salt to user passwords (#7410).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@4936 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/models/user.rb

@@ -83,7 +83,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)
@@ -121,7 +123,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
@@ -200,14 +202,22 @@ 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?
     return true if auth_source_id.blank?
@@ -474,6 +484,20 @@ class User < Principal
     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
   
   def validate
@@ -514,6 +538,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

db/migrate/20110223180944_add_users_salt.rb

@@ -0,0 +1,9 @@
+class AddUsersSalt < ActiveRecord::Migration
+  def self.up
+    add_column :users, :salt, :string, :limit => 64
+  end
+
+  def self.down
+    remove_column :users, :salt
+  end
+end

db/migrate/20110223180953_salt_user_passwords.rb

@@ -0,0 +1,13 @@
+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

extra/svn/Redmine.pm

@@ -148,7 +148,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
@@ -316,11 +316,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 && ((defined $read_only_methods{$method} && $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 && ((defined $read_only_methods{$method} && $permissions =~ /:browse_repository/) || $permissions =~ /:commit_access/) ) {
               $ret = 1;
               last;
           }

test/fixtures/users.yml

@@ -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

test/unit/user_test.rb

@@ -361,7 +361,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
@@ -383,6 +382,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
@@ -728,6 +743,23 @@ class UserTest < ActiveSupport::TestCase
     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)
     
   def test_setting_identity_url