From 1e54645223c0a5d71e910d82a4d7be8cc475f1a4 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Thu, 27 Jun 2019 13:51:29 +0200 Subject: [PATCH] Fixed issue #462 - Password hashing: upgrade to argon2 2.x (argon2id), fix non-unique salt bug - huge thanks to @martinvonwittich ! --- Gemfile | 2 +- Gemfile.lock | 10 ++--- app/models/user.rb | 23 +++++----- lib/password_hash.rb | 21 ++++++--- spec/models/user_spec.rb | 93 ++++++++++++++++++++++++++++++++++++++-- 5 files changed, 119 insertions(+), 30 deletions(-) diff --git a/Gemfile b/Gemfile index 714fbff06..260709821 100644 --- a/Gemfile +++ b/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' diff --git a/Gemfile.lock b/Gemfile.lock index b15e457f9..601314e0d 100644 --- a/Gemfile.lock +++ b/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-compiler (0.1.3) + ffi (1.11.1) + 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 diff --git a/app/models/user.rb b/app/models/user.rb index ea1949e1d..4e293406d 100644 --- a/app/models/user.rb +++ b/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? - return true if PasswordHash.crypted?(password) - - self.password = PasswordHash.crypt(password) + self.password = ensured_password true end - def password_empty? - # set old password again if not given - return if password.present? + def ensured_password + # ensure unset password for blank values of new users + return nil if new_record? && password.blank? - # skip if it's not desired to set a password (yet) - return true if !password + # don't permit empty password update for existing users + return password_was if password.blank? - # get current record - return if !id + # don't re-hash an already hashed passsword + return password if PasswordHash.crypted?(password) - self.password = password_was - true + # hash the plaintext password + PasswordHash.crypt(password) end # reset login_failed if password is changed diff --git a/lib/password_hash.rb b/lib/password_hash.rb index 5e5f230ba..dbdd972f4 100644 --- a/lib/password_hash.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 60f933c89..e0b68c2f8 100644 --- a/spec/models/user_spec.rb +++ b/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