From 446b8e25d992c7e3b3d48f7e676ad9e70ed93c96 Mon Sep 17 00:00:00 2001 From: Bart de Water Date: Sat, 26 Oct 2019 11:32:34 -0400 Subject: [PATCH 1/2] Rename OpenSSL.secure_compare to fixed_length_secure_compare In 1ade643cbc01f3f7bd96e90bd8837df7ed491a09 the Rails-like secure_compare naming was adopted and in original pull request introducing this functionality debate around timing of hash functions followed. This made me realize why Rails' default of hashing the values to protect users from making mistakes is a good idea. --- ext/openssl/ossl.c | 28 ++++++++-------------------- test/test_ossl.rb | 36 ++++++++++++++++++------------------ 2 files changed, 26 insertions(+), 38 deletions(-) diff --git a/ext/openssl/ossl.c b/ext/openssl/ossl.c index bfc4065ae..bdf6053dc 100644 --- a/ext/openssl/ossl.c +++ b/ext/openssl/ossl.c @@ -606,29 +606,17 @@ static void Init_ossl_locks(void) /* * call-seq: - * OpenSSL.secure_compare(string, string) -> boolean + * OpenSSL.fixed_length_secure_compare(string, string) -> boolean * - * Constant time memory comparison. Inputs must be of equal length, otherwise - * an error is raised since timing attacks could leak the length of a - * secret. + * Constant time memory comparison for fixed length strings, such as results + * of HMAC calculations. * - * Returns +true+ if the strings are identical, +false+ otherwise. - * - * 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 = "..." - * secret = "..." - * hashed_input = OpenSSL::Digest::SHA256.digest(user_input) - * hashed_secret = OpenSSL::Digest::SHA256.digest(secret) - * OpenSSL.secure_compare(hashed_input, hashed_secret) && user_input == secret - * - * Be aware that timing attacks against the hash functions may reveal the - * length of the secret. + * Returns +true+ if the strings are identical, +false+ if they are of the same + * length but not identical. If the length is different, +ArgumentError+ is + * raised. */ static VALUE -ossl_crypto_secure_compare(VALUE dummy, VALUE str1, VALUE str2) +ossl_crypto_fixed_length_secure_compare(VALUE dummy, VALUE str1, VALUE str2) { const unsigned char *p1 = (const unsigned char *)StringValuePtr(str1); const unsigned char *p2 = (const unsigned char *)StringValuePtr(str2); @@ -1166,7 +1154,7 @@ Init_openssl(void) */ mOSSL = rb_define_module("OpenSSL"); rb_global_variable(&mOSSL); - rb_define_singleton_method(mOSSL, "secure_compare", ossl_crypto_secure_compare, 2); + rb_define_singleton_method(mOSSL, "fixed_length_secure_compare", ossl_crypto_fixed_length_secure_compare, 2); /* * OpenSSL ruby extension version diff --git a/test/test_ossl.rb b/test/test_ossl.rb index b23b37925..85363cb5c 100644 --- a/test/test_ossl.rb +++ b/test/test_ossl.rb @@ -6,28 +6,28 @@ if defined?(OpenSSL) class OpenSSL::OSSL < OpenSSL::SSLTestCase - def test_secure_compare - assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "a") } - assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "aa") } + def test_fixed_length_secure_compare + assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "a") } + assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "aa") } - assert OpenSSL.secure_compare("aaa", "aaa") - assert OpenSSL.secure_compare( + assert OpenSSL.fixed_length_secure_compare("aaa", "aaa") + assert OpenSSL.fixed_length_secure_compare( OpenSSL::Digest::SHA256.digest("aaa"), OpenSSL::Digest::SHA256.digest("aaa") ) - assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "aaaa") } - refute OpenSSL.secure_compare("aaa", "baa") - refute OpenSSL.secure_compare("aaa", "aba") - refute OpenSSL.secure_compare("aaa", "aab") - assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "aaab") } - assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "b") } - assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "bb") } - refute OpenSSL.secure_compare("aaa", "bbb") - assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "bbbb") } + assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "aaaa") } + refute OpenSSL.fixed_length_secure_compare("aaa", "baa") + refute OpenSSL.fixed_length_secure_compare("aaa", "aba") + refute OpenSSL.fixed_length_secure_compare("aaa", "aab") + assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "aaab") } + assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "b") } + assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "bb") } + refute OpenSSL.fixed_length_secure_compare("aaa", "bbb") + assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "bbbb") } end def test_memcmp_timing - # Ensure using secure_compare takes almost exactly the same amount of time to compare two different strings. + # Ensure using fixed_length_secure_compare takes almost exactly the same amount of time to compare two different strings. # Regular string comparison will short-circuit on the first non-matching character, failing this test. # NOTE: this test may be susceptible to noise if the system running the tests is otherwise under load. a = "x" * 512_000 @@ -36,9 +36,9 @@ def test_memcmp_timing a = "#{a}x" n = 10_000 - a_b_time = Benchmark.measure { n.times { OpenSSL.secure_compare(a, b) } }.real - a_c_time = Benchmark.measure { n.times { OpenSSL.secure_compare(a, c) } }.real - assert_in_delta(a_b_time, a_c_time, 1, "secure_compare timing test failed") + a_b_time = Benchmark.measure { n.times { OpenSSL.fixed_length_secure_compare(a, b) } }.real + a_c_time = Benchmark.measure { n.times { OpenSSL.fixed_length_secure_compare(a, c) } }.real + assert_in_delta(a_b_time, a_c_time, 1, "fixed_length_secure_compare timing test failed") end end From 9a91192e72969ac5b4b09d3123149fcb6adf6f9c Mon Sep 17 00:00:00 2001 From: Bart de Water Date: Sat, 26 Oct 2019 12:13:25 -0400 Subject: [PATCH 2/2] Add OpenSSL.secure_compare with same semantics as Active Support >= 5.2 secure_compare is for user input, fixed_length_secure_compare for already processed data that is known to have the same length --- lib/openssl.rb | 14 ++++++++++++++ test/test_ossl.rb | 17 +++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/lib/openssl.rb b/lib/openssl.rb index 091428292..47a8fc496 100644 --- a/lib/openssl.rb +++ b/lib/openssl.rb @@ -20,3 +20,17 @@ require 'openssl/x509' require 'openssl/ssl' require 'openssl/pkcs5' + +module OpenSSL + # call-seq: + # OpenSSL.secure_compare(string, string) -> boolean + # + # Constant time memory comparison. Inputs are hashed using SHA-256 to mask + # the length of the secret. Returns +true+ if the strings are identical, + # +false+ otherwise. + def self.secure_compare(a, b) + hashed_a = OpenSSL::Digest::SHA256.digest(a) + hashed_b = OpenSSL::Digest::SHA256.digest(b) + OpenSSL.fixed_length_secure_compare(hashed_a, hashed_b) && a == b + end +end diff --git a/test/test_ossl.rb b/test/test_ossl.rb index 85363cb5c..394f30e0d 100644 --- a/test/test_ossl.rb +++ b/test/test_ossl.rb @@ -26,6 +26,23 @@ def test_fixed_length_secure_compare assert_raises(ArgumentError) { OpenSSL.fixed_length_secure_compare("aaa", "bbbb") } end + def test_secure_compare + refute OpenSSL.secure_compare("aaa", "a") + refute OpenSSL.secure_compare("aaa", "aa") + + assert OpenSSL.secure_compare("aaa", "aaa") + + refute OpenSSL.secure_compare("aaa", "aaaa") + refute OpenSSL.secure_compare("aaa", "baa") + refute OpenSSL.secure_compare("aaa", "aba") + refute OpenSSL.secure_compare("aaa", "aab") + refute OpenSSL.secure_compare("aaa", "aaab") + refute OpenSSL.secure_compare("aaa", "b") + refute OpenSSL.secure_compare("aaa", "bb") + refute OpenSSL.secure_compare("aaa", "bbb") + refute OpenSSL.secure_compare("aaa", "bbbb") + end + def test_memcmp_timing # Ensure using fixed_length_secure_compare takes almost exactly the same amount of time to compare two different strings. # Regular string comparison will short-circuit on the first non-matching character, failing this test.