Merge pull request #4838 from oliverguenther/feature/bcrypt

UserPasswords: Replace salted SHA-1 with bcrypt

Gemfile

@@ -80,6 +80,8 @@ gem 'ruby-duration', '~> 3.2.0'
 # provide compatible filesystem information for available storage
 gem 'sys-filesystem', '~> 1.1.4', require: false
 
+gem 'bcrypt', '~> 3.1.6'
+
 # We rely on this specific version, which is the latest as of now (end of 2013),
 # because we have to apply to it a bugfix which could break things in other versions.
 # This can be removed as soon as said bugfix is integrated into rabl itself.

Gemfile.lock

@@ -153,6 +153,7 @@ GEM
       descendants_tracker (~> 0.0.4)
       ice_nine (~> 0.11.0)
       thread_safe (~> 0.3, >= 0.3.1)
+    bcrypt (3.1.10)
     binding_of_caller (0.7.2)
       debug_inspector (>= 0.0.1)
     bourbon (4.2.6)
@@ -592,6 +593,7 @@ DEPENDENCIES
   airbrake (~> 5.1.0)
   autoprefixer-rails
   awesome_nested_set!
+  bcrypt (~> 3.1.6)
   bourbon (~> 4.2.0)
   capybara (~> 2.8.1)
   capybara-ng (~> 0.2.7)

app/controllers/my_controller.rb

@@ -103,7 +103,7 @@ class MyController < ApplicationController
     @user = User.current  # required by "my" layout
     @username = @user.login
     return if redirect_if_password_change_not_allowed_for(@user)
-    if @user.check_password?(params[:password])
+    if @user.check_password?(params[:password], update_legacy: false)
       @user.password = params[:new_password]
       @user.password_confirmation = params[:new_password_confirmation]
       @user.force_password_change = false

app/models/user.rb

@@ -165,7 +165,7 @@ class User < Principal
   # create new password if password was set
   def update_password
     if password && auth_source_id.blank?
-      new_password = passwords.build
+      new_password = passwords.build(type: UserPassword.active_type.to_s)
       new_password.plain_password = password
       new_password.save
 
@@ -357,12 +357,14 @@ class User < Principal
   end
 
   # Returns true if +clear_password+ is the correct user's password, otherwise false
-  def check_password?(clear_password)
+  # If +update_legacy+ is set, will automatically save legacy passwords using the current
+  # format.
+  def check_password?(clear_password, update_legacy: true)
     if auth_source_id.present?
       auth_source.authenticate(login, clear_password)
     else
       return false if current_password.nil?
-      current_password.same_as_plain_password?(clear_password)
+      current_password.matches_plaintext?(clear_password, update_legacy: update_legacy)
     end
   end
 
@@ -741,7 +743,7 @@ class User < Principal
     ban_count = Setting[:password_count_former_banned].to_i
     # make reducing the number of banned former passwords immediately effective
     # by only checking this number of former passwords
-    passwords[0, ban_count].any? { |f| f.same_as_plain_password?(password) }
+    passwords[0, ban_count].any? { |f| f.matches_plaintext?(password) }
   end
 
   def clean_up_former_passwords
@@ -840,7 +842,7 @@ class User < Principal
   end
 
   def self.default_admin_account_changed?
-    !User.active.find_by_login('admin').try(:current_password).try(:same_as_plain_password?, 'admin')
+    !User.active.find_by_login('admin').try(:current_password).try(:matches_plaintext?, 'admin')
   end
 end
 

app/models/user_password.rb

@@ -35,52 +35,75 @@ class UserPassword < ActiveRecord::Base
 
   attr_accessor :plain_password
 
-  # Checks whether the stored password is the same as a given plaintext password
-  def same_as_plain_password?(plain_password)
-    UserPassword.secure_equals?(UserPassword.hash_with_salt(plain_password,
-                                                            salt),
-                                hashed_password)
+  ##
+  # Fixes the active UserPassword Type to use.
+  # This could allow for an entrypoint for plugins or customization
+  def self.active_type
+    UserPassword::Bcrypt
   end
 
-  def expired?
-    days_valid = Setting.password_days_valid.to_i.days
-    return false if days_valid == 0
-    created_at < (Time.now - days_valid)
-  end
+  ##
+  # Determines whether the hashed value of +plain+ matches the stored password hash.
+  def matches_plaintext?(plain, update_legacy: true)
+    if hash_matches?(plain)
 
-  # Returns a 128bits random salt as a hex string (32 chars long)
-  def self.generate_salt
-    SecureRandom.hex(16)
-  end
+      # Update hash if necessary
+      if update_legacy
+        rehash_as_active(plain)
+      end
+
+      return true
+    end
 
