From 05bbc71446bfd483308065b29b66f0f8a0445b92 Mon Sep 17 00:00:00 2001 From: Shriram Date: Tue, 3 Apr 2018 08:14:13 +0530 Subject: [PATCH] Make secure_compare handle empty strings comparison correctly Used Rails' secure_compare method inside the definition of secure_compare. This will handle the empty strings comparison and return true when both the parameters are empty strings. Fixes #4441, #4829 --- CHANGELOG.md | 1 + lib/devise.rb | 8 ++------ test/devise_test.rb | 9 +++++++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47bbcf17..dc95bae4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ * Use `OmniAuth.config.allowed_request_methods` as routing verbs for the auth path [#5508](https://github.com/heartcombo/devise/pull/5508) * Handle `on` and `ON` as true values to check params [#5514](https://github.com/heartcombo/devise/pull/5514) * Fix passing `format` option to `devise_for` [#5732](https://github.com/heartcombo/devise/pull/5732) + * Use `ActiveRecord::SecurityUtils.secure_compare` in `Devise.secure_compare` to match two empty strings correctly. [#4829](https://github.com/heartcombo/devise/pull/4829) Please check [4-stable](https://github.com/heartcombo/devise/blob/4-stable/CHANGELOG.md) diff --git a/lib/devise.rb b/lib/devise.rb index 0336ed70..c4213192 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -517,12 +517,8 @@ module Devise # constant-time comparison algorithm to prevent timing attacks def self.secure_compare(a, b) - return false if a.blank? || b.blank? || a.bytesize != b.bytesize - l = a.unpack "C#{a.bytesize}" - - res = 0 - b.each_byte { |byte| res |= byte ^ l.shift } - res == 0 + return false if a.nil? || b.nil? + ActiveSupport::SecurityUtils.secure_compare(a, b) end def self.deprecator diff --git a/test/devise_test.rb b/test/devise_test.rb index 532aa57d..2f98bb4f 100644 --- a/test/devise_test.rb +++ b/test/devise_test.rb @@ -86,15 +86,20 @@ class DeviseTest < ActiveSupport::TestCase Devise::CONTROLLERS.delete(:kivi) end - test 'should complain when comparing empty or different sized passes' do + test 'Devise.secure_compare fails when comparing different strings or nil' do [nil, ""].each do |empty| assert_not Devise.secure_compare(empty, "something") assert_not Devise.secure_compare("something", empty) - assert_not Devise.secure_compare(empty, empty) end + assert_not Devise.secure_compare(nil, nil) assert_not Devise.secure_compare("size_1", "size_four") end + test 'Devise.secure_compare passes when strings are the same, even two empty strings' do + assert Devise.secure_compare("", "") + assert Devise.secure_compare("something", "something") + end + test 'Devise.email_regexp should match valid email addresses' do valid_emails = ["test@example.com", "jo@jo.co", "f4$_m@you.com", "testing.example@example.com.ua", "test@tt", "test@valid---domain.com"] non_valid_emails = ["rex", "test user@example.com", "test_user@example server.com"]