From 0862bf7e6a6a9a67016083e528c51299824652e9 Mon Sep 17 00:00:00 2001
From: Thorsten Eckel <te@znuny.com>
Date: Fri, 27 Jan 2017 09:17:03 +0100
Subject: [PATCH] Improved password security by using proper password hash
 module backed by Argon2 (official winner of the Password Hashing Competition)
 - thanks to @nomoketo and @benbe.

---
 Gemfile                                       |  8 ++-
 Gemfile.lock                                  | 15 ++++-
 app/models/user.rb                            | 39 ++++++------
 ...170126091128_application_secret_setting.rb | 20 +++++++
 db/seeds.rb                                   | 12 ++++
 lib/auth/internal.rb                          | 25 +++++---
 lib/password_hash.rb                          | 48 +++++++++++++++
 spec/auth/internal_spec.rb                    | 31 ++++++++++
 spec/factories/user.rb                        | 24 ++++++++
 spec/password_hash_spec.rb                    | 60 +++++++++++++++++++
 spec/rails_helper.rb                          |  1 +
 spec/support/factory_girl.rb                  |  3 +
 12 files changed, 255 insertions(+), 31 deletions(-)
 create mode 100644 db/migrate/20170126091128_application_secret_setting.rb
 create mode 100644 lib/password_hash.rb
 create mode 100644 spec/auth/internal_spec.rb
 create mode 100644 spec/factories/user.rb
 create mode 100644 spec/password_hash_spec.rb
 create mode 100644 spec/support/factory_girl.rb

diff --git a/Gemfile b/Gemfile
index 5c3793a04..4ff966627 100644
--- a/Gemfile
+++ b/Gemfile
@@ -66,6 +66,9 @@ require 'yaml'
 
 gem 'net-ldap'
 
+# password security
+gem 'argon2'
+
 gem 'writeexcel'
 gem 'icalendar'
 gem 'browser'
@@ -85,7 +88,7 @@ gem 'diffy'
 # in production environments by default.
 group :development, :test do
 
-  gem 'rspec-rails'
+  gem 'rspec-rails', '~> 3.5'
   gem 'test-unit'
   gem 'spring'
   gem 'spring-commands-rspec'
@@ -115,6 +118,9 @@ group :development, :test do
 
   # Setting ENV for testing purposes
   gem 'figaro'
+
+  # Use Factory Girl for generating random test data
+  gem 'factory_girl_rails'
 end
 
 gem 'puma', group: :puma
diff --git a/Gemfile.lock b/Gemfile.lock
index 2d4f2c7b0..5a7a29ba3 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -46,6 +46,9 @@ GEM
       tzinfo (~> 1.1)
     addressable (2.4.0)
     arel (6.0.3)
+    argon2 (1.1.1)
+      ffi (~> 1.9)
+      ffi-compiler (~> 0.1)
     ast (2.3.0)
     autoprefixer-rails (6.4.1.1)
       execjs
@@ -107,11 +110,19 @@ GEM
     erubis (2.7.0)
     eventmachine (1.2.0.1)
     execjs (2.7.0)
+    factory_girl (4.8.0)
+      activesupport (>= 3.0.0)
+    factory_girl_rails (4.8.0)
+      factory_girl (~> 4.8.0)
+      railties (>= 3.0.0)
     faraday (0.9.2)
       multipart-post (>= 1.2, < 3)
     faraday-http-cache (1.3.1)
       faraday (~> 0.8)
     ffi (1.9.14)
+    ffi-compiler (0.1.3)
+      ffi (>= 1.0.0)
+      rake
     figaro (1.1.1)
       thor (~> 0.14)
     formatador (0.2.5)
@@ -396,6 +407,7 @@ PLATFORMS
 DEPENDENCIES
   activerecord-nulldb-adapter
   activerecord-session_store
+  argon2
   autoprefixer-rails
   biz
   browser
@@ -413,6 +425,7 @@ DEPENDENCIES
   email_verifier
   eventmachine
   execjs
+  factory_girl_rails
   figaro
   github_changelog_generator
   guard
@@ -442,7 +455,7 @@ DEPENDENCIES
   rails (= 4.2.7.1)
   rails-observers
   rb-fsevent
-  rspec-rails
+  rspec-rails (~> 3.5)
   rubocop
   sass-rails
   selenium-webdriver
diff --git a/app/models/user.rb b/app/models/user.rb
index aa0570a0f..d35ba3ede 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -32,7 +32,7 @@ class User < ApplicationModel
   load 'user/search_index.rb'
   include User::SearchIndex
 
-  before_validation :check_name, :check_email, :check_login, :check_password
+  before_validation :check_name, :check_email, :check_login, :ensure_password
   before_create   :check_preferences_default, :validate_roles, :domain_based_assignment
   before_update   :check_preferences_default, :validate_roles
   after_create    :avatar_for_email_check
