Merge pull request #2290 from coreinfrastructure/hmac_runtime_passwords

Hmac runtime passwords

Author: david-a-wheeler

app/models/bad_password.rb

@@ -3,27 +3,82 @@
 # Copyright the OpenSSF Best Practices badge contributors
 # SPDX-License-Identifier: MIT
 
-# This is a simple list of records with column "forbidden" of all
-# "known bad passwords". There's not anything to it; ActiveRecord handles it.
+# This is a simple list of HMAC (keyed cryptographically hashed) records with
+# column "forbidden_hash" of all "lowercased known bad passwords".
+# There's not much to it; ActiveRecord handles quickly seeing if it exists.
+
+# In *production*, make sure you set 'BADGEAPP_BADPWKEY' to a secret key
+# and initialize this database table.
+# You can (re)initialize this database table at any time by running:
+# rake update_bad_password_db
 
 class BadPassword < ApplicationRecord
+  # Should we do the bad password lookups?
+  # We will do a lookup in the test environment *or* if the log level
+  # is not debug log level (level 0).
+  # The ||= is because Rails reloads. It's okay if it recalculates this
+  # twice if it's false, it'll produce the same result each time.
+  DO_LOOKUPS ||= Rails.env.test? || (Rails.logger.level != 0)
+
+  # Provide warning if we are NOT actually doing the bad password lookups.
+  # We want to make sure we avoid logging those lookups, by not doing them,
+  # but we want to warn that it's happening.
+  Rails.logger.info('Bad password lookups disabled') unless DO_LOOKUPS
+
+  # Use this key in HMAC to encrypt and cryptographically hash
+  # the 'bad passwords' and any password used to
+  # compare them. As a result, at runtime the database only sees HMACs
+  # of passwords, never passwords, when comparing to the bad password table.
+  # This isn't necessary for security; passwords are only
+  # *stored* in bcrypt format. However, if an attacker manages to read
+  # communication lines from the app to the *running* database system,
+  # or control the running database system itself,
+  # this measure will ensure that the attacker never gets
+  # access to unencrypted passwords.
+  # In an effort to be max-hard on
+  # attackers, we'll use a keyed cryptographic hash, and whine when we don't
+  # get a key. Then an attacker can only get a keyed HMAC, even if
+  # they can see the database communication in real time (including via a
+  # log of SQL queries). Even a rainbow table won't help if the key isn't known.
+  # We *allow* execution with a known key, because that way we can run tests
+  # or do interactive development without having to rebuild the database
+  # each time.
+  # We use SHA-512, so 128 bytes (512 bits) of key is recommended.
+  # Here's one way to create a key: openssl rand -hex 128
+  DEFAULT_BADPWKEY ||= 'a5' * 128 # For testing and development
+  BADPWKEY ||= ENV['BADGEAPP_BADPWKEY'] || DEFAULT_BADPWKEY
+  Rails.logger.info('BADGEAPP_BADPWKEY unset') if BADPWKEY == DEFAULT_BADPWKEY
+  BADPWKEY_BYTES ||= [BADPWKEY].pack('H*')
+
+  # Return string representation of HMAC of pw (password)
+  def self.hash_password(pw)
+    OpenSSL::HMAC.hexdigest('SHA512', BADPWKEY_BYTES, pw)
+  end
+
+  # Load "bad passwords" from a file, up to "max" count.
   # rubocop:disable Metrics/MethodLength
-  def self.bad_passwords_from_file
+  def self.bad_passwords_from_file(max = nil)
     require 'zlib'
     bad_password_array = []
+    count = 0
     Zlib::GzipReader.open('raw-bad-passwords-lowercase.txt.gz') do |gz|
       gz.each_line do |line|
-        bad_password_array.push({ forbidden: line.chomp.downcase.freeze })
+        pw = line.chomp.downcase.freeze
+        hashed_pw = hash_password(pw)
+        bad_password_array.push({ forbidden_hash: hashed_pw })
+        count += 1
+        break unless max.nil? || max < count
       end
     end
     bad_password_array
   end
   # rubocop:enable Metrics/MethodLength
 
-  # Force load into the database the list of bad passwords.
-  def self.force_load
+  # Force load into the database the list of bad passwords, up to max number.
+  # We have a "max" so that testing doesn't take forever.
+  def self.force_load(max = nil)
     BadPassword.delete_all
-    bad_password_array = bad_passwords_from_file
+    bad_password_array = bad_passwords_from_file(max)
     # Update all in one transaction, or it will take a *long* time
     transaction do
       # TODO: Speed this up with Rails 6's "insert_all!" (bulk insert)
@@ -47,21 +102,16 @@ def self.force_load
   # global logging object is *not* okay. If we simply silenced logging,
   # we would sometimes disable logging of other events.
   # Since we only check for unlogged passwords as an extra help for users,
