Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use faster OpenSSL secure comparison if available #1711

Merged
merged 1 commit into from Nov 13, 2020
Merged

Use faster OpenSSL secure comparison if available #1711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ All notable changes to this project will be documented in this file. For info on
- `Rack::Request#[]` and `#[]=` now warn even in non-verbose mode. ([#1277](https://github.com/rack/rack/issues/1277), [@jeremyevans](https://github.com/jeremyevans))
- Decrease default allowed parameter recursion level from 100 to 32. ([#1640](https://github.com/rack/rack/issues/1640), [@jeremyevans](https://github.com/jeremyevans))
- Attempting to parse a multipart response with an empty body now raises Rack::Multipart::EmptyContentError. ([#1603](https://github.com/rack/rack/issues/1603), [@jeremyevans](https://github.com/jeremyevans))
- `Rack::Utils.secure_compare` uses OpenSSL's faster implementation if available. ([#1711](https://github.com/rack/rack/pull/1711), [@bdewater](https://github.com/bdewater))

### Fixed

Expand Down
20 changes: 14 additions & 6 deletions lib/rack/utils.rb
View file
Open in desktop
Expand Up @@ -374,14 +374,22 @@ def get_byte_ranges(http_range, size)
# 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
if defined?(OpenSSL.fixed_length_secure_compare)
def secure_compare(a, b)
return false unless a.bytesize == b.bytesize

l = a.unpack("C*")
OpenSSL.fixed_length_secure_compare(a, b)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current case (as in the case before this PR), there is timing attack that could allow an attacker to determine the number of bytes we are comparing against. However, assuming that that number is not small (16 bytes or more), it seems unlikely that the code is actually vulnerable. Making this raise instead of returning false for a mismatched size would break backwards compatibility.

else
def secure_compare(a, b)
return false unless a.bytesize == b.bytesize

r, i = 0, -1
b.each_byte { |v| r |= v ^ l[i += 1] }
r == 0
l = a.unpack("C*")

r, i = 0, -1
b.each_byte { |v| r |= v ^ l[i += 1] }
r == 0
end
end

# Context allows the use of a compatible middleware at different points
Expand Down