From: Tom Hughes <tom@compton.nu>
Date: Wed, 27 Oct 2021 18:11:26 +0000 (+0100)
Subject: Switch to Argon2 for password hashing
X-Git-Tag: live~1496^2
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/aad81eb74c9e6d70971f938da542271b8a70b638?ds=sidebyside

Switch to Argon2 for password hashing
---

diff --git a/Gemfile b/Gemfile
index 17f6cc791..15348a989 100644
--- a/Gemfile
+++ b/Gemfile
@@ -33,6 +33,9 @@ gem "autoprefixer-rails"
 # Use image_optim to optimise images
 gem "image_optim_rails"
 
+# Use argon2 for password hashing
+gem "argon2"
+
 # Load rails plugins
 gem "actionpack-page_caching", ">= 1.2.0"
 gem "activerecord-import"
diff --git a/Gemfile.lock b/Gemfile.lock
index 42a6a3524..9c46a8653 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -73,6 +73,9 @@ GEM
     annotate (3.1.1)
       activerecord (>= 3.2, < 7.0)
       rake (>= 10.4, < 14.0)
+    argon2 (2.1.1)
+      ffi (~> 1.14)
+      ffi-compiler (~> 1.0)
     ast (2.4.2)
     autoprefixer-rails (10.3.3.0)
       execjs (~> 2)
@@ -225,6 +228,9 @@ GEM
     faraday-patron (1.0.0)
     faraday-rack (1.0.0)
     ffi (1.15.4)
+    ffi-compiler (1.0.1)
+      ffi (>= 1.0.0)
+      rake
     ffi-libarchive (1.1.3)
       ffi (~> 1.0)
     fspath (3.1.2)
@@ -498,6 +504,7 @@ DEPENDENCIES
   active_record_union
   activerecord-import
   annotate
+  argon2
   autoprefixer-rails
   aws-sdk-s3
   better_errors
diff --git a/lib/password_hash.rb b/lib/password_hash.rb
index afea82c11..de1f20d31 100644
--- a/lib/password_hash.rb
+++ b/lib/password_hash.rb
@@ -1,51 +1,44 @@
-require "securerandom"
-require "openssl"
+require "argon2"
 require "base64"
 require "digest/md5"
+require "openssl"
+require "securerandom"
 
 module PasswordHash
-  SALT_BYTE_SIZE = 32
-  HASH_BYTE_SIZE = 32
-  PBKDF2_ITERATIONS = 10000
-  DIGEST_ALGORITHM = "sha512".freeze
+  FORMAT = Argon2::HashFormat.new(Argon2::Password.create(""))
 
   def self.create(password)
-    salt = SecureRandom.base64(SALT_BYTE_SIZE)
-    hash = self.hash(password, salt, PBKDF2_ITERATIONS, HASH_BYTE_SIZE, DIGEST_ALGORITHM)
-    [hash, [DIGEST_ALGORITHM, PBKDF2_ITERATIONS, salt].join("!")]
+    hash = Argon2::Password.create(password)
+    [hash, nil]
   end
 
   def self.check(hash, salt, candidate)
-    if salt.nil?
-      candidate = Digest::MD5.hexdigest(candidate)
+    if Argon2::HashFormat.valid_hash?(hash)
+      Argon2::Password.verify_password(candidate, hash)
+    elsif salt.nil?
+      hash == Digest::MD5.hexdigest(candidate)
     elsif salt.include?("!")
       algorithm, iterations, salt = salt.split("!")
       size = Base64.strict_decode64(hash).length
-      candidate = self.hash(candidate, salt, iterations.to_i, size, algorithm)
+      hash == pbkdf2(candidate, salt, iterations.to_i, size, algorithm)
     else
-      candidate = Digest::MD5.hexdigest(salt + candidate)
+      hash == Digest::MD5.hexdigest(salt + candidate)
     end
-
-    hash == candidate
   end
 
-  def self.upgrade?(hash, salt)
-    if salt.nil?
-      return true
-    elsif salt.include?("!")
-      algorithm, iterations, salt = salt.split("!")
-      return true if Base64.strict_decode64(salt).length != SALT_BYTE_SIZE
-      return true if Base64.strict_decode64(hash).length != HASH_BYTE_SIZE
-      return true if iterations.to_i != PBKDF2_ITERATIONS
-      return true if algorithm != DIGEST_ALGORITHM
-    else
-      return true
-    end
+  def self.upgrade?(hash, _salt)
+    format = Argon2::HashFormat.new(hash)
 