@@ -906,26 +906,23 @@ returns
     Avatar.remove('User', id)
   end
 
-  def check_password
-
-    # set old password again if not given
-    if password.blank?
-
-      # get current record
-      if id
-        #current = User.find(self.id)
-        #self.password = current.password
-        self.password = password_was
-      end
-
-    end
-
-    # crypt password if not already crypted
-    return if !password
-    return if password =~ /^\{sha2\}/
-
-    crypted       = Digest::SHA2.hexdigest(password)
-    self.password = "{sha2}#{crypted}"
+  def ensure_password
+    return if password_empty?
+    return if PasswordHash.crypted?(password)
+    self.password = PasswordHash.crypt(password)
   end
 
+  def password_empty?
+    # set old password again if not given
+    return if password.present?
+
+    # skip if it's not desired to set a password (yet)
+    return true if !password
+
+    # get current record
+    return if !id
+
+    self.password = password_was
+    true
+  end
 end
diff --git a/db/migrate/20170126091128_application_secret_setting.rb b/db/migrate/20170126091128_application_secret_setting.rb
new file mode 100644
index 000000000..6a81251eb
--- /dev/null
+++ b/db/migrate/20170126091128_application_secret_setting.rb
@@ -0,0 +1,20 @@
+class ApplicationSecretSetting < ActiveRecord::Migration
+  def up
+
+    # return if it's a new setup
+    return if !Setting.find_by(name: 'system_init_done')
+
+    Setting.create_if_not_exists(
+      title: 'Application secret',
+      name: 'application_secret',
+      area: 'Core',
+      description: 'Defines the random application secret.',
+      options: {},
+      state: SecureRandom.hex(128),
+      preferences: {
+        permission: ['admin'],
+      },
+      frontend: false
+    )
+  end
+end
diff --git a/db/seeds.rb b/db/seeds.rb
index f43b3eefb..bf1b538e4 100644
--- a/db/seeds.rb
+++ b/db/seeds.rb
@@ -10,6 +10,18 @@
 # clear old caches to start from scratch
 Cache.clear
 
