Make variable_size_secure_compare public #24510
Conversation
|
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
force-pushed
the
make-variable_size_secure_compare-public
branch
from
4e251b0
to
1abcc86
Compare
|
@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) |
There was a problem hiding this comment.
Should we make this public API, or is variable-length secure_compare sufficient?
There was a problem hiding this comment.
This allows others to make use of compare, without always using SHA256, for example, they may use HMAC.
vipulnsward
force-pushed
the
make-variable_size_secure_compare-public
branch
2 times, most recently
from
aa98dbe
to
7b1b26d
Compare
| 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 |
There was a problem hiding this comment.
@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:
ArgumentErrorraise is only on negative cases.- On positive cases, we won't need to do extra SHA over digest, which out-weighs negative cases.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
make-variable_size_secure_compare-public
branch
from
7b1b26d
to
25db61f
Compare
vipulnsward
force-pushed
the
make-variable_size_secure_compare-public
branch
from
25db61f
to
2e6c8a4
Compare
| @@ -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') | |||
There was a problem hiding this comment.
Can you fix the rubocop violations here?
…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.
vipulnsward
force-pushed
the
make-variable_size_secure_compare-public
branch
from
2e6c8a4
to
fa48776
Compare
| # 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.
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.
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.
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=
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If timing is irrelevant I don't see why not compare the string after it.
There was a problem hiding this comment.
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
…compare-public Make variable_size_secure_compare public
Ideally one would use
variable_size_secure_compareoversecure_compare, since its not leaking length information.So make it public as well, as an option to
secure_compareof variable length strings.r? @tenderlove