From c8c660002f4b0e9606de96325f20b95248b6ff2d Mon Sep 17 00:00:00 2001 From: Guillermo Iguaran Date: Thu, 23 Oct 2014 10:56:48 -0300 Subject: [PATCH 1/3] Add AS::SecurityUtils.secure_compare for constant time string comparison --- .../lib/active_support/security_utils.rb | 20 +++++++++++++++++++ activesupport/test/security_utils_test.rb | 9 +++++++++ 2 files changed, 29 insertions(+) create mode 100644 activesupport/lib/active_support/security_utils.rb create mode 100644 activesupport/test/security_utils_test.rb diff --git a/activesupport/lib/active_support/security_utils.rb b/activesupport/lib/active_support/security_utils.rb new file mode 100644 index 0000000000..64c4801179 --- /dev/null +++ b/activesupport/lib/active_support/security_utils.rb @@ -0,0 +1,20 @@ +module ActiveSupport + module SecurityUtils + # Constant time string comparison. + # + # The values compared should be of fixed length, such as strings + # that have already been processed by HMAC. This should not be used + # on variable length plaintext strings because it could leak length info + # via timing attacks. + def secure_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 + module_function :secure_compare + end +end diff --git a/activesupport/test/security_utils_test.rb b/activesupport/test/security_utils_test.rb new file mode 100644 index 0000000000..08d2e3baa6 --- /dev/null +++ b/activesupport/test/security_utils_test.rb @@ -0,0 +1,9 @@ +require 'abstract_unit' +require 'active_support/security_utils' + +class SecurityUtilsTest < ActiveSupport::TestCase + def test_secure_compare_should_perform_string_comparison + assert ActiveSupport::SecurityUtils.secure_compare('a', 'a') + assert !ActiveSupport::SecurityUtils.secure_compare('a', 'b') + end +end From 4b11dea3917f2101d7125a967877abf19c36317a Mon Sep 17 00:00:00 2001 From: Guillermo Iguaran Date: Thu, 23 Oct 2014 10:58:43 -0300 Subject: [PATCH 2/3] Use AS secure_compare in AS::MessageVerifier --- .../lib/active_support/message_verifier.rb | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/activesupport/lib/active_support/message_verifier.rb b/activesupport/lib/active_support/message_verifier.rb index 6cb2884fb7..4e0796f4f8 100644 --- a/activesupport/lib/active_support/message_verifier.rb +++ b/activesupport/lib/active_support/message_verifier.rb @@ -1,5 +1,6 @@ require 'base64' require 'active_support/core_ext/object/blank' +require 'active_support/security_utils' module ActiveSupport # +MessageVerifier+ makes it easy to generate and verify messages which are @@ -37,7 +38,7 @@ def verify(signed_message) raise InvalidSignature if signed_message.blank? data, digest = signed_message.split("--") - if data.present? && digest.present? && secure_compare(digest, generate_digest(data)) + if data.present? && digest.present? && ActiveSupport::SecurityUtils.secure_compare(digest, generate_digest(data)) begin @serializer.load(::Base64.strict_decode64(data)) rescue ArgumentError => argument_error @@ -55,17 +56,6 @@ def generate(value) end private - # constant-time comparison algorithm to prevent timing attacks - def secure_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 generate_digest(data) require 'openssl' unless defined?(OpenSSL) OpenSSL::HMAC.hexdigest(OpenSSL::Digest.const_get(@digest).new, @secret, data) From 0073d274de5bf3894f6da27f798238908eed43b5 Mon Sep 17 00:00:00 2001 From: Guillermo Iguaran Date: Thu, 23 Oct 2014 11:00:30 -0300 Subject: [PATCH 3/3] Use AS secure_compare for CSRF token comparison --- .../lib/action_controller/metal/request_forgery_protection.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index a4f376816f..fd20682f8f 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -1,5 +1,6 @@ require 'rack/session/abstract/id' require 'action_controller/metal/exceptions' +require 'active_support/security_utils' module ActionController #:nodoc: class InvalidAuthenticityToken < ActionControllerError #:nodoc: @@ -305,8 +306,7 @@ def valid_authenticity_token?(session, encoded_masked_token) end def compare_with_real_token(token, session) - # Borrow a constant-time comparison from Rack - Rack::Utils.secure_compare(token, real_csrf_token(session)) + ActiveSupport::SecurityUtils.secure_compare(token, real_csrf_token(session)) end def real_csrf_token(session)