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

Conversation

Projects
None yet
6 participants
@vipulnsward
Member

vipulnsward commented Apr 11, 2016

Ideally one would use variable_size_secure_compare over secure_compare, since its not leaking length information.

So make it public as well, as an option to secure_compare of variable length strings.

r? @tenderlove

@jeremy

This comment has been minimized.

Member

jeremy commented Apr 11, 2016

I think it's confusing to expose both without clear guidance about which to use.

It'd make sense to swap these, even, and make variable-length comparison the default. Rename current secure_compare to fixed_length_secure_compare and have it raise ArgumentError if there's a length mismatch.

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Apr 11, 2016

@jeremy changed, ready for first review.

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?

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.

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.

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.

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? :)

This comment has been minimized.

@vipulnsward

vipulnsward Apr 12, 2016

Member

:-) On it.

@vipulnsward

View changes

activesupport/lib/active_support/message_verifier.rb Outdated
data.present? && digest.present? && ActiveSupport::SecurityUtils.secure_compare(digest, generate_digest(data))
data.present? && digest.present? && ActiveSupport::SecurityUtils.fixed_length_secure_compare(digest, generate_digest(data))
rescue ArgumentError
false

This comment has been minimized.

@vipulnsward

vipulnsward Apr 12, 2016

Member

@jeremy this is the only intrusive change. This is so we guarantee return of true/false values, that others depend on, like cookies.
I think its ok to do this because:

  • ArgumentError raise is only on negative cases.
  • On positive cases, we won't need to do extra SHA over digest, which out-weighs negative cases.

This comment has been minimized.

@jeremy

jeremy Apr 13, 2016

Member

It does feel weird to do a blanket ArgumentError rescue here, though.

Maybe we should leave the return false unless a.bytesize == b.bytesize for now.

This comment has been minimized.

@vipulnsward

vipulnsward Apr 13, 2016

Member

I reverted this. If this is not acceptable:

  • We change to return false unless a.bytesize == b.bytesize. This would take us back to making it unsafe with length comparison
  • Just keep this as a doc change

@vipulnsward vipulnsward reopened this Jun 6, 2017

activesupport/test/security_utils_test.rb Outdated
@@ -11,4 +11,16 @@ 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')

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 6, 2017

Member

Can you fix the rubocop violations here?

This comment has been minimized.

@vipulnsward

vipulnsward Jun 6, 2017

Member

Should be fixed now.

Changed default behaviour of `ActiveSupport::SecurityUtils.secure_co…
…mpare`,

    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.
# 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?

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.

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=

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.

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.

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

@rafaelfranca rafaelfranca merged commit fa48776 into rails:master Nov 25, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate no new or fixed issues
Details

rafaelfranca added a commit that referenced this pull request Nov 25, 2017

Merge pull request #24510 from vipulnsward/make-variable_size_secure_…
…compare-public

Make variable_size_secure_compare public
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment