From 9238767f7e6bce479319a94a52098d37b9781972 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 11 Apr 2023 10:16:28 +0100 Subject: [PATCH] FEATURE: Persist password hashing algorithm/params in database (#20980) Previously, Discourse's password hashing was hard-coded to a specific algorithm and parameters. Any changes to the algorithm or parameters would essentially invalidate all existing user passwords. This commit introduces a new `password_algorithm` column on the `users` table. This persists the algorithm/parameters which were use to generate the hash for a given user. All existing rows in the users table are assumed to be using Discourse's current algorithm/parameters. With this data stored per-user in the database, we'll be able to keep existing passwords working while adjusting the algorithm/parameters for newly hashed passwords. Passwords which were hashed with an old algorithm will be automatically re-hashed with the new algorithm when the user next logs in. Values in the `password_algorithm` column are based on the PHC string format (https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md). Discourse's existing algorithm is described by the string `$pbkdf2-sha256$i=64000,l=32$` To introduce a new algorithm and start using it, make sure it's implemented in the `PasswordHasher` library, then update `User::TARGET_PASSWORD_ALGORITHM`. --- app/models/concerns/second_factor_manager.rb | 10 ++- app/models/user.rb | 35 ++++++---- ...5121453_add_password_algorithm_to_users.rb | 31 +++++++++ ...4_update_password_algorithm_post_deploy.rb | 17 +++++ lib/password_hasher.rb | 50 ++++++++++++++ spec/lib/password_hasher_spec.rb | 66 +++++++++++++++++++ spec/models/user_spec.rb | 42 +++++++++++- 7 files changed, 232 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20230405121453_add_password_algorithm_to_users.rb create mode 100644 db/post_migrate/20230405121454_update_password_algorithm_post_deploy.rb create mode 100644 lib/password_hasher.rb create mode 100644 spec/lib/password_hasher_spec.rb diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb index dc74d541e08..0adad926788 100644 --- a/app/models/concerns/second_factor_manager.rb +++ b/app/models/concerns/second_factor_manager.rb @@ -263,12 +263,10 @@ module SecondFactorManager end def hash_backup_code(code, salt) - Pbkdf2.hash_password( - code, - salt, - Rails.configuration.pbkdf2_iterations, - Rails.configuration.pbkdf2_algorithm, - ) + # Backup codes have high entropy, so we can afford to use + # a lower number of iterations than for user-specific passwords + iterations = Rails.env.test? ? 10 : 64_000 + Pbkdf2.hash_password(code, salt, iterations, "sha256") end def require_rotp diff --git a/app/models/user.rb b/app/models/user.rb index ee304e17e5e..b8e81d8d0a4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -9,6 +9,10 @@ class User < ActiveRecord::Base DEFAULT_FEATURED_BADGE_COUNT = 3 + PASSWORD_SALT_LENGTH = 16 + TARGET_PASSWORD_ALGORITHM = + "$pbkdf2-#{Rails.configuration.pbkdf2_algorithm}$i=#{Rails.configuration.pbkdf2_iterations},l=32$" + # not deleted on user delete has_many :posts has_many :topics @@ -927,8 +931,20 @@ class User < ActiveRecord::Base end def confirm_password?(password) - return false unless password_hash && salt - self.password_hash == hash_password(password, salt) + return false unless password_hash && salt && password_algorithm + confirmed = self.password_hash == hash_password(password, salt, password_algorithm) + + if confirmed && persisted? && password_algorithm != TARGET_PASSWORD_ALGORITHM + # Regenerate password_hash with new algorithm and persist + salt = SecureRandom.hex(PASSWORD_SALT_LENGTH) + update_columns( + password_algorithm: TARGET_PASSWORD_ALGORITHM, + salt: salt, + password_hash: hash_password(password, salt, TARGET_PASSWORD_ALGORITHM), + ) + end + + confirmed end def new_user_posting_on_first_day? @@ -1862,8 +1878,9 @@ class User < ActiveRecord::Base def ensure_password_is_hashed if @raw_password - self.salt = SecureRandom.hex(16) - self.password_hash = hash_password(@raw_password, salt) + self.salt = SecureRandom.hex(PASSWORD_SALT_LENGTH) + self.password_algorithm = TARGET_PASSWORD_ALGORITHM + self.password_hash = hash_password(@raw_password, salt, password_algorithm) end end @@ -1879,14 +1896,9 @@ class User < ActiveRecord::Base end end - def hash_password(password, salt) + def hash_password(password, salt, algorithm) raise StandardError.new("password is too long") if password.size > User.max_password_length - Pbkdf2.hash_password( - password, - salt, - Rails.configuration.pbkdf2_iterations, - Rails.configuration.pbkdf2_algorithm, - ) + PasswordHasher.hash_password(password: password, salt: salt, algorithm: algorithm) end def add_trust_level @@ -2228,6 +2240,7 @@ end # secure_identifier :string # flair_group_id :integer # last_seen_reviewable_id :integer +# password_algorithm :string(64) # # Indexes # diff --git a/db/migrate/20230405121453_add_password_algorithm_to_users.rb b/db/migrate/20230405121453_add_password_algorithm_to_users.rb new file mode 100644 index 00000000000..3a3ae02ba86 --- /dev/null +++ b/db/migrate/20230405121453_add_password_algorithm_to_users.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class AddPasswordAlgorithmToUsers < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + BATCH_SIZE = 5000 + + def up + if !column_exists?(:users, :password_algorithm) + add_column :users, :password_algorithm, :string, limit: 64 + end + + sql = <<~SQL + UPDATE users SET password_algorithm = '$pbkdf2-sha256$i=64000,l=32$' + WHERE id IN ( + SELECT id FROM users + WHERE users.password_hash IS NOT NULL + AND users.password_algorithm IS NULL + LIMIT #{BATCH_SIZE} + ) + SQL + + loop do + changed_count = execute(sql).cmd_tuples + break if changed_count < BATCH_SIZE + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/post_migrate/20230405121454_update_password_algorithm_post_deploy.rb b/db/post_migrate/20230405121454_update_password_algorithm_post_deploy.rb new file mode 100644 index 00000000000..c07333afe6c --- /dev/null +++ b/db/post_migrate/20230405121454_update_password_algorithm_post_deploy.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class UpdatePasswordAlgorithmPostDeploy < ActiveRecord::Migration[7.0] + def up + # Handles any users that were created by old-version app code since + # the 20230405121453 pre-deploy migration was run + execute <<~SQL + UPDATE users SET password_algorithm = '$pbkdf2-sha256$i=64000,l=32$' + WHERE users.password_algorithm IS NULL + AND users.password_hash IS NOT NULL + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/password_hasher.rb b/lib/password_hasher.rb new file mode 100644 index 00000000000..755c9280144 --- /dev/null +++ b/lib/password_hasher.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +class PasswordHasher + class InvalidAlgorithmError < StandardError + end + + class UnsupportedAlgorithmError < StandardError + end + + HANDLERS = {} + + def self.register_handler(id, &blk) + HANDLERS[id] = blk + end + + # Algorithm should be specified according to the id/params parts of the + # PHC string format. + # https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md + def self.hash_password(password:, salt:, algorithm:) + algorithm = algorithm.delete_prefix("$").delete_suffix("$") + + parts = algorithm.split("$") + raise InvalidAlgorithmError if parts.length != 2 + + algorithm_id, algorithm_params = parts + + algorithm_params = algorithm_params.split(",").map { |pair| pair.split("=") }.to_h + + handler = HANDLERS[algorithm_id] + if handler.nil? + raise UnsupportedAlgorithmError.new "#{algorithm_id} is not a supported password algorithm" + end + + handler.call(password: password, salt: salt, params: algorithm_params) + end + + register_handler("pbkdf2-sha256") do |password:, salt:, params:| + raise ArgumentError.new("Salt and password must be supplied") if password.blank? || salt.blank? + + if params["l"].to_i != 32 + raise UnsupportedAlgorithmError.new("pbkdf2 implementation only supports l=32") + end + + if params["i"].to_i < 1 + raise UnsupportedAlgorithmError.new("pbkdf2 iterations must be 1 or more") + end + + Pbkdf2.hash_password(password, salt, params["i"].to_i, "sha256") + end +end diff --git a/spec/lib/password_hasher_spec.rb b/spec/lib/password_hasher_spec.rb new file mode 100644 index 00000000000..c31d054c012 --- /dev/null +++ b/spec/lib/password_hasher_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +describe PasswordHasher do + def hash(password: "mypass", salt: "mysalt", algorithm:) + PasswordHasher.hash_password(password: password, salt: salt, algorithm: algorithm) + end + + describe "pbkdf2-sha256 algorithm" do + it "can hash correctly" do + result = hash(password: "mypass", salt: "mysalt", algorithm: "$pbkdf2-sha256$i=3,l=32$") + expect(result).to eq("8d4f9a685ff73eef7b06e07ab5889784775290a0a9cebb6eb4492a695c93a51e") + end + + it "supports different iteration numbers" do + iter_3 = hash(algorithm: "$pbkdf2-sha256$i=3,l=32$") + iter_4 = hash(algorithm: "$pbkdf2-sha256$i=4,l=32$") + + expect(iter_3).not_to eq(iter_4) + end + + it "raises an error for non-standard length" do + expect { hash(algorithm: "$pbkdf2-sha256$i=3,l=20$") }.to raise_error( + PasswordHasher::UnsupportedAlgorithmError, + ) + end + + it "raises an error for missing length param" do + expect { hash(algorithm: "$pbkdf2-sha256$i=3$") }.to raise_error( + PasswordHasher::UnsupportedAlgorithmError, + ) + end + + it "raises an error for missing iteration param" do + expect { hash(algorithm: "$pbkdf2-sha256$l=32$") }.to raise_error( + PasswordHasher::UnsupportedAlgorithmError, + ) + end + + it "raises an error for missing salt" do + expect { hash(salt: nil, algorithm: "$pbkdf2-sha256$l=32,i=3$") }.to raise_error( + ArgumentError, + ) + end + + it "raises an error for missing password" do + expect { hash(password: nil, algorithm: "$pbkdf2-sha256$l=32,i=3$") }.to raise_error( + ArgumentError, + ) + end + end + + it "raises error for invalid algorithm" do + expect { hash(algorithm: "$pbkdf2-sha256$l=32$somethinginvalid") }.to raise_error( + PasswordHasher::InvalidAlgorithmError, + ) + end + + it "raises error for unknown algorithm" do + expect { hash(algorithm: "$pbkdf2-invalid$l=32$") }.to raise_error( + PasswordHasher::UnsupportedAlgorithmError, + ) + expect { hash(algorithm: "$unknown$l=32$") }.to raise_error( + PasswordHasher::UnsupportedAlgorithmError, + ) + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e77593e0c20..d33be8e9a9e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1856,8 +1856,8 @@ RSpec.describe User do describe "hash_passwords" do let(:too_long) { "x" * (User.max_password_length + 1) } - def hash(password, salt) - User.new.send(:hash_password, password, salt) + def hash(password, salt, algorithm = User::TARGET_PASSWORD_ALGORITHM) + User.new.send(:hash_password, password, salt, algorithm) end it "returns the same hash for the same password and salt" do @@ -1875,6 +1875,44 @@ RSpec.describe User do it "raises an error when passwords are too long" do expect { hash(too_long, "gravy") }.to raise_error(StandardError) end + + it "uses the target algorithm for new users" do + expect(user.password_algorithm).to eq(User::TARGET_PASSWORD_ALGORITHM) + end + + it "can use an older algorithm to verify existing passwords, then upgrade" do + old_algorithm = "$pbkdf2-sha256$i=5,l=32$" + expect(old_algorithm).not_to eq(User::TARGET_PASSWORD_ALGORITHM) + + password = "poutine" + old_hash = hash(password, user.salt, old_algorithm) + + user.update!(password_algorithm: old_algorithm, password_hash: old_hash) + + expect(user.password_algorithm).to eq(old_algorithm) + expect(user.password_hash).to eq(old_hash) + + # With an incorrect attempt, should return false with no side effects + expect(user.confirm_password?("notthepassword")).to eq(false) + expect(user.password_algorithm).to eq(old_algorithm) + expect(user.password_hash).to eq(old_hash) + + # Should correctly verify against old algorithm + expect(user.confirm_password?(password)).to eq(true) + + # Auto-upgrades to new algorithm + expected_new_hash = hash(password, user.salt, User::TARGET_PASSWORD_ALGORITHM) + expect(user.password_algorithm).to eq(User::TARGET_PASSWORD_ALGORITHM) + expect(user.password_hash).to eq(expected_new_hash) + + # And persists to the db + user.reload + expect(user.password_algorithm).to eq(User::TARGET_PASSWORD_ALGORITHM) + expect(user.password_hash).to eq(expected_new_hash) + + # And can still log in + expect(user.confirm_password?(password)).to eq(true) + end end describe "automatic group membership" do