-  # Return password digest
-  def self.hash_password(plain_password)
-    Digest::SHA1.hexdigest(plain_password)
+    false
   end
 
-  # Hash a plaintext password with a given salt
-  # The hashed password has following form: SHA1(salt + SHA1(password))
-  def self.hash_with_salt(plain_password, salt)
-    # We should really use a standard key-derivation function like bcrypt here
-    hash_password("#{salt}#{hash_password plain_password}")
+  ##
+  # Rehash the password using the currently active strategy.
+  # This replaces the password and keeps expiry date identical.
+  def rehash_as_active(plain)
+    active_class = UserPassword.active_type
+
+    unless is_a?(active_class)
+      active = becomes!(active_class)
+      active.rehash_from_legacy(plain)
+
+      active
+    end
+  rescue => e
+    Rails.logger.error("Unable to re-hash UserPassword for #{user.login}: #{e.message}")
   end
 
-  # constant-time comparison algorithm to prevent timing attacks
-  def self.secure_equals?(a, b)
-    return false if a.blank? || b.blank? || a.bytesize != b.bytesize
-    l = a.unpack "C#{a.bytesize}"
+  ##
+  # Actually derive and save the password using +active_type+
+  # We require a public interface since +becomes!+ does change the STI instance,
+  # but returns, not changes the actual current object.
+  def rehash_from_legacy(plain)
+    self.hashed_password = derive_password!(plain)
+    save!
+  end
 
-    res = 0
-    b.each_byte do |byte| res |= byte ^ l.shift end
-    res == 0
+  def expired?
+    days_valid = Setting.password_days_valid.to_i.days
+    return false if days_valid == 0
+    created_at < (Time.now - days_valid)
   end
 
-  private
+  protected
 
+  # Save hashed_password from the initially passed plain password
+  # if it is is set.
   def salt_and_hash_password!
     return if plain_password.nil?
-    self.salt = UserPassword.generate_salt
-    self.hashed_password = UserPassword.hash_with_salt(plain_password,
-                                                       salt)
+    self.hashed_password = derive_password!(plain_password)
+  end
+
+  # Require the implementation to provide a secure comparisation
+  def hash_matches?(_plain)
+    raise NotImplementedError, 'Must be overridden by subclass'
+  end
+
+  def derive_password!(_input)
+    raise NotImplementedError, 'Must be overridden by subclass'
   end
 end

app/models/user_password/bcrypt.rb

@@ -0,0 +1,44 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+##
+# Password hashing method using bcrypt
+class UserPassword::Bcrypt < UserPassword
+  protected
+
+  ##
+  # Determines whether the hashed value of +plain+ matches the stored password hash.
+  def hash_matches?(plain)
+    BCrypt::Password.new(hashed_password) == plain
+  end
+
+  def derive_password!(input)
+    BCrypt::Password.create(input)
+  end
+end

app/models/user_password/sha1.rb

@@ -0,0 +1,70 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+##
+# LEGACY password hashing method using salted SHA-1
+# This is only included for testing hashed passwords and will raise when trying
+# to save new passwords with that strategy.
+class UserPassword::SHA1 < UserPassword
+  protected
+
+  ##
+  # Determines whether the hashed value of +plain+ matches the stored password hash.
+  def hash_matches?(plain)
+    test_hash = derive_password!(plain)
+    secure_equals?(test_hash, hashed_password)
+  end
+
+  # constant-time comparison algorithm to prevent timing attacks
+  def secure_equals?(a, b)
+    return false if a.blank? || b.blank? || a.bytesize != b.bytesize
+    l = a.unpack "C#{a.bytesize}"
+
+    res = 0
+    b.each_byte do |byte| res |= byte ^ l.shift end
+    res == 0
+  end
+
+  ##
+  # Override the base method to disallow new passwords being generated this way.
+  def salt_and_hash_password!
+    raise ArgumentError, 'Do not use UserPassword::SHA1 for new passwords!'
+  end
+
+  ##
+  # Hash a plaintext password with a given salt
+  # The hashed password has following form: SHA1(salt + SHA1(password))
+  def derive_password!(input)
+    hashfn("#{salt}#{hashfn(input)}")
+  end
+
+  def hashfn(input)
+    Digest::SHA1.hexdigest(input)
+  end
+end

config/initializers/bcrypt.rb

@@ -0,0 +1,41 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+if OpenProject::Configuration.override_bcrypt_cost_factor?
+  cost_factor = OpenProject::Configuration.override_bcrypt_cost_factor.to_i
+  current = BCrypt::Engine.cost
+
+  if cost_factor < 8
+    Rails.logger.warn {
+      "Ignoring BCrypt cost factor #{cost_factor}. Using default (#{current})."
+    }
+  else
+    BCrypt::Engine.cost = cost_factor
+  end
+end

db/migrate/20160829225633_introduce_bcrypt_passwords.rb

@@ -0,0 +1,36 @@
+class IntroduceBcryptPasswords < ActiveRecord::Migration
+  def up
+    # Introduce type to UserPassword
+    add_column :user_passwords, :type, :string, null: true
+
+    # Increase hash limit due to bcrypt embedded salt
+    change_column :user_passwords, :hashed_password, :string, limit: 128, null: false
+
+    # All current passwords are assumed to be SHA-1 salted.
+    UserPassword.update_all(type: 'UserPassword::SHA1')
+
+    # Make type non-optional
+    change_column :user_passwords, :type, :string, null: false
+
+    # Make salt explicitly optional
+    change_column_null :user_passwords, :salt, true
+  end
+
+  def down
+    unless ENV['OPENPROJECT_CONFIRM_ROLLBACK'] == '20160829225633'
+      raise ActiveRecord::IrreversibleMigration, <<-EXC.strip_heredoc
+        WARNING
+
+        You cannot roll back this migration without losing passwords.
+        If you really want to do undo BCrypt passwords, set the following ENV variable:
+
+        export OPENPROJECT_CONFIRM_ROLLBACK="20160829225633"
+      EXC
+    end
+
+    UserPassword.where(type: 'UserPassword::Bcrypt').delete_all
+    remove_column :user_passwords, :type
+    change_column :user_passwords, :hashed_password, :string, limit: 40
+    # Salt was (implictly) optional
+  end
+end

lib/open_project/configuration.rb

@@ -84,6 +84,7 @@ module OpenProject
       'internal_password_confirmation' => true,
 
       'disable_password_choice' => false,
+      'override_bcrypt_cost_factor' => nil,
 
       'disabled_modules' => [], # allow to disable default modules
       'hidden_menu_items' => {},

spec/factories/user_password_factory.rb

@@ -27,7 +27,7 @@
 #++
 
 FactoryGirl.define do
-  factory :user_password do
+  factory :user_password, class: UserPassword.active_type do
     association :user
     plain_password 'adminADMIN!'
 
@@ -36,4 +36,18 @@ FactoryGirl.define do
       updated_at 1.year.ago
     end
   end
+
+  factory :legacy_sha1_password, class: UserPassword::SHA1 do
+    association :user
+    type 'UserPassword::SHA1'
+    plain_password 'mylegacypassword!'
+
+    # Avoid going through the after_save hook
+    # As it's no longer possible for Sha1 passwords
+    after(:build) do |obj|
+      obj.salt = SecureRandom.hex(16)
+      obj.hashed_password = obj.send(:derive_password!, obj.plain_password)
+      obj.plain_password = nil
+    end
+  end
 end

spec/models/user_password_spec.rb

@@ -30,7 +30,8 @@ require 'spec_helper'
 
 describe UserPassword, type: :model do
   let(:old_password) { FactoryGirl.create(:old_user_password) }
-  let(:password) { FactoryGirl.create(:user_password) }
+  let(:user) { FactoryGirl.create(:user) }
+  let(:password) { FactoryGirl.create(:user_password, user: user, plain_password: 'adminAdmin!') }
 
   describe '#expired?' do
     context 'with expiry value set',
@@ -51,4 +52,56 @@ describe UserPassword, type: :model do
       end
     end
   end
+
+  describe '#matches_plaintext?' do
+    it 'still matches the password' do
+      expect(password).to be_a(UserPassword.active_type)
+      expect(password.matches_plaintext?('adminAdmin!')).to be_truthy
+    end
+  end
+
+  describe '#rehash_as_active' do
+    let(:password) {
+      pass = FactoryGirl.build(:legacy_sha1_password, user: user, plain_password: 'adminAdmin!')
+      expect(pass).to receive(:salt_and_hash_password!).and_return nil
+
+      pass.save!
+      pass
+    }
+
+    before do
+      password
+      user.reload
+    end
+
+    it 'rehashed the password when correct' do
+      expect(user.current_password).to be_a(UserPassword::SHA1)
+      expect {
+        password.matches_plaintext?('adminAdmin!')
+      }.to_not change { user.passwords.count }
+
+      expect(user.current_password).to be_a(UserPassword::Bcrypt)
+      expect(user.current_password.hashed_password).to start_with '$2a$'
+    end
+
+    it 'does not alter the password when invalid' do
+      expect(password.matches_plaintext?('wat')).to be false
+      expect(password).to be_a(UserPassword::SHA1)
+    end
+
+    it 'does not alter the password when disabled' do
+      expect(password.matches_plaintext?('adminAdmin!', update_legacy: false)).to be true
+      expect(user.current_password).to be_a(UserPassword::SHA1)
+    end
+  end
+
+  describe '#save' do
+    let(:password) { FactoryGirl.build(:user_password) }
+
+    it 'saves correctly' do
+      expect(password).to receive(:salt_and_hash_password!).and_call_original
+      expect { password.save! }.not_to raise_error
+      expect(password).not_to be_expired
+    end
+  end
 end

