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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the probability is 2^-256, I'm not a cryptology but I think it is close enough to impossible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically we could, but 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= There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Timing is irrelevant if we've already proven they're SHA256-equal. I'm just nervous about effectively adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If timing is irrelevant I don't see why not compare the string after it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Everywhere I see, everyone just relies on this definition: 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, other internal usage where we're definitely comparing equal-length strings, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :-) On it. |
||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this public API, or is variable-length secure_compare sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows others to make use of compare, without always using
SHA256
, for example, they may useHMAC
.