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.

3 files changed, 31 insertions, 10 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index fc1e5516f8..8a76e011c1 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*
+    
 *   Add default option to module and class attribute accessors.
 
         mattr_accessor :settings, default: {}
diff --git a/activesupport/lib/active_support/security_utils.rb b/activesupport/lib/active_support/security_utils.rb
index b655d64449..5f5d4faa60 100644
--- a/activesupport/lib/active_support/security_utils.rb
+++ b/activesupport/lib/active_support/security_utils.rb
@@ -2,14 +2,12 @@ require "digest"
 
 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}"
 
@@ -17,11 +15,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 e8f762da22..8d7f7d699a 100644
--- a/activesupport/test/security_utils_test.rb
+++ b/activesupport/test/security_utils_test.rb
@@ -11,4 +11,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