From 807770ec1bead8aff0716384621638ed80e9f56b Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Fri, 15 Jun 2012 21:14:08 +1000 Subject: [PATCH] Issue #15061: Don't oversell the capabilities of the new non-shortcircuiting comparison function in hmac --- Doc/library/hmac.rst | 41 ++++++++++++++++++------------- Lib/hmac.py | 26 ++++++++++---------- Lib/test/test_hmac.py | 56 ++++++++++++++++++++++--------------------- Misc/NEWS | 5 ++++ 4 files changed, 72 insertions(+), 56 deletions(-) diff --git a/Doc/library/hmac.rst b/Doc/library/hmac.rst index 8274bb1e3dd..e6ce99b669a 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 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. + 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. + + 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 13ffdbe14b2..e47965b1cf9 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 042bc5d8f7a..4e5961db71d 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) - - # Testing str/bytes of different lengths - a, b = "foobar", "foo" - self.assertFalse(hmac.secure_compare(a, b)) - a, b = b"foobar", b"foo" - self.assertFalse(hmac.secure_compare(a, b)) - a, b = b"\xde\xad\xbe\xef", b"\xde\xad" - self.assertFalse(hmac.secure_compare(a, b)) - - # Testing str/bytes of same lengths, different values - a, b = "foobar", "foobaz" - self.assertFalse(hmac.secure_compare(a, b)) - a, b = b"foobar", b"foobaz" - self.assertFalse(hmac.secure_compare(a, b)) - a, b = b"\xde\xad\xbe\xef", b"\xab\xad\x1d\xea" - self.assertFalse(hmac.secure_compare(a, b)) - - # Testing str/bytes of same lengths, same values + 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.assertTrue(hmac.secure_compare(a, b)) + 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 bytes of different lengths + a, b = b"foobar", b"foo" + self.assertFalse(hmac.compare_digest(a, b)) + a, b = b"\xde\xad\xbe\xef", b"\xde\xad" + self.assertFalse(hmac.compare_digest(a, b)) + + # Testing bytes of same lengths, different values + a, b = b"foobar", b"foobaz" + self.assertFalse(hmac.compare_digest(a, b)) + a, b = b"\xde\xad\xbe\xef", b"\xab\xad\x1d\xea" + self.assertFalse(hmac.compare_digest(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 058555f4452..7d7d4344ad1 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.