1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

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.
This commit is contained in:
Vipul A M 2016-04-12 02:41:06 +05:30
parent ac8b79d553
commit fa487763d9
No known key found for this signature in database
GPG key ID: 4C9362FE1F574587
5 changed files with 37 additions and 19 deletions

View file

@ -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

View file

@ -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

View file

@ -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: {}

View file

@ -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

View file

@ -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