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

@@ -263,12 +263,10 @@ module SecondFactorManager
   end
 
   def hash_backup_code(code, salt)
-    Pbkdf2.hash_password(
+    # Backup codes have high entropy, so we can afford to use
-      code,
+    # a lower number of iterations than for user-specific passwords
-      salt,
+    iterations = Rails.env.test? ? 10 : 64_000
-      Rails.configuration.pbkdf2_iterations,
+    Pbkdf2.hash_password(code, salt, iterations, "sha256")
-      Rails.configuration.pbkdf2_algorithm,
-    )
   end
 
   def require_rotp

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
+    return false unless password_hash && salt && password_algorithm
-    self.password_hash == hash_password(password, salt)
+    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.salt = SecureRandom.hex(PASSWORD_SALT_LENGTH)
-      self.password_hash = hash_password(@raw_password, salt)
+      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(
+    PasswordHasher.hash_password(password: password, salt: salt, algorithm: algorithm)
-      password,
-      salt,
-      Rails.configuration.pbkdf2_iterations,
-      Rails.configuration.pbkdf2_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
 #

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

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

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

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

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)
+    def hash(password, salt, algorithm = User::TARGET_PASSWORD_ALGORITHM)
-      User.new.send(:hash_password, password, salt)
+      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