Fixed issue #462 - Password hashing: upgrade to argon2 2.x (argon2id), fix non-unique salt bug - huge thanks to @martinvonwittich !

Gemfile

@@ -29,7 +29,7 @@ gem 'em-websocket'
 gem 'eventmachine'
 
 # core - password security
-gem 'argon2', '1.1.5'
+gem 'argon2'
 
 # core - state machine
 gem 'aasm'

Gemfile.lock

@@ -101,9 +101,9 @@ GEM
     addressable (2.5.2)
       public_suffix (>= 2.0.2, < 4.0)
     arel (8.0.0)
-    argon2 (1.1.5)
+    argon2 (2.0.2)
       ffi (~> 1.9)
-      ffi-compiler (~> 0.1)
+      ffi-compiler (>= 0.1)
     ast (2.4.0)
     autoprefixer-rails (9.5.1)
       execjs
@@ -191,8 +191,8 @@ GEM
       multipart-post (>= 1.2, < 3)
     faraday-http-cache (2.0.0)
       faraday (~> 0.8)
-    ffi (1.10.0)
+    ffi (1.11.1)
-    ffi-compiler (0.1.3)
+    ffi-compiler (1.0.1)
       ffi (>= 1.0.0)
       rake
     formatador (0.2.5)
@@ -545,7 +545,7 @@ DEPENDENCIES
   activerecord-nulldb-adapter
   activerecord-session_store
   acts_as_list
-  argon2 (= 1.1.5)
+  argon2
   autodiscover!
   autoprefixer-rails
   biz

app/models/user.rb

@@ -1236,25 +1236,22 @@ raise 'Minimum one user need to have admin permissions'
   end
 
   def ensure_password
-    return true if password_empty?
+    self.password = ensured_password
-    return true if PasswordHash.crypted?(password)
-
-    self.password = PasswordHash.crypt(password)
     true
   end
 
-  def password_empty?
+  def ensured_password
-    # set old password again if not given
+    # ensure unset password for blank values of new users
-    return if password.present?
+    return nil if new_record? && password.blank?
 
-    # skip if it's not desired to set a password (yet)
+    # don't permit empty password update for existing users
-    return true if !password
+    return password_was if password.blank?
 
-    # get current record
+    # don't re-hash an already hashed passsword
-    return if !id
+    return password if PasswordHash.crypted?(password)
 
-    self.password = password_was
+    # hash the plaintext password
-    true
+    PasswordHash.crypt(password)
   end
 
   # reset login_failed if password is changed

lib/password_hash.rb

@@ -6,7 +6,8 @@ module PasswordHash
   extend self
 
   def crypt(password)
-    argon2.create(password)
+    # take a fresh Argon2::Password instances to ensure randomized salt
+    Argon2::Password.new(secret: secret).create(password)
   end
 
   def verified?(pw_hash, password)
@@ -27,7 +28,10 @@ module PasswordHash
     return false if pw_hash.blank?
     return false if !password
 
-    sha2?(pw_hash, password)
+    return true if sha2?(pw_hash, password)
+    return true if hashed_argon2i?(pw_hash, password)
+
+    false
   end
 
   def hashed_sha2?(pw_hash)
@@ -35,8 +39,15 @@ module PasswordHash
   end
 
   def hashed_argon2?(pw_hash)
+    Argon2::Password.valid_hash?(pw_hash)
+  end
+
+  def hashed_argon2i?(pw_hash, password)
     # taken from: https://github.com/technion/ruby-argon2/blob/7e1f4a2634316e370ab84150e4f5fd91d9263713/lib/argon2.rb#L33
-    pw_hash =~ /^\$argon2i\$.{,112}/
+    return false if !pw_hash.match?(/^\$argon2i\$.{,112}/)
+
+    # Argon2::Password.verify_password verifies argon2i hashes, too
+    verified?(pw_hash, password)
   end
 
   def sha2(password)
@@ -52,10 +63,6 @@ module PasswordHash
     pw_hash == sha2(password)
   end
 
-  def argon2
-    @argon2 ||= Argon2::Password.new(secret: secret)
-  end
-
   def secret
     @secret ||= Setting.get('application_secret')
   end