-  # losing this isn't a problem.
-
-  # Should we do the bad password lookups?
-  # We will do a lookup in the test environment *or* if the log level
-  # is not debug log level (level 0).
-  # The ||= is because Rails reloads. It's okay if it recalculates this
-  # twice if it's false, it'll produce the same result each time.
-  DO_LOOKUPS ||= Rails.env.test? || (Rails.logger.level != 0)
-
-  # Provide warning if we are NOT actually doing the bad password lookups.
-  # We want to make sure we avoid logging those lookups, by not doing them,
-  # but we want to warn that it's happening.
-  Rails.logger.info('Bad password lookups disabled') unless DO_LOOKUPS
+  # losing this functionality isn't a serious problem.
+  # It's better to not check for bad passwords to ensure *no* password
+  # is ever exposed.
+  #
+  # Note that we only checked keyed cryptographic hashes. That way, even
+  # if an attacker manages to see the queries, they'll also need the key
+  # before they can *start* doing brute-force searches for a password.
 
-  def self.unlogged_exists?(forbidden)
-    DO_LOOKUPS && exists?(forbidden)
+  def self.unlogged_exists?(pw)
+    pw_hash = hash_password(pw.downcase)
+    DO_LOOKUPS && exists?(forbidden_hash: pw_hash)
   end
 end

app/validators/password_validator.rb

@@ -6,7 +6,7 @@
 
 class PasswordValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
-    return unless BadPassword.unlogged_exists?(forbidden: value.downcase)
+    return unless BadPassword.unlogged_exists?(value)
 
     record.errors.add attribute,
                       options[:message] ||

db/migrate/20250222215212_rename_forbidden_to_forbidden_hash_in_bad_passwords.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class RenameForbiddenToForbiddenHashInBadPasswords < ActiveRecord::Migration[8.0]
+  def change
+    # Because we're *renaming* a column, the index will continue to exist
+    rename_column :bad_passwords, 'forbidden', 'forbidden_hash'
+    puts 'Remember to run: rake update_bad_password_db'
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2024_12_27_211821) do
+ActiveRecord::Schema[8.0].define(version: 2025_02_22_215212) do
   create_schema "heroku_ext"
 
   # These are extensions that must be enabled in order to support this database
@@ -29,8 +29,8 @@
   end
 
   create_table "bad_passwords", id: false, force: :cascade do |t|
-    t.string "forbidden"
-    t.index ["forbidden"], name: "index_bad_passwords_on_forbidden"
+    t.string "forbidden_hash"
+    t.index ["forbidden_hash"], name: "index_bad_passwords_on_forbidden_hash"
   end
 
   create_table "pg_search_documents", id: :serial, force: :cascade do |t|

docs/assurance-case.md

@@ -543,7 +543,7 @@ We *do* consider this data higher-value and protect them specially.
 
 User passwords for local accounts are only stored on the server as
 iterated per-user salted hashes (using bcrypt), and thus cannot
-be retrieved in an unencrypted form.
+be retrieved in an unencrypted form later on from a recorded database.
 
 Any user password sent to the system is sent by the application router
 to the "update" method of the user controller
@@ -563,6 +563,44 @@ until that memory is recycled, but we assume that the underlying
 system will protect memory during the time before it is garbage-collected
 and reused.
 
