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.

2 files changed, 6 insertions, 9 deletions

diff --git a/actionpack/lib/action_controller/metal/http_authentication.rb b/actionpack/lib/action_controller/metal/http_authentication.rb
index d8bc895265..09df39db1f 100644
--- a/actionpack/lib/action_controller/metal/http_authentication.rb
+++ b/actionpack/lib/action_controller/metal/http_authentication.rb
@@ -70,10 +70,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
@@ -348,10 +348,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
diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb
index 5051c02a62..13662fc021 100644
--- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb
+++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -353,7 +353,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:
@@ -364,7 +364,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