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.

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

Speed up ActiveSupport::SecurityUtils.fixed_length_secure_compare #40429

Merged

Conversation

natematykiewicz
Copy link
Contributor

Summary

Speed up ActiveSupport::SecurityUtils.fixed_length_secure_compare by using OpenSSL.fixed_length_secure_compare, if available. OpenSSL's version is a C method, which is faster.

Other Information

# frozen_string_literal: true

require 'openssl'
require 'benchmark/ips'

string = '1234567890'

def slow(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 fast(a, b)
  OpenSSL.fixed_length_secure_compare(a, b)
end

Benchmark.ips do |x|
  x.report('slow') { slow(string, string) }
  x.report('fast') { fast(string, string) }
  x.compare!
end
Warming up --------------------------------------
                slow    69.393k i/100ms
                fast     1.302M i/100ms
Calculating -------------------------------------
                slow    638.026k (± 5.3%) i/s -      3.192M in   5.018762s
                fast     12.416M (± 5.4%) i/s -     62.517M in   5.050475s

Comparison:
                fast: 12415792.1 i/s
                slow:   638026.5 i/s - 19.46x  (± 0.00) slower

There is one minor difference between the two methods. While both raise an ArgumentError if the strings are not equal length, OpenSSL's error message is inputs must be of equal length (https://github.com/ruby/openssl/blob/master/ext/openssl/ossl.c#L627), while ActiveSupport's is string length mismatch..

Since the ArgumentError is not really expected, I figured it doesn't matter that the messages are different. If anyone is actually rescuing this error, they should probably just be using ActiveSupport::SecurityUtils.secure_compare which doesn't raise in the first place.

by using `OpenSSL.fixed_length_secure_compare`, if available.
@rafaelfranca rafaelfranca merged commit 7eb855b into rails:master Oct 22, 2020
@natematykiewicz natematykiewicz deleted the openssl_fixed_length_secure_compare branch October 22, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants