Add OpenSSL.memcmp? for constant time/timing safe comparison

[bdewater]

Fixes https://bugs.ruby-lang.org/issues/10098 - at the 2019-02-07 developer meeting it was punted back to OpenSSL to implement. I went with @shyouhei's suggestion to mirror String#casecmp? naming.

One way or the other, I think having a function like this built in to the standard library instead of several application implementations is a good thing. E.g. .NET Core has it too.

Some benchmarking on my Macbook Pro 15" 2018 shows this is also faster than Ruby implementations:

> BYTES=16 bundle exec benchmark.rb
Calculating -------------------------------------
      Active Support    457.338k (± 0.8%) i/s -      2.311M in   5.053220s
                Rack    569.963k (± 0.5%) i/s -      2.871M in   5.036927s
             OpenSSL     11.822M (± 0.8%) i/s -     59.350M in   5.020598s

Comparison:
             OpenSSL: 11822116.2 i/s
                Rack:   569963.5 i/s - 20.74x  slower
      Active Support:   457338.3 i/s - 25.85x  slower



> BYTES=32 bundle exec benchmark.rb
Calculating -------------------------------------
      Active Support    259.457k (± 0.6%) i/s -      1.309M in   5.046238s
                Rack    309.408k (± 1.0%) i/s -      1.553M in   5.020759s
             OpenSSL     10.744M (± 0.7%) i/s -     53.870M in   5.014193s

Comparison:
             OpenSSL: 10743986.5 i/s
                Rack:   309408.0 i/s - 34.72x  slower
      Active Support:   259457.5 i/s - 41.41x  slower

require 'benchmark/ips'
require 'openssl'

BYTES = ENV["BYTES"].to_i
STRING1 = 'A' * BYTES
STRING2 = 'A' * BYTES

def active_support(a, b)
  raise ArgumentError, "string length mismatch." unless a.bytesize == b.bytesize

  l = a.unpack "C#{a.bytesize}"

  res = 0
  b.each_byte { |byte| res |= byte ^ l.shift }
  res == 0
end

def rack(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(a, b)
  OpenSSL.memcmp?(a, b)
end

Benchmark.ips do |x|
  x.report('Active Support') { active_support(STRING1, STRING2) }
  x.report('Rack')           { rack(STRING1, STRING2) }
  x.report('OpenSSL')        { openssl(STRING1, STRING2) }
  x.compare!
end

[ioquatix]

For a crytographic function that's trying to be timing invariant, is this acceptable?

[bdewater]

We have to do something to ensure the strings are of equal length, otherwise the refute OpenSSL.memcmp?("aaa", "aaaa") test fails since CRYPTO_memcmp compares up to len1 bytes.

Rack, Active Support up to 5.1 (rails/rails#24510), .NET Core and the snippets in https://codahale.com/a-lesson-in-timing-attacks/ do the same thing and exit early. I think it's not terrible, but calling it out in the docs would be the least we can do to make it better.

Active Support 5.2+ replaced the default behaviour by always hashing with SHA-256 first and raising on length inequality in a separate method. I can add something similar if you prefer.

[ioquatix]

@picatz do you have any input on this?

Here is an example which shows that such an implementation leaks the string length:

#!/usr/bin/env ruby

require 'benchmark'

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

	l = a.unpack "C#{a.bytesize}"

	res = 0
	b.each_byte { |byte| res |= byte ^ l.shift }
	res == 0
end


def find_length_of_secret(secret)
	samples = 100.times.collect do |i|
		test = "\0" * i
		
		[i, Benchmark.realtime do
			1000.times do
				compare(test, secret)
			end
		end]
	end
	
	best_guess = samples.sort_by{|sample| sample.last}.last
	
	return best_guess.first
end

pp find_length_of_secret("foobar" * 2)

The example you give from ActiveSupport throws an exception when strings are not the same length. Provided we clearly mark the arguments (user input) and (secret), we can run the algorithm up to the length of secret in every case, that will minimise leaking any details about the length of the secret string... the entire runtime of the algorithm might be used to deduce the string length. For example:

compare(user_input, "twenty characters!!!") -> 20ns
compare(user_input, secret) -> 40ns

Maybe user can figure out secret is 40 characters long.

But, given how much noise and other overheads we expect, maybe it's not a concern. Even though I can think of several ways to make this harder, I'm not sure it's feasible to fix - e.g. adding a small random amount of noise to the function timing (sleep rand).

[ioquatix]

Maybe throwing an exception if the string lengths are different isn't such a stupid idea?

[bdewater]

After thinking about this a bit more, I think it's the most safe option to pick yeah. The OpenSSL variant might be harder to attack than the Ruby one due to the lower speed variance as seen in the benchmark. Or the length of the secret might not be much of a secret in itself (e.g. 32 bytes/SHA-256). But then again you might never know how people use this API and how that can be abused 🤷‍♂

I'll work on this change and some additional documentation tomorrow.

[p8]

Maybe do CRYPTO_memcmp('a', 'b', 1) (or another example that always returns false) if the lenghts are not the same?

[p8]

In ruby psuedo code you could do something like this:

def ossl_crypto_memcmp(str1, str2)
  len1 = str1.length
  len2 = str2.length
  # always generate incorrect answers even when str1 and str2 are the same length so it doesn't effect the time.
  incorrect1 = 'a' * len1
  incorrect2 = 'b' * len1
  if len1 != len2
    CRYPTO_memcmp(incorrect1 , incorrect2, len1)
  else
    CRYPTO_memcmp(str1, str2, len1)
  end
end

But reading the documentation for CRYPTO_memcmp it can still leak the length of the other string. Using this would require users to always make sure the strings to compare are always the same length.

The CRYPTO_memcmp function compares the len bytes pointed to by a and b for equality. It takes an amount of time dependent on len, but independent of the contents of the memory regions pointed to by a and b.

[p8]

Here's a good discussion about the keeping the length secret https://security.stackexchange.com/a/92238

[ioquatix]

It looks very good.

[shyouhei]

In order to prevent false positives in "aaa" vs. "aaaa" -like comparisons, You can return (len1==len2) ? Qtrue : Qfalse here instead of just Qtrue. That way the if statement above can be eliminated.

[ioquatix]

Do you mean using minimum length for comparison and then doing final length check?

[shyouhei]

Yes. And I now think I was wrong.

Basically, when you do a timing-safe comparison, one side is "good" and the other side is "bad", like SHA-2 versus user inputs. It seems C APIs for timing-safe comparison are expecting the length argument to be equal to that of "good" ones, which is normally known before actual comparison.

For us, because this API is going to take only 2 arguments, I think we have to assume either side being "good" and take its length, like OpenSSL.memcmp?(good, bad) or vice versa. This is fragile. I can think of several ways to reroute:

• Change the API so that we can take third argument: OpenSSL.memcmp?(good, bad, length=good.length). This way it is up to the user to properly pass expected length. Not friendly, though.
• If we are going with the current API: allocate buffers internally, align length of both sides, fill them with the arguments + zero padded, and do the actual comparison. This might introduce another problem (attacker can feed 1TB junk password).
• Like @bdewater wrote other implementations just early return so it might be okay as-is.
• Not sure about raising exceptions, at the moment.

[ioquatix]

What is the risk of knowing the secret length?

[p8]

• An attacker can find users with the shortest/weakest passwords making them an easier attack target.
• An attacker doesn't have to try secrets with a smaller secret length. This is can be around a 1-2% advantage for random characters (1/number of characters).
  See also: https://security.stackexchange.com/a/92238

A better question might be: what's a valid case for comparing different length strings? Shouldn't this method always compare hashes anyway?

[bdewater]

Changed to raise exception on length mismatch and added documentation.

[bdewater]

I'm not sure why the AppVeyor build is cancelled, other than that I believe this is ready. Let me know what you think :)

[ioquatix]

Okay, so the only thing I'm wondering about is the naming right for Ruby.

memcmp? kind of seems.. C-specific.

I guess it's reflecting the underlying OpenSSL method.

But maybe something more "obvious" could be better. Like "OpenSSL.secure_compare(s1, s2)"

I do like memcmp but it's not a question, so why memcmp? because this seems like "Can I do memcmp?". The equivalent would be String#casecmp.

Thoughts?

[bdewater]

@ioquatix there's been a healthy discussion over at https://bugs.ruby-lang.org/issues/10098 but no clear consensus emerged for a name.

The Rails-centric secure_compare name might be the best one for recognizability, even if I personally don't feel like it's the best name it's not the hill I'm going to die on. Happy to change it to something else but I'm about to leave on vacation so it'll be a few weeks before I can change it.

[ioquatix]

I am going to merge this as is, but I will probably rename the function before the next release.

[ioquatix]

I just need some time to review the existing notes.. and I don't want this PR to get forgotten.

[bdewater]

Thanks for merging!

[ioquatix]

I am reviewing this code.

 * For securely comparing user input, it's recommended to use hashing and
 * regularly compare after to prevent an unlikely false positive due to a
 * collision.
 *
 *   user_input      = "..."
 *   expected        = "..."
 *   hashed_input    = OpenSSL::Digest::SHA256.digest(user_input)
 *   hashed_expected = OpenSSL::Digest::SHA256.digest(expected)
 *   OpenSSL.secure_compare(hashed_input, hashed_expected) && user_input == expected

Isn't it possible for SHA256.digest to leak the length of the string? Because it's not fixed cost operation. It's proportional to the size of the input (I assume).

Thoughts?

[ioquatix]

Just FYI, I want to avoid making recommendations in the documentation that might be wrong or give the wrong idea about how to do something securely.

[ioquatix]

I made a minor change to clarify the risk of hashing the secret.

1ade643

[bdewater]

From that commit:

Be aware that timing attacks against the hash functions may reveal the length of the secret.

I strongly agree with wanting to not make wrong recommendations 🙂 but this feels wrong to me since no options are given to address this issue mentioned, and could make people decide not to hashing which I believe to be a worse outcome if you compare the two.

To my understanding SHA-256 works on 64 byte message blocks, anything that fits within that block size always takes the same amount of time to compute. So at best you'd be able to guess the multiples of 64 bytes the original input was, which is useless info in a world where strong secrets fit into that size. Compare that to not using SHA-256 on user input at all, where you would be able to guess the secret's length exactly.

Benchmarking supports this:

require 'benchmark/ips'
require 'openssl'

STRING1 = 'A' * 16
STRING2 = 'A' * 17
STRING3 = 'A' * 32

def compare_with_hash(a, b)
  hashed_a = OpenSSL::Digest::SHA256.digest(a)
  hashed_b = OpenSSL::Digest::SHA256.digest(b)
  begin
    OpenSSL.memcmp?(hashed_a, hashed_b) && a == b
  rescue ArgumentError
    false
  end
end

def compare_direct(a, b)
  begin
    OpenSSL.memcmp?(a, b)
  rescue ArgumentError
    false
  end
end

Benchmark.ips do |x|
  x.report('readme example, same string lengths') { compare_with_hash(STRING1, STRING1) }
  x.report('readme example, small difference length') { compare_with_hash(STRING1, STRING2) }
  x.report('readme example, double length') { compare_with_hash(STRING1, STRING3) }
  x.report('direct comparison, same string lengths') { compare_direct(STRING1, STRING1) }
  x.report('direct comparison, small difference length') { compare_direct(STRING1, STRING2) }
  x.report('direct comparison, double length') { compare_direct(STRING1, STRING3) }
  x.compare!
end

Results:

Calculating -------------------------------------
readme example, same string lengths
                        451.244k (± 4.2%) i/s -      2.267M in   5.033424s
readme example, small difference length
                        448.077k (± 2.0%) i/s -      2.254M in   5.032493s
readme example, double length
                        447.173k (± 1.9%) i/s -      2.254M in   5.042373s
direct comparison, same string lengths
                         12.001M (± 1.3%) i/s -     60.013M in   5.001435s
direct comparison, small difference length
                        732.699k (± 3.4%) i/s -      3.679M in   5.028277s
direct comparison, double length
                        734.025k (± 2.5%) i/s -      3.701M in   5.044770s

I also found https://paragonie.com/blog/2015/11/preventing-timing-attacks-on-string-comparison-with-double-hmac-strategy where @paragonie-scott writes the following about timing SHA-256:

In fact, it's conceptually impossible to stop length from leaking out.