+Setting.create_if_not_exists(
+  title: 'Application secret',
+  name: 'application_secret',
+  area: 'Core',
+  description: 'Defines the random application secret.',
+  options: {},
+  state: SecureRandom.hex(128),
+  preferences: {
+    permission: ['admin'],
+  },
+  frontend: false
+)
 Setting.create_if_not_exists(
   title: 'System Init Done',
   name: 'system_init_done',
diff --git a/lib/auth/internal.rb b/lib/auth/internal.rb
index 4eaf8b500..b8a26a751 100644
--- a/lib/auth/internal.rb
+++ b/lib/auth/internal.rb
@@ -1,21 +1,30 @@
 # Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
 
 module Auth::Internal
-  def self.check(username, password, _config, user)
+
+  # rubocop:disable Style/ModuleFunction
+  extend self
+
+  def check(username, password, _config, user)
 
     # return if no user exists
     return false if !username
     return false if !user
 
-    # sha auth check
-    if user.password =~ /^\{sha2\}/
-      crypted = Digest::SHA2.hexdigest(password)
-      return user if user.password == "{sha2}#{crypted}"
+    if PasswordHash.legacy?(user.password, password)
+      update_password(user, password)
+      return user
     end
 
-    # plain auth check
-    return user if user.password == password
+    return false if !PasswordHash.verified?(user.password, password)
 
-    false
+    user
+  end
+
+  private
+
+  def update_password(user, password)
+    user.password = PasswordHash.crypt(password)
+    user.save
   end
 end
diff --git a/lib/password_hash.rb b/lib/password_hash.rb
new file mode 100644
index 000000000..4adae53c4
--- /dev/null
+++ b/lib/password_hash.rb
@@ -0,0 +1,48 @@
+# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
+
+module PasswordHash
+  include ApplicationLib
+
+  # rubocop:disable Style/ModuleFunction
+  extend self
+
+  def crypt(password)
+    argon2.create(password)
+  end
+
+  def verified?(pw_hash, password)
+    Argon2::Password.verify_password(password, pw_hash, secret)
+  rescue
+    false
+  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
+  end
+
+  def legacy?(pw_hash, password)
+    return if pw_hash.blank?
+    return if !password
+    legacy_sha2?(pw_hash, password)
+  end
+
+  private
+
+  def legacy_sha2?(pw_hash, password)
+    return if !pw_hash.start_with?('{sha2}')
+    crypted = Digest::SHA2.hexdigest(password)
+    pw_hash == "{sha2}#{crypted}"
+  end
+
+  def argon2
+    return @argon2 if @argon2
+    @argon2 = Argon2::Password.new(secret: secret)
+  end
+
+  def secret
+    Setting.get('application_secret')
+  end
+end
diff --git a/spec/auth/internal_spec.rb b/spec/auth/internal_spec.rb
new file mode 100644
index 000000000..df50bd5cf
--- /dev/null
+++ b/spec/auth/internal_spec.rb
@@ -0,0 +1,31 @@
+require 'rails_helper'
+
+RSpec.describe Auth::Internal do
+
+  it 'authenticates via password' do
+    user     = create(:user)
+    password = 'zammad'
+    result   = described_class.check(user.login, password, {}, user)
+
+    expect(result).to be_an_instance_of(User)
+  end
+
+  it "doesn't authenticate via plain password" do
+    user   = create(:user)
+    result = described_class.check(user.login, user.password, {}, user)
+
+    expect(result).to be_falsy
+  end
+
+  it 'converts legacy sha2 passwords' do
+    user     = create(:user_legacy_password_sha2)
+    password = 'zammad'
+
+    expect(PasswordHash.crypted?(user.password)).to be_falsy
+
+    result = described_class.check(user.login, password, {}, user)
+
+    expect(result).to be_an_instance_of(User)
+    expect(PasswordHash.crypted?(user.password)).to be true
+  end
+end
diff --git a/spec/factories/user.rb b/spec/factories/user.rb
new file mode 100644
index 000000000..bae6b1953
--- /dev/null
+++ b/spec/factories/user.rb
@@ -0,0 +1,24 @@
+FactoryGirl.define do
+  sequence :email do |n|
+    "nicole.braun#{n}@zammad.org"
+  end
+end
+
+FactoryGirl.define do
+
+  factory :user do
+    login         'nicole.braun'
+    firstname     'Nicole'
+    lastname      'Braun'
+    email         { generate(:email) }
+    password      'zammad'
+    active        true
+    updated_by_id 1
+    created_by_id 1
+  end
+
+  factory :user_legacy_password_sha2, parent: :user do
+    after(:build) { |user| user.class.skip_callback(:validation, :before, :ensure_password) }
+    password '{sha2}dd9c764fa7ea18cd992c8600006d3dc3ac983d1ba22e9ba2d71f6207456be0ba' # zammad
+  end
+end
diff --git a/spec/password_hash_spec.rb b/spec/password_hash_spec.rb
new file mode 100644
index 000000000..9c62daf68
--- /dev/null
+++ b/spec/password_hash_spec.rb
@@ -0,0 +1,60 @@
+require 'rails_helper'
+
+RSpec.describe PasswordHash do
+
+  let(:pw_plain) { 'zammad' }
+
+  context 'stable API' do
+    it 'responds to crypt' do
+      expect(described_class).to respond_to(:crypt)
+    end
+
+    it 'responds to verified?' do
+      expect(described_class).to respond_to(:verified?)
+    end
+
+    it 'responds to crypted?' do
+      expect(described_class).to respond_to(:crypted?)
+    end
+
+    it 'responds to legacy?' do
+      expect(described_class).to respond_to(:legacy?)
+    end
+  end
+
+  context 'encryption' do
+
+    it 'crypts passwords' do
+      pw_crypted = described_class.crypt(pw_plain)
+      expect(pw_crypted).not_to eq(pw_plain)
+    end
+
+    it 'verifies crypted passwords' do
+      pw_crypted = described_class.crypt(pw_plain)
+      expect(described_class.verified?(pw_crypted, pw_plain)).to be true
+    end
+
+    it 'detects crypted passwords' do
+      pw_crypted = described_class.crypt(pw_plain)
+      expect(described_class.crypted?(pw_crypted)).to be true
+    end
+  end
+
+  context 'legacy' do
+
+    let(:zammad_sha2) { '{sha2}dd9c764fa7ea18cd992c8600006d3dc3ac983d1ba22e9ba2d71f6207456be0ba' }
+
+    it 'requires hash to be not blank' do
+      expect(described_class.legacy?(nil, pw_plain)).to be_falsy
+      expect(described_class.legacy?('', pw_plain)).to be_falsy
+    end
+
+    it 'requires password to be not nil' do
+      expect(described_class.legacy?(zammad_sha2, nil)).to be_falsy
+    end
+
+    it 'detects sha2 hashes' do
+      expect(described_class.legacy?(zammad_sha2, pw_plain)).to be true
+    end
+  end
+end
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index fc688e11d..9d8b4dd45 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -6,6 +6,7 @@ require File.expand_path('../../config/environment', __FILE__)
 abort('The Rails environment is running in production mode!') if Rails.env.production?
 require 'spec_helper'
 require 'rspec/rails'
+require 'support/factory_girl'
 # Add additional requires below this line. Rails is not loaded until this point!
 
 # Requires supporting ruby files with custom matchers and macros, etc, in
diff --git a/spec/support/factory_girl.rb b/spec/support/factory_girl.rb
new file mode 100644
index 000000000..eec437fb3
--- /dev/null
+++ b/spec/support/factory_girl.rb
@@ -0,0 +1,3 @@
+RSpec.configure do |config|
+  config.include FactoryGirl::Syntax::Methods
+end