From b8a92582a1e3725769122b523a86778d049865b8 Mon Sep 17 00:00:00 2001
From: Thorsten Eckel <te@znuny.com>
Date: Mon, 8 May 2017 12:04:54 +0200
Subject: [PATCH] Fixed bug: Legacy hashed passwords (like sha2) will get
 rehashed when set.

This happens when a user password should get set without transfering it in plain like e.g. the REST API, an import or the auto_wizard.json file. Therefore Zammad should accept legacy passwords when set but should convert them on first login to the internal format (as already implemented).

Additionally there should be no exclusion for the import mode when setting/ensuring a password since Zammand couldn't handle it later.
---
 app/models/user.rb             |  1 -
 lib/password_hash.rb           | 35 +++++++++++++++++++++++-----------
 spec/factories/user.rb         |  5 -----
 spec/lib/auth/internal_spec.rb | 11 ++++++++---
 spec/lib/password_hash_spec.rb | 32 +++++++++++++++++++++++++++++++
 5 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index cbf763f2a..8b01e0afa 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -980,7 +980,6 @@ raise 'Minimum one user need to have admin permissions'
 
   def ensure_password
     return if password_empty?
-    return if Setting.get('import_mode')
     return if PasswordHash.crypted?(password)
     self.password = PasswordHash.crypt(password)
   end
diff --git a/lib/password_hash.rb b/lib/password_hash.rb
index 4adae53c4..831a8a2e8 100644
--- a/lib/password_hash.rb
+++ b/lib/password_hash.rb
@@ -17,24 +17,37 @@ module PasswordHash
   end
 
   def crypted?(pw_hash)
-    return if !pw_hash
-    # taken from: https://github.com/technion/ruby-argon2/blob/7e1f4a2634316e370ab84150e4f5fd91d9263713/lib/argon2.rb#L33
-    return if pw_hash !~ /^\$argon2i\$.{,112}/
-    true
+    return false if !pw_hash
+    return true if hashed_argon2?(pw_hash)
+    return true if hashed_sha2?(pw_hash)
+    false
   end
 
   def legacy?(pw_hash, password)
-    return if pw_hash.blank?
-    return if !password
-    legacy_sha2?(pw_hash, password)
+    return false if pw_hash.blank?
+    return false if !password
+    sha2?(pw_hash, password)
+  end
+
+  def hashed_sha2?(pw_hash)
+    pw_hash.start_with?('{sha2}')
+  end
+
+  def hashed_argon2?(pw_hash)
+    # taken from: https://github.com/technion/ruby-argon2/blob/7e1f4a2634316e370ab84150e4f5fd91d9263713/lib/argon2.rb#L33
+    pw_hash =~ /^\$argon2i\$.{,112}/
+  end
+
+  def sha2(password)
+    crypted = Digest::SHA2.hexdigest(password)
+    "{sha2}#{crypted}"
   end
 
   private
 
-  def legacy_sha2?(pw_hash, password)
-    return if !pw_hash.start_with?('{sha2}')
-    crypted = Digest::SHA2.hexdigest(password)
-    pw_hash == "{sha2}#{crypted}"
+  def sha2?(pw_hash, password)
+    return false if !hashed_sha2?(pw_hash)
+    pw_hash == sha2(password)
   end
 
   def argon2
diff --git a/spec/factories/user.rb b/spec/factories/user.rb
index 53becc371..8077e61be 100644
--- a/spec/factories/user.rb
+++ b/spec/factories/user.rb
@@ -25,9 +25,4 @@ FactoryGirl.define do
   factory :user_login_failed, parent: :user do
     login_failed { (Setting.get('password_max_login_failed').to_i || 10) + 1 }
   end
-
-  factory :user_legacy_password_sha2, parent: :user do
-    after(:build) { |user| user.class.skip_callback(:validation, :before, :ensure_password, if: -> { password && password.start_with?('{sha2}') }) }
-    password '{sha2}dd9c764fa7ea18cd992c8600006d3dc3ac983d1ba22e9ba2d71f6207456be0ba' # zammad
-  end
 end
diff --git a/spec/lib/auth/internal_spec.rb b/spec/lib/auth/internal_spec.rb
index b5aa1b7a2..cfddfb2e8 100644
--- a/spec/lib/auth/internal_spec.rb
+++ b/spec/lib/auth/internal_spec.rb
@@ -20,13 +20,18 @@ RSpec.describe Auth::Internal do
     end
 
     it 'converts legacy sha2 passwords' do
-      user = create(:user_legacy_password_sha2)
 
-      expect(PasswordHash.crypted?(user.password)).to be_falsy
+      pw_plain = 'zammad'
+      sha2_pw  = PasswordHash.sha2(pw_plain)
+      user     = create(:user, password: sha2_pw)
 
-      result = instance.valid?(user, 'zammad')
+      expect(PasswordHash.crypted?(user.password)).to be true
+      expect(PasswordHash.legacy?(user.password, pw_plain)).to be true
+
+      result = instance.valid?(user, pw_plain)
       expect(result).to be true
 
+      expect(PasswordHash.legacy?(user.password, pw_plain)).to be false
       expect(PasswordHash.crypted?(user.password)).to be true
     end
   end
diff --git a/spec/lib/password_hash_spec.rb b/spec/lib/password_hash_spec.rb
index 9c62daf68..70d5573a4 100644
--- a/spec/lib/password_hash_spec.rb
+++ b/spec/lib/password_hash_spec.rb
@@ -20,6 +20,18 @@ RSpec.describe PasswordHash do
     it 'responds to legacy?' do
       expect(described_class).to respond_to(:legacy?)
     end
+
+    it 'responds to sha2' do
+      expect(described_class).to respond_to(:sha2)
+    end
+
+    it 'responds to hashed_sha2?' do
+      expect(described_class).to respond_to(:hashed_sha2?)
+    end
+
+    it 'responds to hashed_argon2?' do
+      expect(described_class).to respond_to(:hashed_argon2?)
+    end
   end
 
   context 'encryption' do
@@ -56,5 +68,25 @@ RSpec.describe PasswordHash do
     it 'detects sha2 hashes' do
       expect(described_class.legacy?(zammad_sha2, pw_plain)).to be true
     end
+
+    it 'detects crypted passwords' do
+      expect(described_class.crypted?(zammad_sha2)).to be true
+    end
+
+    describe '::sha2' do
+
+      it 'creates sha2 hashes' do
+        hashed = described_class.sha2(pw_plain)
+        expect(hashed).to eq zammad_sha2
+      end
+    end
+
+    describe '::hashed_sha2?' do
+
+      it 'detects sha2 hashes' do
+        expect(described_class.hashed_sha2?(zammad_sha2)).to be true
+      end
+    end
   end
+
 end