Rename memcmp? to secure_compare.

Minor improvements to formatting and documentation.
ruby · Oct 26, 2019 · 1ade643 · 1ade643
1 parent e7ed01b
commit 1ade643
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 30 deletions.
diff --git 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
@@ -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