New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make variable_size_secure_compare public #24510

Merged
merged 1 commit into from Nov 25, 2017
Jump to file or symbol
Failed to load files and symbols.
+37 −19
Diff settings

Always

Just for now

@@ -70,10 +70,10 @@ def http_basic_authenticate_with(options = {})
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 @@ def opaque(secret_key)
# 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
@@ -353,7 +353,7 @@ def unmask_token(masked_token) # :doc:
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 @@ def valid_per_form_csrf_token?(token, session) # :doc:
request.request_method
)
ActiveSupport::SecurityUtils.secure_compare(token, correct_token)
ActiveSupport::SecurityUtils.fixed_length_secure_compare(token, correct_token)
else
false
end
@@ -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: {}
@@ -2,26 +2,28 @@
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)

This comment has been minimized.

@jeremy

jeremy Apr 11, 2016

Member

Should we make this public API, or is variable-length secure_compare sufficient?

@jeremy

jeremy Apr 11, 2016

Member

Should we make this public API, or is variable-length secure_compare sufficient?

This comment has been minimized.

@vipulnsward

vipulnsward Apr 11, 2016

Member

This allows others to make use of compare, without always using SHA256, for example, they may use HMAC.

@vipulnsward

vipulnsward Apr 11, 2016

Member

This allows others to make use of compare, without always using SHA256, for example, they may use HMAC.

raise ArgumentError, "string length mismatch." unless a.bytesize == b.bytesize
l = a.unpack "C#{a.bytesize}"
res = 0
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))

This comment has been minimized.

@matthewd

matthewd Jun 6, 2017

Member

Should we technically be doing a comparison of the actual strings afterwards, too? Collisions are incredibly unlikely, but are they close enough to impossible for us to outright ignore?

@matthewd

matthewd Jun 6, 2017

Member

Should we technically be doing a comparison of the actual strings afterwards, too? Collisions are incredibly unlikely, but are they close enough to impossible for us to outright ignore?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 7, 2017

Member

I think the probability is 2^-256, I'm not a cryptology but I think it is close enough to impossible.

@rafaelfranca

rafaelfranca Jun 7, 2017

Member

I think the probability is 2^-256, I'm not a cryptology but I think it is close enough to impossible.

This comment has been minimized.

@vipulnsward

vipulnsward Jun 8, 2017

Member

Technically we could, but == is not timing attack safe as it uses memcpy? Hence this hoop.

There is https://bugs.ruby-lang.org/issues/10098 ,but its not in ruby yet.

I see we do the same in rack as well: https://github.com/rack/rack/search?utf8=%E2%9C%93&q=secure_compare&type=

@vipulnsward

vipulnsward Jun 8, 2017

Member

Technically we could, but == is not timing attack safe as it uses memcpy? Hence this hoop.

There is https://bugs.ruby-lang.org/issues/10098 ,but its not in ruby yet.

I see we do the same in rack as well: https://github.com/rack/rack/search?utf8=%E2%9C%93&q=secure_compare&type=

This comment has been minimized.

@matthewd

matthewd Jun 8, 2017

Member

Timing is irrelevant if we've already proven they're SHA256-equal.

I'm just nervous about effectively adding || rand(N) == 0 onto arbitrary conditionals, even if that N is 2**256.

@matthewd

matthewd Jun 8, 2017

Member

Timing is irrelevant if we've already proven they're SHA256-equal.

I'm just nervous about effectively adding || rand(N) == 0 onto arbitrary conditionals, even if that N is 2**256.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 9, 2017

Member

If timing is irrelevant I don't see why not compare the string after it.

@rafaelfranca

rafaelfranca Jun 9, 2017

Member

If timing is irrelevant I don't see why not compare the string after it.

This comment has been minimized.

@vipulnsward

vipulnsward Jun 9, 2017

Member

Everywhere I see, everyone just relies on this definition:

https://github.com/plataformatec/devise/blob/ee01bac8b0b828b3da0d79c46115ba65c433d6c8/lib/devise.rb#L497

I can see that this is the oldest amongst, rack, rails, etc.

I tried finding if the length leak is even true, if so devise is currently leaking length.

I see that the comment got into rack in https://github.com/rack/rack/pull/625/files and then in Rails.

More reading, and I see, its a valid comparison ref: https://paragonie.com/blog/2015/11/preventing-timing-attacks-on-string-comparison-with-double-hmac-strategy even with different string lengths.

Here is what other languages do: https://security.stackexchange.com/questions/83660/simple-string-comparisons-not-secure-against-timing-attacks

We can continue with what we have right now, without the the SHA256 and loop over, considering it as "safe".

Or use the SHA256 which is what is called a double hmac: https://security.stackexchange.com/questions/83660/simple-string-comparisons-not-secure-against-timing-attacks

@vipulnsward

vipulnsward Jun 9, 2017

Member

Everywhere I see, everyone just relies on this definition:

https://github.com/plataformatec/devise/blob/ee01bac8b0b828b3da0d79c46115ba65c433d6c8/lib/devise.rb#L497

I can see that this is the oldest amongst, rack, rails, etc.

I tried finding if the length leak is even true, if so devise is currently leaking length.

I see that the comment got into rack in https://github.com/rack/rack/pull/625/files and then in Rails.

More reading, and I see, its a valid comparison ref: https://paragonie.com/blog/2015/11/preventing-timing-attacks-on-string-comparison-with-double-hmac-strategy even with different string lengths.

Here is what other languages do: https://security.stackexchange.com/questions/83660/simple-string-comparisons-not-secure-against-timing-attacks

We can continue with what we have right now, without the the SHA256 and loop over, considering it as "safe".

Or use the SHA256 which is what is called a double hmac: https://security.stackexchange.com/questions/83660/simple-string-comparisons-not-secure-against-timing-attacks

end
module_function :variable_size_secure_compare
module_function :secure_compare

This comment has been minimized.

@jeremy

jeremy Apr 11, 2016

Member

Should we change other internal usage of secure_compare to use fixed_length_secure_compare now? In these cases we guarantee that the strings we're comparing are the same length, so raising an exception on mismatch is desirable.

@jeremy

jeremy Apr 11, 2016

Member

Should we change other internal usage of secure_compare to use fixed_length_secure_compare now? In these cases we guarantee that the strings we're comparing are the same length, so raising an exception on mismatch is desirable.

This comment has been minimized.

@vipulnsward

vipulnsward Apr 11, 2016

Member

We can't do that, in previous case we would return false in case of mismatch. We need to use new one to be backwards compatible, else we would raise unexpectedly.

@vipulnsward

vipulnsward Apr 11, 2016

Member

We can't do that, in previous case we would return false in case of mismatch. We need to use new one to be backwards compatible, else we would raise unexpectedly.

This comment has been minimized.

@jeremy

jeremy Apr 12, 2016

Member

I mean, other internal usage where we're definitely comparing equal-length strings, like AS::MessageVerifier#valid_message? :)

@jeremy

jeremy Apr 12, 2016

Member

I mean, other internal usage where we're definitely comparing equal-length strings, like AS::MessageVerifier#valid_message? :)

This comment has been minimized.

@vipulnsward

vipulnsward Apr 12, 2016

Member

:-) On it.

@vipulnsward

vipulnsward Apr 12, 2016

Member

:-) On it.

end
end
@@ -11,4 +11,15 @@ def test_variable_size_secure_compare_should_perform_string_comparison
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
ProTip! Use n and p to navigate between commits in a pull request.