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

Make variable_size_secure_compare public

actionpack/lib/action_controller/metal/http_authentication.rb

@@ -72,10 +72,10 @@ module ActionController
             before_action(options.except(:name, :password, :realm)) do
               authenticate_or_request_with_http_basic(options[:realm] || "Application") do |name, password|
                 # This comparison uses & so that it doesn't short circuit and
-                # uses `variable_size_secure_compare` so that length information
+                # uses `secure_compare` so that length information
                 # isn't leaked.
-                ActiveSupport::SecurityUtils.variable_size_secure_compare(name, options[:name]) &
-                  ActiveSupport::SecurityUtils.variable_size_secure_compare(password, options[:password])
+                ActiveSupport::SecurityUtils.secure_compare(name, options[:name]) &
+                  ActiveSupport::SecurityUtils.secure_compare(password, options[:password])
               end
             end
           end
@@ -350,10 +350,7 @@ module ActionController
     #         authenticate_or_request_with_http_token do |token, options|
     #           # Compare the tokens in a time-constant manner, to mitigate
     #           # timing attacks.
-    #           ActiveSupport::SecurityUtils.secure_compare(
-    #             ::Digest::SHA256.hexdigest(token),
-    #             ::Digest::SHA256.hexdigest(TOKEN)
-    #           )
+    #           ActiveSupport::SecurityUtils.secure_compare(token, TOKEN)
     #         end
     #       end
     #   end

actionpack/lib/action_controller/metal/request_forgery_protection.rb

@@ -369,7 +369,7 @@ module ActionController #:nodoc:
       end
 
       def compare_with_real_token(token, session) # :doc:
-        ActiveSupport::SecurityUtils.secure_compare(token, real_csrf_token(session))
+        ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, real_csrf_token(session))
       end
 
       def valid_per_form_csrf_token?(token, session) # :doc:
@@ -380,7 +380,7 @@ module ActionController #:nodoc:
             request.request_method
           )
 
-          ActiveSupport::SecurityUtils.secure_compare(token, correct_token)
+          ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, correct_token)
         else
           false
         end

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`.
 

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

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