Issue #15061: Don't oversell the capabilities of the new non-shortcircuiting comparison function in hmac

4 files changed, 67 insertions, 51 deletions

diff --git a/Doc/library/hmac.rst b/Doc/library/hmac.rst
index 8274bb1..e6ce99b 100644
--- a/Doc/library/hmac.rst
+++ b/Doc/library/hmac.rst
@@ -42,8 +42,8 @@ An HMAC object has the following methods:
 
       When comparing the output of :meth:`digest` to an externally-supplied
       digest during a verification routine, it is recommended to use the
-      :func:`hmac.secure_compare` function instead of the ``==`` operator
-      to avoid potential timing attacks.
+      :func:`compare_digest` function instead of the ``==`` operator
+      to reduce the vulnerability to timing attacks.
 
 
 .. method:: HMAC.hexdigest()
@@ -54,10 +54,11 @@ An HMAC object has the following methods:
 
    .. warning::
 
-      When comparing the output of :meth:`hexdigest` to an externally-supplied
-      digest during a verification routine, it is recommended to use the
-      :func:`hmac.secure_compare` function instead of the ``==`` operator
-      to avoid potential timing attacks.
+      The output of :meth:`hexdigest` should not be compared directly to an
+      externally-supplied digest during a verification routine. Instead, the
+      externally supplied digest should be converted to a :class:`bytes`
+      value and compared to the output of :meth:`digest` with
+      :func:`compare_digest`.
 
 
 .. method:: HMAC.copy()
@@ -68,20 +69,28 @@ An HMAC object has the following methods:
 
 This module also provides the following helper function:
 
-.. function:: secure_compare(a, b)
+.. function:: compare_digest(a, b)
+
+   Returns the equivalent of ``a == b``, but avoids content based
+   short circuiting behaviour to reduce the vulnerability to timing
+   analysis. The inputs must be :class:`bytes` instances.
 
-   Returns the equivalent of ``a == b``, but using a time-independent
-   comparison method. Comparing the full lengths of the inputs *a* and *b*,
-   instead of short-circuiting the comparison upon the first unequal byte,
-   prevents leaking information about the inputs being compared and mitigates
-   potential timing attacks. The inputs must be either :class:`str` or
-   :class:`bytes` instances.
+   Using a short circuiting comparison (that is, one that terminates as soon
+   as it finds any difference between the values) to check digests for
+   correctness can be problematic, as it introduces a potential
+   vulnerability when an attacker can control both the message to be checked
+   *and* the purported signature value. By keeping the plaintext consistent
+   and supplying different signature values, an attacker may be able to use
+   timing variations to search the signature space for the expected value in
+   O(n) time rather than the desired O(2**n).
 
    .. note::
 
-      While the :func:`hmac.secure_compare` function prevents leaking the
-      contents of the inputs via a timing attack, it does leak the length
-      of the inputs. However, this generally is not a security risk.
+      While this function reduces the likelihood of leaking the contents of
+      the expected digest via a timing attack, it still uses short circuiting
+      behaviour based on the *length* of the inputs. It is assumed that the
+      expected length of the digest is not a secret, as it is typically
+      published as part of a file format, network protocol or API definition.
 
    .. versionadded:: 3.3
 
diff --git a/Lib/hmac.py b/Lib/hmac.py
index 13ffdbe..e47965b 100644
--- a/Lib/hmac.py
+++ b/Lib/hmac.py
@@ -13,24 +13,24 @@ trans_36 = bytes((x ^ 0x36) for x in range(256))
 digest_size = None
 
 
-def secure_compare(a, b):
-    """Returns the equivalent of 'a == b', but using a time-independent
-    comparison method to prevent timing attacks."""
-    if not ((isinstance(a, str) and isinstance(b, str)) or
-            (isinstance(a, bytes) and isinstance(b, bytes))):
-        raise TypeError("inputs must be strings or bytes")
-
+def compare_digest(a, b):
+    """Returns the equivalent of 'a == b', but avoids content based short
+    circuiting to reduce the vulnerability to timing attacks."""
+    # Consistent timing matters more here than data type flexibility
+    if not (isinstance(a, bytes) and isinstance(b, bytes)):
+        raise TypeError("inputs must be bytes instances")
+
+    # We assume the length of the expected digest is public knowledge,
+    # thus this early return isn't leaking anything an attacker wouldn't
+    # already know
     if len(a) != len(b):
         return False
 
+    # We assume that integers in the bytes range are all cached,
+    # thus timing shouldn't vary much due to integer object creation
     result = 0
