Merge pull request #24510 from vipulnsward/make-variable_size_secure_compare-public

Make variable_size_secure_compare public

3 files changed, 31 insertions, 10 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 3257c63fd2..b6eb64c1c9 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Changed default behaviour of `ActiveSupport::SecurityUtils.secure_compare`,
+    to make it not leak length information even for variable length string.
+
+    Renamed old `ActiveSupport::SecurityUtils.secure_compare` to `fixed_length_secure_compare`,
+    and started raising `ArgumentError` in case of length mismatch of passed strings.
+
+    *Vipul A M*
+
 *   Make `ActiveSupport::TimeZone.all` return only time zones that are in
     `ActiveSupport::TimeZone::MAPPING`.
 
diff --git a/activesupport/lib/active_support/security_utils.rb b/activesupport/lib/active_support/security_utils.rb
index b6b31ef140..4d129bfe41 100644
--- a/activesupport/lib/active_support/security_utils.rb
+++ b/activesupport/lib/active_support/security_utils.rb
@@ -4,14 +4,12 @@ require "digest/sha2"
 
 module ActiveSupport
   module SecurityUtils
-    # Constant time string comparison.
+    # Constant time string comparison, for fixed length strings.
     #
     # The values compared should be of fixed length, such as strings
-    # that have already been processed by HMAC. This should not be used
-    # on variable length plaintext strings because it could leak length info
-    # via timing attacks.
-    def secure_compare(a, b)
-      return false unless a.bytesize == b.bytesize
+    # that have already been processed by HMAC. Raises in case of length mismatch.
+    def fixed_length_secure_compare(a, b)
+      raise ArgumentError, "string length mismatch." unless a.bytesize == b.bytesize
 
       l = a.unpack "C#{a.bytesize}"
 
@@ -19,11 +17,15 @@ module ActiveSupport
       b.each_byte { |byte| res |= byte ^ l.shift }
       res == 0
     end
-    module_function :secure_compare
+    module_function :fixed_length_secure_compare
 
-    def variable_size_secure_compare(a, b) # :nodoc:
-      secure_compare(::Digest::SHA256.hexdigest(a), ::Digest::SHA256.hexdigest(b))
+    # Constant time string comparison, for variable length strings.
+    #
+    # The values are first processed by SHA256, so that we don't leak length info
+    # via timing attacks.
+    def secure_compare(a, b)
+      fixed_length_secure_compare(::Digest::SHA256.hexdigest(a), ::Digest::SHA256.hexdigest(b))
     end
-    module_function :variable_size_secure_compare
+    module_function :secure_compare
   end
 end
diff --git a/activesupport/test/security_utils_test.rb b/activesupport/test/security_utils_test.rb
index efd2bcfa0f..6945f653e6 100644
--- a/activesupport/test/security_utils_test.rb
+++ b/activesupport/test/security_utils_test.rb
@@ -13,4 +13,15 @@ class SecurityUtilsTest < ActiveSupport::TestCase
     assert ActiveSupport::SecurityUtils.variable_size_secure_compare("a", "a")
     assert_not ActiveSupport::SecurityUtils.variable_size_secure_compare("a", "b")
   end
+
+  def test_fixed_length_secure_compare_should_perform_string_comparison
+    assert ActiveSupport::SecurityUtils.fixed_length_secure_compare("a", "a")
+    assert !ActiveSupport::SecurityUtils.fixed_length_secure_compare("a", "b")
+  end
+
+  def test_fixed_length_secure_compare_raise_on_length_mismatch
+    assert_raises(ArgumentError, "string length mismatch.") do
+      ActiveSupport::SecurityUtils.fixed_length_secure_compare("a", "ab")
+    end
+  end
 end