From 3261e41b3d16e0f5dad4045485edd1c2b0abd5c1 Mon Sep 17 00:00:00 2001 From: Jack McCracken Date: Mon, 4 May 2020 13:30:45 -0400 Subject: [PATCH] Remove hashing from secure_compare and clarify documentation --- activesupport/lib/active_support/security_utils.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/activesupport/lib/active_support/security_utils.rb b/activesupport/lib/active_support/security_utils.rb index 5e455fca5775e..2693f3440c645 100644 --- a/activesupport/lib/active_support/security_utils.rb +++ b/activesupport/lib/active_support/security_utils.rb @@ -19,12 +19,14 @@ def fixed_length_secure_compare(a, b) end module_function :fixed_length_secure_compare - # Constant time string comparison, for variable length strings. + # Secure string comparison for strings of variable length. # - # The values are first processed by SHA256, so that we don't leak length info - # via timing attacks. + # While a timing attack would not be able to discern the content of + # a secret compared via secure_compare, it is possible to determine + # the secret length. This should be considered when using secure_compare + # to compare weak, short secrets to user input. def secure_compare(a, b) - fixed_length_secure_compare(::Digest::SHA256.digest(a), ::Digest::SHA256.digest(b)) && a == b + a.length == b.length && fixed_length_secure_compare(a, b) end module_function :secure_compare end