-    if isinstance(a, bytes):
-        for x, y in zip(a, b):
-            result |= x ^ y
-    else:
-        for x, y in zip(a, b):
-            result |= ord(x) ^ ord(y)
-
+    for x, y in zip(a, b):
+        result |= x ^ y
     return result == 0
 
 
diff --git a/Lib/test/test_hmac.py b/Lib/test/test_hmac.py
index 042bc5d..4e5961d 100644
--- a/Lib/test/test_hmac.py
+++ b/Lib/test/test_hmac.py
@@ -302,40 +302,42 @@ class CopyTestCase(unittest.TestCase):
         self.assertEqual(h1.hexdigest(), h2.hexdigest(),
             "Hexdigest of copy doesn't match original hexdigest.")
 
-class SecureCompareTestCase(unittest.TestCase):
+class CompareDigestTestCase(unittest.TestCase):
 
     def test_compare(self):
         # Testing input type exception handling
         a, b = 100, 200
-        self.assertRaises(TypeError, hmac.secure_compare, a, b)
-        a, b = 100, "foobar"
-        self.assertRaises(TypeError, hmac.secure_compare, a, b)
+        self.assertRaises(TypeError, hmac.compare_digest, a, b)
+        a, b = 100, b"foobar"
+        self.assertRaises(TypeError, hmac.compare_digest, a, b)
+        a, b = b"foobar", 200
+        self.assertRaises(TypeError, hmac.compare_digest, a, b)
         a, b = "foobar", b"foobar"
-        self.assertRaises(TypeError, hmac.secure_compare, a, b)
+        self.assertRaises(TypeError, hmac.compare_digest, a, b)
+        a, b = b"foobar", "foobar"
+        self.assertRaises(TypeError, hmac.compare_digest, a, b)
+        a, b = "foobar", "foobar"
+        self.assertRaises(TypeError, hmac.compare_digest, a, b)
+        a, b = bytearray(b"foobar"), bytearray(b"foobar")
+        self.assertRaises(TypeError, hmac.compare_digest, a, b)
 
-        # Testing str/bytes of different lengths
-        a, b = "foobar", "foo"
-        self.assertFalse(hmac.secure_compare(a, b))
+        # Testing bytes of different lengths
         a, b = b"foobar", b"foo"
-        self.assertFalse(hmac.secure_compare(a, b))
+        self.assertFalse(hmac.compare_digest(a, b))
         a, b = b"\xde\xad\xbe\xef", b"\xde\xad"
-        self.assertFalse(hmac.secure_compare(a, b))
+        self.assertFalse(hmac.compare_digest(a, b))
 
-        # Testing str/bytes of same lengths, different values
-        a, b = "foobar", "foobaz"
-        self.assertFalse(hmac.secure_compare(a, b))
+        # Testing bytes of same lengths, different values
         a, b = b"foobar", b"foobaz"
-        self.assertFalse(hmac.secure_compare(a, b))
+        self.assertFalse(hmac.compare_digest(a, b))
         a, b = b"\xde\xad\xbe\xef", b"\xab\xad\x1d\xea"
-        self.assertFalse(hmac.secure_compare(a, b))
+        self.assertFalse(hmac.compare_digest(a, b))
 
-        # Testing str/bytes of same lengths, same values
-        a, b = "foobar", "foobar"
-        self.assertTrue(hmac.secure_compare(a, b))
+        # Testing bytes of same lengths, same values
         a, b = b"foobar", b"foobar"
-        self.assertTrue(hmac.secure_compare(a, b))
+        self.assertTrue(hmac.compare_digest(a, b))
         a, b = b"\xde\xad\xbe\xef", b"\xde\xad\xbe\xef"
-        self.assertTrue(hmac.secure_compare(a, b))
+        self.assertTrue(hmac.compare_digest(a, b))
 
 def test_main():
     support.run_unittest(
@@ -343,7 +345,7 @@ def test_main():
         ConstructorTestCase,
         SanityTestCase,
         CopyTestCase,
-        SecureCompareTestCase
+        CompareDigestTestCase
     )
 
 if __name__ == "__main__":
diff --git a/Misc/NEWS b/Misc/NEWS
index 058555f..7d7d434 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -21,6 +21,11 @@ Core and Builtins
 Library
 -------
 
+- Issue #15061: The inappropriately named hmac.secure_compare has been
+  renamed to hash.compare_digest, restricted to operating on bytes inputs
+  only and had its documentation updated to more acurrately reflect both its
+  intent and its limitations
+
 - Issue #13841: Make child processes exit using sys.exit() on Windows.
 
 - Issue #14936: curses_panel was converted to PEP 3121 and PEP 384 API.