Skip to content
Permalink
Browse files Browse the repository at this point in the history
Add secure_compare to Rack::Utils
Conflicts:
	test/spec_utils.rb
  • Loading branch information
raggi committed Feb 7, 2013
1 parent 5497fd2 commit 9a81b96
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 0 deletions.
12 changes: 12 additions & 0 deletions lib/rack/utils.rb
Expand Up @@ -395,6 +395,18 @@ def byte_ranges(env, size)
end
module_function :byte_ranges

# Constant time string comparison.
def secure_compare(a, b)
return false unless bytesize(a) == bytesize(b)

l = a.unpack("C*")

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

# Context allows the use of a compatible middleware at different points
# in a request handling stack. A compatible middleware must define
# #context which should take the arguments env and app. The first of which
Expand Down
5 changes: 5 additions & 0 deletions test/spec_utils.rb
Expand Up @@ -354,6 +354,11 @@ def kcodeu
Rack::Utils.bytesize("FOO\xE2\x82\xAC").should.equal 6
end

should "should perform constant time string comparison" do
Rack::Utils.secure_compare('a', 'a').should.equal true

This comment has been minimized.

Copy link
@postmodern

postmodern Feb 8, 2013

Contributor

This does not actually test where secure_compare performs a constant time string comparison, just that it compares two Strings.

Rack::Utils.secure_compare('a', 'b').should.equal false
end

should "return status code for integer" do
Rack::Utils.status_code(200).should.equal 200
end
Expand Down

8 comments on commit 9a81b96

@mbj
Copy link

@mbj mbj commented on 9a81b96 Feb 8, 2013

Choose a reason for hiding this comment

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

How do you know a future ruby implementation will not optimize this in a "return early" version again? Could you add a spec that checks for constant runtime? Maybe an integration spec that does a timing analysis? I'd totally accept if rack would depend on the NaCL ruby bindings to delegate constant time comparisons.

@headius
Copy link
Contributor

@headius headius commented on 9a81b96 Feb 9, 2013

Choose a reason for hiding this comment

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

I hope this method is not going to be used anywhere. Perhaps I don't understand what it's needed for?

  • You can't compare strings in constant time; comparison will always depend on the length of the strings.
  • This is probably the most inefficient way to compare two strings. In fact, I don't actually understand what it is accomplishing.

@oscardelben
Copy link
Contributor

Choose a reason for hiding this comment

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

@headius this method is used to achieve an almost constant time comparison. See 0cd7e9a

@raggi
Copy link
Member Author

@raggi raggi commented on 9a81b96 Feb 9, 2013

Choose a reason for hiding this comment

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

@mbj I am happy to add some tests that make a loose effort at proving at the time is relative to the length of both strings, and doesn't fast exit. The rest of your suggestions and comments are impossible, excessive or odd.

@headius sure, the difference with this method is that it will always take time relative to the length of the strings, not relative to the content of the strings. It is constant with respect to varying external content (not length), except for the single (safe) fast exit case of the input being empty(edit: different lengths. i'm tired, and it's the weekend, forgive me). You are absolutely correct, that in algorithmic terminology, this is not constant time as in O(1) regardless of the input. In crypto circles, this kind of comparison is generally referred to as "constant time", even though that is a misnomer. A relevant example paper that covers these timing issues can be found here: http://crypto.stanford.edu/~dabo/papers/ssl-timing.pdf. Timing attacks against HMAC are actually somewhat easier.

@raggi
Copy link
Member Author

@raggi raggi commented on 9a81b96 Feb 9, 2013

Choose a reason for hiding this comment

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

It may be true I should handle some encoding details here too, if the method is to be used in a generic fashion. For the current intended use case, where all valid characters are ascii, we do not reveal anything useful in bytesize.

@raggi
Copy link
Member Author

@raggi raggi commented on 9a81b96 Feb 9, 2013

Choose a reason for hiding this comment

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

Honestly feel like I'm being trolled here.

@tarcieri
Copy link

Choose a reason for hiding this comment

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

@headius what it's trying to accomplish is preventing short-circuiting. The phrase "constant time" is being used in regard to two Message Authentication Codes which are fixed-size. The goal is to have them always compare in constant time. Just using "==" will fail fast on the first byte. If we can measure timing information over the course of many requests and see a "signal" when brute forcing various byte combinations, we can use that signal to determine when we've successfully brute forced a single byte of a MAC. It looks something like this:

Screen Shot 2013-02-07 at 10 06 03 PM

It's somewhat analogous to picking a lock. Once you've gotten the first tumbler, you can move on to the next one, and the next one until you've picked them all.

@headius
Copy link
Contributor

@headius headius commented on 9a81b96 Feb 9, 2013

Choose a reason for hiding this comment

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

@raggi @tarcieri Yeah, I had it clarified for me elsewhere.

Please sign in to comment.