Add hashed secure compare

[bdewater]

Follow-up from the discussion over at #269 (comment) and 1ade643 where the Rails-like secure_compare naming was adopted. The debate around timing of hash functions made me realize why Rails' default of hashing the values to protect users from making mistakes is a good idea.

[ioquatix]

@tenderlove @jeremyevans I am okay with this but since you have some background in this area, do you mind commenting?

[ioquatix]

Thanks for this.

Since this is functionality in excess of OpenSSL, I'm on the fence as to whether it belongs here or not. Because we start implementing functionality and features that are layered on top of OpenSSL.

We also don't want to give users tools that are difficult to use correctly. So, I see the value in this.

Carrying on the previous discussion from #269

There are places where this this interface might actually be worse in practice.

Let's say you have some kind of IP address and/or user agent stored in a session, and that is considered secret.

The web server might receive the session id from a cookie, and then need to secure_compare the user agent strings. In order to avoid leaking the length or other details of the user agent strings, they are hashed first according to this function. Whether or not of practical use, some information is leaked about the secret in this process. You can't avoid it - a hash checksum is proportional to the size of the input to a certain extent.

Therefore, the more robust solution is to compute the hash of the user agent before saving it in the very first instance (non-hostile environment) and all subsequent checks are fixed length and therefore cannot possibly leak information.

Unfortunately, this interface encourages users to do the former, not the latter.

Thoughts?

[ioquatix]

#!/usr/bin/env ruby

require 'digest/sha2'

class Secret
	def initialize(string)
		@secret = digest(string)
	end
	
	def digest(string)
		Digest::SHA256.digest(string)
	end
	
	def == hostile
		# OpenSSL.secure_compare(digest(hostile), @secret)
		digest(hostile) == @secret
	end
end

secret = Secret.new("Hello World!")

puts secret == "Hello World!"
puts secret == "Goodbye World!"

[bdewater]

Thanks for your continuous feedback @ioquatix!

There are places where this this interface might actually be worse in practice.

fixed_length_secure_compare is still available for those who know what they're doing.

Therefore, the more robust solution is to compute the hash of the user agent before saving it in the very first instance (non-hostile environment) and all subsequent checks are fixed length and therefore cannot possibly leak information.

Double checking if I understand you correctly by rewording it in another example: my API is secured with a key supplied via an environment variable. You're saying it's more secure to hash that key once during app boot, instead of doing it every time the controller code is called?

I agree that's better in theory, but let's take a look at the benchmarks one more time - I forgot to post the comparison results in the previous PR. The difference in practice is very small with hashing:

readme example, same string lengths:       445628.5 i/s
readme example, small difference length:   436656.1 i/s - same-ish: difference falls within error
readme example, large difference length:   433763.4 i/s - same-ish: difference falls within error

Note that "large difference length" is the exact same length (32 bytes) of a prehashed SHA-256 digest. And we're working in a garbage collected language too so who knows when the GC kicks in in production 😄

While the direct comparison without hashing shows an order of magnitude difference:

direct comparison, same string lengths:   12001413.5 i/s
direct comparison, double length:           734025.4 i/s - 16.35x  slower
direct comparison, small difference length: 732699.2 i/s - 16.38x  slower

If there's only one thing someone is going to remember, I think this is a good default.

[ioquatix]

The real proof is whether you can write code, given everything except the secret string, can determine the length of the secret string.

Something like:

#!/usr/bin/env ruby

require 'benchmark'
require 'digest/sha2'

LETTERS = ('a'..'z').to_a

@secret = rand(0..250).times.map{LETTERS.sample}.join

puts "Secret: #@secret (#{@secret.bytesize} bytes)"

def digest(string)
	Digest::SHA256.digest(string)
end

def test(hostile)
	digest(hostile)
	digest(@secret)
end

def attack
	timings = []
	
	base_case = Benchmark.measure do
		10000.times do
			test("")
		end
	end.utime
	
	puts "Empty case: #{base_case}s"
	
	250.times do |length|
		puts "Checking #{length}..."
		
		string = "." * length
		
		duration = Benchmark.measure do
			10000.times do
				test(string)
			end
		end.utime
		
		if duration >= (base_case*1.1)
			puts "Duration: #{duration} **"
		else
			puts "Duration: #{duration}"
		end
	end
end

attack

If we can show an attack against the digest approach, then it's instant failure.

But if we can't show any attack, it might be feasible.

That's my advice right now.

[ioquatix]

The other question is, do we about leaking length of secret string. Maybe it's irrelevant. I'm okay if we accept that position. It just needs to be clearly documented.

[ioquatix]

direct comparison, same string lengths:   12001413.5 i/s
direct comparison, double length:           734025.4 i/s - 16.35x  slower
direct comparison, small difference length: 732699.2 i/s - 16.38x  slower

Doesn't make sense to me, is this using the constant time string comparison? It doesn't support different string lengths?

[jeremyevans]

I don't think leaking the size of the secret is a major issue. Either the secret is large enough to make brute force attacks impractical, or it is small enough that it is vulnerable to brute force attacks anyway and trying brute force attacks on a smaller keyspace (fewer characters) first would not result in a significant slowdown.

That being said, I don't really like the fact that OpenSSL.secure_compare operates differently than Rack::Utils.secure_compare and raises an ArgumentError for different string lengths. So if we don't want to drop the ArgumentError being raised, hashing before compare seems reasonable.

[ioquatix]

Is there any reason why we can't just use def self.secure_compare? rather than module_function.

[jeremyevans]

module_function also makes it a private instance method, so that if someone does include OpenSSL, then they call secure_compare (instead of OpenSSL.secure_compare). FWIW, Rack::Utils.secure_compare also uses module_function.

[ioquatix]

Okay based on that feedback, I think this is a pretty reasonable implementation. Happy to merge. Just one minor feedback regarding the module_function.

[bdewater]

If we can show an attack against the digest approach, then it's instant failure.
But if we can't show any attack, it might be feasible.

&

direct comparison, same string lengths:   12001413.5 i/s
direct comparison, double length:           734025.4 i/s - 16.35x  slower
direct comparison, small difference length: 732699.2 i/s - 16.38x  slower

Doesn't make sense to me, is this using the constant time string comparison? It doesn't support different string lengths?

Yes, it uses what this PR now calls fixed_length_secure_compare but rescues the ArgumentError similar to how a web application's exception handler would. By hashing the inputs the operation is running more in constant time than without, meaning an attacker gets less useful information to go on.

That being said, I don't really like the fact that OpenSSL.secure_compare operates differently than Rack::Utils.secure_compare and raises an ArgumentError for different string lengths.

Rack can still return early (as it does today) before calling fixed_length_secure_compare to keep that behaviour :)

Just one minor feedback regarding the module_function.

Adressed 👍

[ioquatix]

LGTM.