spec/models/user_spec.rb

@@ -103,11 +103,13 @@ RSpec.describe User, type: :model do
 
       context 'with valid user and invalid password' do
         it 'increments failed login count' do
+          expect(described_class).to receive(:sleep).with(1)
           expect { described_class.authenticate(user.login, password.next) }
             .to change { user.reload.login_failed }.by(1)
         end
 
         it 'returns nil' do
+          expect(described_class).to receive(:sleep).with(1)
           expect(described_class.authenticate(user.login, password.next)).to be(nil)
         end
       end
@@ -137,6 +139,38 @@ RSpec.describe User, type: :model do
           expect(described_class.authenticate(user.login, '')).to be(nil)
         end
       end
+
+      context 'with empty password string when the stored password is an empty string' do
+        before { user.update_column(:password, '') }
+
+        context 'when password is an empty string' do
+          it 'returns nil' do
+            expect(described_class.authenticate(user.login, '')).to be(nil)
+          end
+        end
+
+        context 'when password is nil' do
+          it 'returns nil' do
+            expect(described_class.authenticate(user.login, nil)).to be(nil)
+          end
+        end
+      end
+
+      context 'with empty password string when the stored hash represents an empty string' do
+        before { user.update(password: PasswordHash.crypt('')) }
+
+        context 'when password is an empty string' do
+          it 'returns nil' do
+            expect(described_class.authenticate(user.login, '')).to be(nil)
+          end
+        end
+
+        context 'when password is nil' do
+          it 'returns nil' do
+            expect(described_class.authenticate(user.login, nil)).to be(nil)
+          end
+        end
+      end
     end
 
     describe '.identify' do
@@ -700,18 +734,59 @@ RSpec.describe User, type: :model do
     end
 
     describe '#password' do
+      let(:password) { Faker::Internet.password }
+
       context 'when set to plaintext password' do
         it 'hashes password before saving to DB' do
-          user.password = 'password'
+          user.password = password
 
           expect { user.save }
-            .to change(user, :password).to(PasswordHash.crypt('password'))
+            .to change { PasswordHash.crypted?(user.password) }
+        end
+      end
+
+      context 'for existing user records' do
+        context 'when changed to empty string' do
+          before { user.update(password: password) }
+
+          it 'keeps previous password' do
+
+            expect { user.update!(password: '') }
+              .not_to change(user, :password)
+          end
+        end
+
+        context 'when changed to nil' do
+          before { user.update(password: password) }
+
+          it 'keeps previous password' do
+            expect { user.update!(password: nil) }
+              .not_to change(user, :password)
+          end
+        end
+      end
+
+      context 'for new user records' do
+        context 'when passed as an empty string' do
+          let(:another_user) { create(:user, password: '') }
+
+          it 'sets password to nil' do
+            expect(another_user.password).to eq(nil)
+          end
+        end
+
+        context 'when passed as nil' do
+          let(:another_user) { create(:user, password: nil) }
+
+          it 'sets password to nil' do
+            expect(another_user.password).to eq(nil)
+          end
         end
       end
 
       context 'when set to SHA2 digest (to facilitate OTRS imports)' do
         it 'does not re-hash before saving' do
-          user.password = "{sha2}#{Digest::SHA2.hexdigest('password')}"
+          user.password = "{sha2}#{Digest::SHA2.hexdigest(password)}"
 
           expect { user.save }.not_to change(user, :password)
         end
@@ -719,11 +794,21 @@ RSpec.describe User, type: :model do
 
       context 'when set to Argon2 digest' do
         it 'does not re-hash before saving' do
-          user.password = PasswordHash.crypt('password')
+          user.password = PasswordHash.crypt(password)
 
           expect { user.save }.not_to change(user, :password)
         end
       end
+
+      context 'when creating two users with the same password' do
+        before { user.update(password: password) }
+
+        let(:another_user) { create(:user, password: password) }
+
+        it 'does not generate the same password hash' do
+          expect(user.password).not_to eq(another_user.password)
+        end
+      end
     end
 
     describe '#phone' do