Use faster OpenSSL secure comparison if available

[bdewater]

openssl_secure_compare:  9397508.4 i/s
   rack_secure_compare:   515938.0 i/s - 18.21x  (± 0.00) slower

Benchmark

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  gem "benchmark-ips"
  gem "openssl", "~> 2.2"
end

require 'benchmark/ips'
require 'openssl'

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

  l = a.unpack("C*")

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

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

  OpenSSL.fixed_length_secure_compare(a, b)
end

Benchmark.ips do |x|
  string = "acbdefgh12345678"
  
x.report("rack_secure_compare") do
    rack_secure_compare(string, string)
  end

  x.report("openssl_secure_compare") do
    openssl_secure_compare(string, string)
  end

  x.compare!
end

[jeremyevans]

I'm fine using the faster implementation in the openssl extension. Only one minor implementation comment.

[ioquatix]

Thanks for this!

[jeremyevans]

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.

[akostadinov]

Sorry, this doesn't sound convincing. Backward compatibility can be achieved by using slower variable length algorithm, then writing a new fixed-length method.

[jeremyevans]

I fail to see how this PR made things less secure, considering the return false unless a.bytesize == b.bytesize was there before, so that part has not changed.

You are correct that we could add a new fixed-length method that raises instead of returning false. However, I recommend that new code that wanted those semantics call OpenSSL.fixed_length_secure_compare directly. Why add a method to rack that just delegated to that?

[tenderlove]

Is raising an exception really not backwards compatible? The method documentation clearly says users should not pass strings of differing length. It would only be backwards incompatible in the case that someone is using the method wrong. If I was using this method wrong I would like to know about it, and "returning false" isn't going to tell me that.

[jeremyevans]

Is raising an exception really not backwards compatible?

Yes, it really is not backwards compatible to switch from returning false to raising an exception.

The method documentation clearly says users should not pass strings of differing length. It would only be backwards incompatible in the case that someone is using the method wrong. If I was using this method wrong I would like to know about it, and "returning false" isn't going to tell me that.

The method was not originally documented that way (9a81b96), that was added later (9b4d955). Granted, the warning was added about 8 months after it was added, and that was about a decade ago.

I'm not opposed to changing this to raise an exception if we follow our standard deprecation procedure (deprecation warning in Rack 3.1, removal in 3.2 or 4.0).

[akostadinov]

If one uses "#secure_compare" but it is in fact insecure for their use case, wouldn't that justify such a risk? To me it does.

[jeremyevans]

If one uses "#secure_compare" but it is in fact insecure for their use case, wouldn't that justify such a risk? To me it does.

If this actually opened a timing bug that would allow determination of the compared data, and not just the length of the compared data, I would agree with you. However, as I already stated, if the compared data is of sufficient length, leaking the length is not a large decrease in security. Even if you are only comparing decimal input ([0-9]+), we are talking an 11% decrease in brute force time. For lowercase alphanumeric input ([a-z0-9]+), the decrease in brute force time is 2.3%. For alphanumeric input ([A-Za-z0-9+]), the decrease in brute force time is 1.6%. The formula for determining brute force time decrease: (1...x).map{|i| n**i}/(n.to_f**x) where n is the number of allowed characters, and x is the length of the input. If I've got the math wrong here, please let me know.

[akostadinov]

This is a assuming a use case, "sufficient length" and non-prior knowledge of a potential attacker. I maintain that if a method promised certain guarantees, it has to provide them or fail loudly.

[jeremyevans]

If what you are comparing against is not of sufficient length, then it is going to be faster to run a brute force attack on progressively increasing lengths, than a timing attack to determine the exact length followed by a brute force attack on the exact length. If the attacker has prior knowledge of what the comparison values might be for length x, without knowing what the exact value of x, then brute force attacks based on the prior knowledge with increasing values of x is also likely going to be faster than running a timing attack to determine x and then running the attack with the prior knowledge of x (e.g. try the most likely value for lengths 1 to max, then the second most likely value for lengths 1 to max, etc.).

The method already documents that it leaks length info via timing attacks, so it already provides what it promises.