spec/models/user_passwords/sha1_spec.rb

@@ -0,0 +1,54 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe UserPassword::SHA1, type: :model do
+  let(:legacy_password) {
+    pass = FactoryGirl.build(:legacy_sha1_password, plain_password: 'adminAdmin!')
+    expect(pass).to receive(:salt_and_hash_password!).and_return nil
+
+    pass.save!
+    pass
+  }
+
+  describe '#matches_plaintext?' do
+    it 'still matches for existing passwords' do
+      expect(legacy_password).to be_a(UserPassword::SHA1)
+      expect(legacy_password.matches_plaintext?('adminAdmin!')).to be_truthy
+    end
+  end
+
+  describe '#create' do
+    let(:legacy_password) { FactoryGirl.build(:legacy_sha1_password) }
+
+    it 'raises an exception trying to save it' do
+      expect { legacy_password.save! }.to raise_error(ArgumentError)
+    end
+  end
+end

spec_legacy/fixtures/user_passwords.yml

@@ -30,6 +30,7 @@
 user_passwords_004:
   created_at: 2006-07-19 19:34:07 +02:00
   # password = foo
+  type: UserPassword::SHA1
   salt: 3126f764c3c5ac61cbfc103f25f934cf
   hashed_password: 9e4dd7eeb172c12a0691a6d9d3a269f7e9fe671b
   updated_at: 2006-07-19 19:34:07 +02:00
@@ -38,6 +39,7 @@ user_passwords_004:
 user_passwords_001:
   created_at: 2006-07-19 19:12:21 +02:00
   # password = adminADMIN!
+  type: UserPassword::SHA1
   salt: a19f9743f4b7b043a2fd07b8eb13a1df
   hashed_password: b4596ce83c0e154e5ce9d3dd5636fde4d6f38b75
   updated_at: 2006-07-19 22:57:52 +02:00
@@ -46,6 +48,7 @@ user_passwords_001:
 user_passwords_002:
   created_at: 2006-07-19 19:32:09 +02:00
   # password = jsmith
+  type: UserPassword::SHA1
   salt: 67eb4732624d5a7753dcea7ce0bb7d7d
   hashed_password: bfbe06043353a677d0215b26a5800d128d5413bc
   updated_at: 2006-07-19 22:42:15 +02:00
@@ -54,6 +57,7 @@ user_passwords_002:
 user_passwords_003:
   created_at: 2006-07-19 19:33:19 +02:00
   # password = foo
+  type: UserPassword::SHA1
   salt: 7599f9963ec07b5a3b55b354407120c0
   hashed_password: 8f659c8d7c072f189374edacfa90d6abbc26d8ed
   updated_at: 2006-07-19 19:33:19 +02:00
@@ -63,28 +67,34 @@ user_passwords_005:
   id: 5
   user_id: 5
   created_at: 2006-07-19 19:33:19 +02:00
+  type: UserPassword::SHA1
   hashed_password: 1
   updated_at: 2006-07-19 19:33:19 +02:00
 user_passwords_006:
   id: 6
   user_id: 6
   created_at: 2006-07-19 19:33:19 +02:00
+  type: UserPassword::SHA1
   hashed_password: 1
   updated_at: 2006-07-19 19:33:19 +02:00
 user_passwords_007:
   id: 7
   user_id: 7
+  type: UserPassword::SHA1
+  hashed_password: 1
   created_at: 2006-07-19 19:33:19 +02:00
   updated_at: 2006-07-19 19:33:19 +02:00
 user_passwords_008:
   id: 8
   user_id: 8
+  type: UserPassword::SHA1
   created_at: 2006-07-19 19:33:19 +02:00
   hashed_password: 1
   updated_at: 2006-07-19 19:33:19 +02:00
 user_passwords_009:
   id: 9
   user_id: 9
+  type: UserPassword::SHA1
   created_at: 2006-07-19 19:33:19 +02:00
   hashed_password: 1
   updated_at: 2006-07-19 19:33:19 +02:00