From d0883f9018c4cecdf4cd301d839a7929e11c0278 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Mon, 4 May 2026 17:20:57 -0600 Subject: [PATCH] fix(auth): hash MFA backup codes (#1629) * fix(auth): hash MFA backup codes * fix(auth): lock and filter backup code verification * test(auth): assert consumed backup code digest * fix(auth): strengthen backup code handling * fix(auth): require otp secret before mfa enable * test(auth): assert backup code digest consumption * fix(auth): rehash legacy MFA backup codes * fix(auth): narrow legacy backup code migration --- app/controllers/mfa_controller.rb | 3 +- app/models/user.rb | 96 +++++++++++++++---- app/views/mfa/verify.html.erb | 2 +- ...80000_rehash_plaintext_mfa_backup_codes.rb | 51 ++++++++++ db/schema.rb | 2 +- test/controllers/mfa_controller_test.rb | 13 ++- test/models/user_test.rb | 92 +++++++++++++++++- 7 files changed, 230 insertions(+), 29 deletions(-) create mode 100644 db/migrate/20260503180000_rehash_plaintext_mfa_backup_codes.rb diff --git a/app/controllers/mfa_controller.rb b/app/controllers/mfa_controller.rb index 568d0f103..51154ac51 100644 --- a/app/controllers/mfa_controller.rb +++ b/app/controllers/mfa_controller.rb @@ -11,8 +11,7 @@ class MfaController < ApplicationController def create if Current.user.verify_otp?(params[:code]) - Current.user.enable_mfa! - @backup_codes = Current.user.otp_backup_codes + @backup_codes = Current.user.enable_mfa! render :backup_codes else Current.user.disable_mfa! diff --git a/app/models/user.rb b/app/models/user.rb index ee51c97d1..9f7f64d80 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -9,9 +9,6 @@ class User < ApplicationRecord if encryption_ready? # MFA secrets encrypts :otp_secret, deterministic: true - # Note: otp_backup_codes is a PostgreSQL array column which doesn't support - # AR encryption. To encrypt it, a migration would be needed to change the - # column type from array to text/jsonb. # PII - emails (deterministic for lookups, downcase for case-insensitive) encrypts :email, deterministic: true, downcase: true @@ -40,6 +37,8 @@ class User < ApplicationRecord has_many :shared_accounts, through: :account_shares, source: :account accepts_nested_attributes_for :family, update_only: true + MFA_BACKUP_CODE_COUNT = 8 + validates :email, presence: true, uniqueness: true, format: { with: URI::MailTo::EMAIL_REGEXP } validate :ensure_valid_profile_image validates :default_period, inclusion: { in: Period::PERIODS.keys } @@ -221,10 +220,17 @@ class User < ApplicationRecord end def enable_mfa! + raise ArgumentError, "OTP secret must be set before enabling MFA" if otp_secret.blank? + + backup_codes = generate_backup_codes + + # Store bcrypt digests only; this Postgres array cannot use AR encryption. update!( otp_required: true, - otp_backup_codes: generate_backup_codes + otp_backup_codes: backup_codes.map { |code| digest_backup_code(code) } ) + + backup_codes end def disable_mfa! @@ -240,8 +246,13 @@ class User < ApplicationRecord def verify_otp?(code) return false if otp_secret.blank? - return true if verify_backup_code?(code) - totp.verify(code, drift_behind: 15) + + normalized_code = normalize_mfa_code(code) + return false if normalized_code.blank? + return true if totp.verify(normalized_code, drift_behind: 15) + return false unless backup_code_input?(normalized_code) + + consume_backup_code!(normalized_code) end def provisioning_uri @@ -464,21 +475,72 @@ class User < ApplicationRecord ROTP::TOTP.new(otp_secret, issuer: "Sure Finances") end - def verify_backup_code?(code) - return false if otp_backup_codes.blank? + def consume_backup_code!(normalized_code) + consumed = false - # Find and remove the used backup code - if (index = otp_backup_codes.index(code)) - remaining_codes = otp_backup_codes.dup - remaining_codes.delete_at(index) - update!(otp_backup_codes: remaining_codes) - true - else - false + transaction do + lock! + + if otp_backup_codes.present? + matching_index = otp_backup_codes.index do |stored_code| + backup_code_matches?(stored_code, normalized_code) + end + + if matching_index + remaining_codes = otp_backup_codes.dup + remaining_codes.delete_at(matching_index) + update!(otp_backup_codes: remaining_codes) + consumed = true + end + end end + + consumed end def generate_backup_codes - 8.times.map { SecureRandom.hex(4) } + MFA_BACKUP_CODE_COUNT.times.map { SecureRandom.hex(8) } + end + + def digest_backup_code(code) + BCrypt::Password.create(normalize_mfa_code(code), cost: backup_code_digest_cost).to_s + end + + def backup_code_matches?(stored_code, normalized_code) + if backup_code_digest?(stored_code) + return false unless backup_code_input?(normalized_code) + + BCrypt::Password.new(stored_code).is_password?(normalized_code) + else + # Legacy plaintext codes are accepted once so existing MFA users are + # not locked out after backup-code hashing ships. + ActiveSupport::SecurityUtils.secure_compare(stored_code.to_s, normalized_code) + end + rescue BCrypt::Errors::InvalidHash + false + end + + def backup_code_digest?(stored_code) + stored_code.to_s.start_with?("$2a$", "$2b$", "$2y$") + end + + def normalize_mfa_code(code) + code.to_s.strip.downcase + end + + def backup_code_input?(code) + backup_code_candidate?(code) || legacy_plaintext_backup_code_candidate?(code) + end + + def backup_code_candidate?(code) + code.to_s.match?(/\A[0-9a-f]{16}\z/) + end + + def legacy_plaintext_backup_code_candidate?(code) + code.to_s.match?(/\A[0-9a-f]{8}\z/) + end + + def backup_code_digest_cost + ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost end end diff --git a/app/views/mfa/verify.html.erb b/app/views/mfa/verify.html.erb index bfb772345..8627d15e9 100644 --- a/app/views/mfa/verify.html.erb +++ b/app/views/mfa/verify.html.erb @@ -31,7 +31,7 @@ required: true, autofocus: true, autocomplete: "one-time-code", - type: "number", + type: "text", label: t(".page_title") %> <%= form.submit t(".verify_button") %> diff --git a/db/migrate/20260503180000_rehash_plaintext_mfa_backup_codes.rb b/db/migrate/20260503180000_rehash_plaintext_mfa_backup_codes.rb new file mode 100644 index 000000000..2987270b2 --- /dev/null +++ b/db/migrate/20260503180000_rehash_plaintext_mfa_backup_codes.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +class RehashPlaintextMfaBackupCodes < ActiveRecord::Migration[7.2] + class MigrationUser < ActiveRecord::Base + self.table_name = "users" + end + + BCRYPT_PREFIXES = %w[$2a$ $2b$ $2y$].freeze + PLAINTEXT_BACKUP_CODE_PATTERN = /\A[0-9a-f]{8}\z/ + + def up + require "bcrypt" + + say_with_time "Rehashing plaintext MFA backup codes" do + rehashed_users_count = 0 + + MigrationUser.where(otp_required: true).find_each do |user| + backup_codes = Array(user.otp_backup_codes) + next if backup_codes.blank? + next unless backup_codes.any? { |code| plaintext_backup_code?(code) } + + rehashed_codes = backup_codes.map do |code| + plaintext_backup_code?(code) ? BCrypt::Password.create(normalize_backup_code(code), cost: bcrypt_cost).to_s : code + end + + user.update_columns(otp_backup_codes: rehashed_codes, updated_at: Time.current) + rehashed_users_count += 1 + end + + rehashed_users_count + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end + + private + def plaintext_backup_code?(code) + normalized_code = normalize_backup_code(code) + normalized_code.match?(PLAINTEXT_BACKUP_CODE_PATTERN) && BCRYPT_PREFIXES.none? { |prefix| normalized_code.start_with?(prefix) } + end + + def normalize_backup_code(code) + code.to_s.strip.downcase + end + + def bcrypt_cost + ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost + end +end diff --git a/db/schema.rb b/db/schema.rb index 72858f535..1c11eea00 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_05_02_120000) do +ActiveRecord::Schema[7.2].define(version: 2026_05_03_180000) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" diff --git a/test/controllers/mfa_controller_test.rb b/test/controllers/mfa_controller_test.rb index f9b558afe..f43fe3555 100644 --- a/test/controllers/mfa_controller_test.rb +++ b/test/controllers/mfa_controller_test.rb @@ -39,7 +39,12 @@ class MfaControllerTest < ActionDispatch::IntegrationTest assert_response :success assert @user.reload.otp_required? assert_equal 8, @user.otp_backup_codes.length + assert @user.otp_backup_codes.all? { |code| code.start_with?("$2") } assert_select "div.grid-cols-2" # Check for backup codes grid + rendered_codes = css_select("div.grid-cols-2 div").map { |node| node.text.strip } + assert_equal 8, rendered_codes.length + assert rendered_codes.all? { |code| code.match?(/\A[0-9a-f]{16}\z/) } + assert_empty rendered_codes & @user.otp_backup_codes end test "does not enable MFA with invalid code" do @@ -96,17 +101,19 @@ class MfaControllerTest < ActionDispatch::IntegrationTest test "verify_code authenticates with valid backup code" do @user.setup_mfa! - @user.enable_mfa! + backup_code = @user.enable_mfa!.first + matching_digest = @user.otp_backup_codes.find { |digest| BCrypt::Password.new(digest).is_password?(backup_code) } + assert_not_nil matching_digest sign_out post sessions_path, params: { email: @user.email, password: user_password_test } - backup_code = @user.otp_backup_codes.first post verify_mfa_path, params: { code: backup_code } assert_redirected_to root_path assert Session.exists?(user_id: @user.id) - assert_not @user.reload.otp_backup_codes.include?(backup_code) + assert_equal 7, @user.reload.otp_backup_codes.size + assert_not_includes @user.otp_backup_codes, matching_digest end test "verify_code rejects invalid codes" do diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 8a44d57f0..9912e5a9e 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -91,11 +91,24 @@ class UserTest < ActiveSupport::TestCase test "enable_mfa! enables MFA and generates backup codes" do user = users(:family_member) user.setup_mfa! - user.enable_mfa! + backup_codes = user.enable_mfa! assert user.otp_required? + assert_equal 8, backup_codes.length + assert backup_codes.all? { |code| code.match?(/\A[0-9a-f]{16}\z/) } assert_equal 8, user.otp_backup_codes.length - assert user.otp_backup_codes.all? { |code| code.length == 8 } + assert user.otp_backup_codes.all? { |code| code.start_with?("$2") } + assert_empty backup_codes & user.otp_backup_codes + end + + test "enable_mfa! requires an OTP secret" do + user = users(:family_member) + user.setup_mfa! + user.update_column(:otp_secret, nil) + + assert_raises(ArgumentError) { user.enable_mfa! } + assert_not user.reload.otp_required? + assert_empty user.otp_backup_codes end test "disable_mfa! removes all MFA data" do @@ -155,22 +168,91 @@ class UserTest < ActiveSupport::TestCase assert_not user.verify_otp?("123456") end - test "verify_otp? accepts backup codes" do + test "verify_otp? does not check backup code digests for normal TOTP input" do + user = users(:family_member) + user.setup_mfa! + user.enable_mfa! + valid_code = ROTP::TOTP.new(user.otp_secret, issuer: "Sure Finances").now + + BCrypt::Password.expects(:new).never + + assert user.verify_otp?(valid_code) + end + + test "verify_otp? fast rejects non-backup-code input before digest checks" do user = users(:family_member) user.setup_mfa! user.enable_mfa! - backup_code = user.otp_backup_codes.first + BCrypt::Password.expects(:new).never + + assert_not user.verify_otp?("not-a-backup-code") + end + + test "verify_otp? rejects unmatched legacy-shaped backup input" do + user = users(:family_member) + user.setup_mfa! + user.enable_mfa! + + assert_not user.verify_otp?("deadbeef") + end + + test "verify_otp? accepts backup codes" do + user = users(:family_member) + user.setup_mfa! + backup_codes = user.enable_mfa! + + backup_code = backup_codes.first + matching_digest = user.otp_backup_codes.find { |digest| BCrypt::Password.new(digest).is_password?(backup_code) } + assert_not_nil matching_digest + assert user.verify_otp?(backup_code) # Backup code should be consumed - assert_not user.otp_backup_codes.include?(backup_code) assert_equal 7, user.otp_backup_codes.length + assert_not_includes user.otp_backup_codes, matching_digest # Used backup code should not work again assert_not user.verify_otp?(backup_code) end + test "verify_otp? reloads backup codes while consuming under lock" do + user = users(:family_member) + user.setup_mfa! + backup_code = user.enable_mfa!.first + stale_user = User.find(user.id) + + user.update!(otp_backup_codes: []) + + assert_not stale_user.verify_otp?(backup_code) + assert_empty stale_user.reload.otp_backup_codes + end + + test "verify_otp? accepts and consumes legacy plaintext backup codes once" do + user = users(:family_member) + user.setup_mfa! + user.update!(otp_required: true, otp_backup_codes: [ "deadbeef" ]) + + assert user.verify_otp?("deadbeef") + + assert_empty user.reload.otp_backup_codes + assert_not user.verify_otp?("deadbeef") + end + + test "verify_otp? accepts and consumes migrated legacy backup code digests" do + user = users(:family_member) + user.setup_mfa! + user.update!( + otp_required: true, + otp_backup_codes: [ BCrypt::Password.create("deadbeef", cost: BCrypt::Engine::MIN_COST).to_s ] + ) + + assert user.verify_otp?("deadbeef") + + assert_empty user.reload.otp_backup_codes + assert_not user.verify_otp?("deadbeef") + end + test "provisioning_uri generates correct URI" do user = users(:family_member) user.setup_mfa!