-    false
+    format.variant != FORMAT.variant ||
+      format.version != FORMAT.version ||
+      format.t_cost != FORMAT.t_cost ||
+      format.m_cost != FORMAT.m_cost ||
+      format.p_cost != FORMAT.p_cost
+  rescue Argon2::ArgonHashFail
+    true
   end
 
-  def self.hash(password, salt, iterations, size, algorithm)
+  def self.pbkdf2(password, salt, iterations, size, algorithm)
     digest = OpenSSL::Digest.new(algorithm)
     pbkdf2 = OpenSSL::PKCS5.pbkdf2_hmac(password, salt, iterations, size, digest)
     Base64.strict_encode64(pbkdf2)
diff --git a/test/lib/password_hash_test.rb b/test/lib/password_hash_test.rb
index 1440b35c4..54450b186 100644
--- a/test/lib/password_hash_test.rb
+++ b/test/lib/password_hash_test.rb
@@ -25,14 +25,27 @@ class PasswordHashTest < ActiveSupport::TestCase
     assert PasswordHash.check("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A=", "password")
     assert_not PasswordHash.check("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A=", "wrong")
     assert_not PasswordHash.check("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtMwronguvanFT5/WtWaCwdOdrir8QOtFwxhO0A=", "password")
-    assert_not PasswordHash.upgrade?("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A=")
+    assert PasswordHash.upgrade?("3wYbPiOxk/tU0eeIDjUhdvi8aDP3AbFtwYKKxF1IhGg=", "sha512!10000!OUQLgtM7eD8huvanFT5/WtWaCwdOdrir8QOtFwxhO0A=")
+  end
+
+  def test_argon2_upgradeable
+    assert PasswordHash.check("$argon2id$v=19$m=65536,t=1,p=1$KXGHWfWMf5H5kY4uU3ua8A$YroVvX6cpJpljTio62k19C6UpuIPtW7me2sxyU2dyYg", nil, "password")
+    assert_not PasswordHash.check("$argon2id$v=19$m=65536,t=1,p=1$KXGHWfWMf5H5kY4uU3ua8A$YroVvX6cpJpljTio62k19C6UpuIPtW7me2sxyU2dyYg", nil, "wrong")
+    assert PasswordHash.upgrade?("$argon2id$v=19$m=65536,t=1,p=1$KXGHWfWMf5H5kY4uU3ua8A$YroVvX6cpJpljTio62k19C6UpuIPtW7me2sxyU2dyYg", nil)
+  end
+
+  def test_argon2
+    assert PasswordHash.check("$argon2id$v=19$m=65536,t=2,p=1$b2E7zSvjT6TC5DXrqvfxwg$P4hly807ckgYc+kfvaf3rqmJcmKStzw+kV14oMaz8PQ", nil, "password")
+    assert_not PasswordHash.check("$argon2id$v=19$m=65536,t=2,p=1$b2E7zSvjT6TC5DXrqvfxwg$P4hly807ckgYc+kfvaf3rqmJcmKStzw+kV14oMaz8PQ", nil, "wrong")
+    assert_not PasswordHash.upgrade?("$argon2id$v=19$m=65536,t=2,p=1$b2E7zSvjT6TC5DXrqvfxwg$P4hly807ckgYc+kfvaf3rqmJcmKStzw+kV14oMaz8PQ", nil)
   end
 
   def test_default
     hash1, salt1 = PasswordHash.create("password")
     hash2, salt2 = PasswordHash.create("password")
     assert_not_equal hash1, hash2
-    assert_not_equal salt1, salt2
+    assert_nil salt1
+    assert_nil salt2
     assert PasswordHash.check(hash1, salt1, "password")
     assert_not PasswordHash.check(hash1, salt1, "wrong")
     assert PasswordHash.check(hash2, salt2, "password")
@@ -40,4 +53,12 @@ class PasswordHashTest < ActiveSupport::TestCase
     assert_not PasswordHash.upgrade?(hash1, salt1)
     assert_not PasswordHash.upgrade?(hash2, salt2)
   end
+
+  def test_format
+    hash, _salt = PasswordHash.create("password")
+    format = Argon2::HashFormat.new(hash)
+
+    assert_equal "argon2id", format.variant
+    assert format.version <= 19
+  end
 end