+To help users avoid the *worst* passwords, we check proposed passwords
+against a list of bad passwords and don't allow passwords to change to them.
+This complies with NIST Special Publication 800-63B section 5.1.1.2.
+That list is long, so for speed we keep this list in the database
+(if we kept it in memory it would use a lot of memory).
+We don't trust long-term storage, so it's vital that we *never* record
+in logs a query that includes an unencrypted password.
+Also, while we trust our database and its connections, we want to limit
+privileges where we can. Thus, we take two steps to protect passwords
+while we check them against the bad password list in the database:
+
+1. We disable the bad password check if the log level is set to debug
+   (which logs SQL queries) and we aren't running a test.
+   It's better to disable the bad password check than to risk exposing
+   a password. When this occurs, a warning it noted in the log.
+   Note that the production environments settings do *not* use log level
+   debug by default anyway, but we want to make sure it won't happen.
+2. Instead of storing the bad passwords directly, we stored keyed HMAC
+   values of them, and the new password must *also* be converted into a
+   keyed HMAC for comparison. The key is a secret for the application.
+   This means that even if an attacker gains control over the database
+   and/or can read communications to it (which we assume they can't do),
+   the attacker would have to use a brute-force to find the key
+   (we recommend 512 bits), *then* use that key to compute HMACs to attempt
+   to discover a user's password. Rekeying the bad password database
+   is trivial, when desired. A new key can be set in `BADGEAPP_BADPWKEY`
+   and then you can run `rake update_bad_password_db` (it takes a few minutes).
+   We do this to protect passwords used for queries between the application
+   and the database, to counter someone snooping it.
+   We *never* store the HMAC of the password anywhere. We instead use
+   bcrypt for stored passwords (it's designed for that).
+   Strictly speaking using HMAC isn't necessary, since we don't store it,
+   but providing another layer of protections for passwords seemed appropriate.
+   Theoretically a user could choose another password that isn't
+   on the bad password list but matches its HMAC; that's so astronomically
+   unlikely that it won't happen, and even if it did, it would just mean
+   that the user would have to choose a different password.
+
 #### Remember me token
 
 Users may choose to "remember me" to automatically re-login on
@@ -1946,9 +1984,9 @@ as of 2015-12-14:
 6. *User management.*
    Local passwords have a minimum length (8) and cannot be
    a member of a set of known-bad passwords.  We allow much longer passwords.
-   This complies with draft NIST Special Publication 800-63B,
+   This complies with NIST Special Publication 800-63B,
    "Digital Authentication Guideline: Authentication and Lifecycle Management"
-   dated Thu, 24 Nov 2016 08:15:51 -0500 <https://pages.nist.gov/800-63-3/>.
+   <https://pages.nist.gov/800-63-3/sp800-63b.html> section 5.1.1.2.
    We expect users to
    protect their own passwords; we do not try to protect users from themselves.
    The system is not fast enough for a naive password-guesser to succeed

docs/implementation.md

@@ -93,6 +93,14 @@ The application is configured by various environment variables:
   Used by aes-256-gcm (256-bit AES in GCM mode).
 * EMAIL_BLIND_INDEX_KEY: Key for blind index created for user email addresses
   (used by PBKDF2-HMAC-SHA256).  Must be 64 hex digits (==32 bytes==256 bits).
+* BADGEAPP_BADPWKEY: Key used for additional protection for passwords
+  when checking the bad password database. The list of bad passwords
+  is converted by HMAC (using SHA-512 and this key) before being stored in
+  the database, and the same happens to passwords before the database is
+  asked to compare them. We recommend setting this to a cryptographically
+  random 512-bit value; you can get that with `openssl rand -hex 128` or
+  similar. Then run `rake update_bad_password_db` to load the database
+  with the bad password list (it will take a few minutes to load them all).
 * BADGEAPP_DENY_LOGIN: If a non-blank value is set ("true" is recommended),
   then no on can log in, no one can create a new account (sign up),
   and no one can do anything that requires being logged (users are always

test/fixtures/bad_passwords.yml

@@ -1,8 +1,11 @@
 # Read about fixtures at http://api.rubyonrails.org/classes/ActiveRecord/FixtureSet.html
+# We presume tests use the default BADPWKEY (that's why it's the default).
 
-# World's worst password
 one:
-  forbidden: "password"
+  # World's worst password
+  # BadPassword::hash_password("password")
+  forbidden_hash: "587a7deba2325735fbbfbba4a6797c5d3adbbdc5f4a37d516fcae5ba31210f0dadc3cbf3bac0b4230d910a054f24577625532226f6c1a047b7ffe3e03c19b184"
 
 two:
-  forbidden: "123456"
+  # BadPassword::hash_password("123456")
+  forbidden_hash: "5cc3782528f34852f90f130cad8e55e75a5aec78bbea4331eb6f2008cedfc399de70acc085dac4fc14a7e2eacaced720eb6fdcc9d3d71fd4c951dee2e92281f0"

test/models/bad_password_test.rb

@@ -7,19 +7,18 @@
 
 class BadPasswordTest < ActiveSupport::TestCase
   test 'Bad password found in BadPassword' do
-    assert BadPassword.exists?(forbidden: '123456')
+    assert BadPassword.unlogged_exists?('123456')
   end
 
   test 'Good password not found in BadPassword' do
-    assert_not BadPassword.exists?(forbidden: 'asjfdksajdklfajdfkjaslkdfj')
-  end
-
-  test 'Ensure that we have small bad password list for tests' do
-    assert_not BadPassword.exists?(forbidden: '10101010')
+    assert_not BadPassword.unlogged_exists?('asjfdksajdklfajdfkjaslkdfj')
   end
 
+  # Running BadPassword.force_load without limit would take a *long* time.
+  # For testing purposes, we load a few values and make sure it's found.
   test 'Load full bad password list' do
     BadPassword.force_load
-    assert BadPassword.exists?(forbidden: '10101010')
+    assert BadPassword.unlogged_exists?('10101010')
+    assert_not BadPassword.unlogged_exists?('A_decent_pass_Maybe?_581905012')
   end
 end