From 1ade643cbc01f3f7bd96e90bd8837df7ed491a09 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Sat, 26 Oct 2019 15:28:39 +1300 Subject: [PATCH] Rename `memcmp?` to `secure_compare`. Minor improvements to formatting and documentation. --- ext/openssl/ossl.c | 30 ++++++++++++++++++------------ test/test_ossl.rb | 36 ++++++++++++++++++------------------ 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/ext/openssl/ossl.c b/ext/openssl/ossl.c index c67492678..bfc4065ae 100644 --- a/ext/openssl/ossl.c +++ b/ext/openssl/ossl.c @@ -606,36 +606,42 @@ static void Init_ossl_locks(void) /* * call-seq: - * OpenSSL.memcmp?(string, string) -> boolean + * OpenSSL.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. * + * 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 = "..." - * expected = "..." - * hashed_input = OpenSSL::Digest::SHA256.digest(user_input) - * hashed_expected = OpenSSL::Digest::SHA256.digest(expected) - * OpenSSL.memcmp?(hashed_input, hashed_expected) && user_input == expected + * 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. */ static VALUE -ossl_crypto_memcmp(VALUE dummy, VALUE str1, VALUE str2) +ossl_crypto_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); long len1 = RSTRING_LEN(str1); long len2 = RSTRING_LEN(str2); - if(len1 != len2) - ossl_raise(rb_eArgError, "inputs must be of equal length"); + if (len1 != len2) { + ossl_raise(rb_eArgError, "inputs must be of equal length"); + } switch (CRYPTO_memcmp(p1, p2, len1)) { - case 0: return Qtrue; - default: return Qfalse; + case 0: return Qtrue; + default: return Qfalse; } } @@ -1160,7 +1166,7 @@ Init_openssl(void) */ mOSSL = rb_define_module("OpenSSL"); rb_global_variable(&mOSSL); - rb_define_singleton_method(mOSSL, "memcmp?", ossl_crypto_memcmp, 2); + rb_define_singleton_method(mOSSL, "secure_compare", ossl_crypto_secure_compare, 2); /* * OpenSSL ruby extension version diff --git a/test/test_ossl.rb b/test/test_ossl.rb index bcefef413..b23b37925 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_memcmp? - assert_raises(ArgumentError) { OpenSSL.memcmp?("aaa", "a") } - assert_raises(ArgumentError) { OpenSSL.memcmp?("aaa", "aa") } + def test_secure_compare + assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "a") } + assert_raises(ArgumentError) { OpenSSL.secure_compare("aaa", "aa") } - assert OpenSSL.memcmp?("aaa", "aaa") - assert OpenSSL.memcmp?( + assert OpenSSL.secure_compare("aaa", "aaa") + assert OpenSSL.secure_compare( OpenSSL::Digest::SHA256.digest("aaa"), OpenSSL::Digest::SHA256.digest("aaa") ) - assert_raises(ArgumentError) { OpenSSL.memcmp?("aaa", "aaaa") } - refute OpenSSL.memcmp?("aaa", "baa") - refute OpenSSL.memcmp?("aaa", "aba") - refute OpenSSL.memcmp?("aaa", "aab") - assert_raises(ArgumentError) { OpenSSL.memcmp?("aaa", "aaab") } - assert_raises(ArgumentError) { OpenSSL.memcmp?("aaa", "b") } - assert_raises(ArgumentError) { OpenSSL.memcmp?("aaa", "bb") } - refute OpenSSL.memcmp?("aaa", "bbb") - assert_raises(ArgumentError) { OpenSSL.memcmp?("aaa", "bbbb") } + 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") } end def test_memcmp_timing - # Ensure using memcmp? takes almost exactly the same amount of time to compare two different strings. + # Ensure using 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.memcmp?(a, b) } }.real - a_c_time = Benchmark.measure { n.times { OpenSSL.memcmp?(a, c) } }.real - assert_in_delta(a_b_time, a_c_time, 1, "memcmp? timing test failed") + 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") end end