Skip to content

bpo-9216: hashlib usedforsecurity fixes #20258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Lib/hashlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ def __hash_new(name, data=b'', **kwargs):
# salt, personal, tree hashing or SSE.
return __get_builtin_constructor(name)(data, **kwargs)
try:
return _hashlib.new(name, data)
return _hashlib.new(name, data, **kwargs)
except ValueError:
# If the _hashlib module (OpenSSL) doesn't support the named
# hash, try using our builtin implementations.
Expand Down
114 changes: 82 additions & 32 deletions Lib/test/test_hashlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import itertools
import os
import sys
import sysconfig
import threading
import unittest
import warnings
Expand All @@ -26,11 +27,20 @@
c_hashlib = import_fresh_module('hashlib', fresh=['_hashlib'])
py_hashlib = import_fresh_module('hashlib', blocked=['_hashlib'])

builtin_hashes = sysconfig.get_config_var("PY_BUILTIN_HASHLIB_HASHES")
if builtin_hashes is None:
builtin_hashes = {'md5', 'sha1', 'sha256', 'sha512', 'sha3', 'blake2'}
else:
builtin_hashes = {
m.strip() for m in builtin_hashes.strip('"').lower().split(",")
}

try:
from _hashlib import HASH, HASHXOF
from _hashlib import HASH, HASHXOF, openssl_md_meth_names
except ImportError:
HASH = None
HASHXOF = None
openssl_md_meth_names = frozenset()

try:
import _blake2
Expand Down Expand Up @@ -175,10 +185,17 @@ def hash_constructors(self):
constructors = self.constructors_to_test.values()
return itertools.chain.from_iterable(constructors)

@property
def is_fips_mode(self):
if hasattr(self._hashlib, "get_fips_mode"):
return self._hashlib.get_fips_mode()
else:
return None

def test_hash_array(self):
a = array.array("b", range(10))
for cons in self.hash_constructors:
c = cons(a)
c = cons(a, usedforsecurity=False)
if c.name in self.shakes:
c.hexdigest(16)
else:
Expand All @@ -193,14 +210,26 @@ def test_algorithms_available(self):
self.assertTrue(set(hashlib.algorithms_guaranteed).
issubset(hashlib.algorithms_available))

def test_usedforsecurity(self):
def test_usedforsecurity_true(self):
hashlib.new("sha256", usedforsecurity=True)
if self.is_fips_mode:
self.skipTest("skip in FIPS mode")
for cons in self.hash_constructors:
cons(usedforsecurity=True)
cons(usedforsecurity=False)
cons(b'', usedforsecurity=True)
cons(b'', usedforsecurity=False)
hashlib.new("sha256", usedforsecurity=True)
hashlib.new("md5", usedforsecurity=True)
hashlib.md5(usedforsecurity=True)
if self._hashlib is not None:
self._hashlib.new("md5", usedforsecurity=True)
self._hashlib.openssl_md5(usedforsecurity=True)

def test_usedforsecurity_false(self):
hashlib.new("sha256", usedforsecurity=False)
for cons in self.hash_constructors:
cons(usedforsecurity=False)
cons(b'', usedforsecurity=False)
hashlib.new("md5", usedforsecurity=False)
hashlib.md5(usedforsecurity=False)
if self._hashlib is not None:
self._hashlib.new("md5", usedforsecurity=False)
self._hashlib.openssl_md5(usedforsecurity=False)
Expand Down Expand Up @@ -240,7 +269,7 @@ def test_get_builtin_constructor(self):

def test_hexdigest(self):
for cons in self.hash_constructors:
h = cons()
h = cons(usedforsecurity=False)
if h.name in self.shakes:
self.assertIsInstance(h.digest(16), bytes)
self.assertEqual(hexstr(h.digest(16)), h.hexdigest(16))
Expand All @@ -252,7 +281,7 @@ def test_digest_length_overflow(self):
# See issue #34922
large_sizes = (2**29, 2**32-10, 2**32+10, 2**61, 2**64-10, 2**64+10)
for cons in self.hash_constructors:
h = cons()
h = cons(usedforsecurity=False)
if h.name not in self.shakes:
continue
if HASH is not None and isinstance(h, HASH):
Expand All @@ -266,13 +295,16 @@ def test_digest_length_overflow(self):

def test_name_attribute(self):
for cons in self.hash_constructors:
h = cons()
h = cons(usedforsecurity=False)
self.assertIsInstance(h.name, str)
if h.name in self.supported_hash_names:
self.assertIn(h.name, self.supported_hash_names)
else:
self.assertNotIn(h.name, self.supported_hash_names)
self.assertEqual(h.name, hashlib.new(h.name).name)
self.assertEqual(
h.name,
hashlib.new(h.name, usedforsecurity=False).name
)

def test_large_update(self):
aas = b'a' * 128
Expand All @@ -281,7 +313,7 @@ def test_large_update(self):
dees = b'd' * 2048 # HASHLIB_GIL_MINSIZE

for cons in self.hash_constructors:
m1 = cons()
m1 = cons(usedforsecurity=False)
m1.update(aas)
m1.update(bees)
m1.update(cees)
Expand All @@ -291,15 +323,15 @@ def test_large_update(self):
else:
args = ()

m2 = cons()
m2 = cons(usedforsecurity=False)
m2.update(aas + bees + cees + dees)
self.assertEqual(m1.digest(*args), m2.digest(*args))

m3 = cons(aas + bees + cees + dees)
m3 = cons(aas + bees + cees + dees, usedforsecurity=False)
self.assertEqual(m1.digest(*args), m3.digest(*args))

# verify copy() doesn't touch original
m4 = cons(aas + bees + cees)
m4 = cons(aas + bees + cees, usedforsecurity=False)
m4_digest = m4.digest(*args)
m4_copy = m4.copy()
m4_copy.update(dees)
Expand Down Expand Up @@ -359,7 +391,7 @@ def check_blocksize_name(self, name, block_size=0, digest_size=0,
digest_length=None):
constructors = self.constructors_to_test[name]
for hash_object_constructor in constructors:
m = hash_object_constructor()
m = hash_object_constructor(usedforsecurity=False)
self.assertEqual(m.block_size, block_size)
self.assertEqual(m.digest_size, digest_size)
if digest_length:
Expand Down Expand Up @@ -418,15 +450,24 @@ def test_blocksize_name_blake2(self):
self.check_blocksize_name('blake2s', 64, 32)

def test_case_md5_0(self):
self.check('md5', b'', 'd41d8cd98f00b204e9800998ecf8427e')
self.check(
'md5', b'', 'd41d8cd98f00b204e9800998ecf8427e',
usedforsecurity=False
)

def test_case_md5_1(self):
self.check('md5', b'abc', '900150983cd24fb0d6963f7d28e17f72')
self.check(
'md5', b'abc', '900150983cd24fb0d6963f7d28e17f72',
usedforsecurity=False
)

def test_case_md5_2(self):
self.check('md5',
b'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789',
'd174ab98d277d9f5a5611c2c9f419d9f')
self.check(
'md5',
b'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789',
'd174ab98d277d9f5a5611c2c9f419d9f',
usedforsecurity=False
)

@unittest.skipIf(sys.maxsize < _4G + 5, 'test cannot run on 32-bit systems')
@bigmemtest(size=_4G + 5, memuse=1, dry_run=False)
Expand Down Expand Up @@ -806,22 +847,28 @@ def test_gil(self):
gil_minsize = 2048

for cons in self.hash_constructors:
m = cons()
m = cons(usedforsecurity=False)
m.update(b'1')
m.update(b'#' * gil_minsize)
m.update(b'1')

m = cons(b'x' * gil_minsize)
m = cons(b'x' * gil_minsize, usedforsecurity=False)
m.update(b'1')

m = hashlib.md5()
m = hashlib.sha256()
m.update(b'1')
m.update(b'#' * gil_minsize)
m.update(b'1')
self.assertEqual(m.hexdigest(), 'cb1e1a2cbc80be75e19935d621fb9b21')
self.assertEqual(
m.hexdigest(),
'1cfceca95989f51f658e3f3ffe7f1cd43726c9e088c13ee10b46f57cef135b94'
)

m = hashlib.md5(b'x' * gil_minsize)
self.assertEqual(m.hexdigest(), 'cfb767f225d58469c5de3632a8803958')
m = hashlib.sha256(b'1' + b'#' * gil_minsize + b'1')
self.assertEqual(
m.hexdigest(),
'1cfceca95989f51f658e3f3ffe7f1cd43726c9e088c13ee10b46f57cef135b94'
)

@support.reap_threads
def test_threaded_hashing(self):
Expand Down Expand Up @@ -859,10 +906,10 @@ def hash_in_chunks(chunk_size):

self.assertEqual(expected_hash, hasher.hexdigest())

@unittest.skipUnless(hasattr(c_hashlib, 'get_fips_mode'),
'need _hashlib.get_fips_mode')
def test_get_fips_mode(self):
self.assertIsInstance(c_hashlib.get_fips_mode(), int)
fips_mode = self.is_fips_mode
if fips_mode is not None:
self.assertIsInstance(fips_mode, int)

@unittest.skipUnless(HASH is not None, 'need _hashlib')
def test_internal_types(self):
Expand Down Expand Up @@ -934,8 +981,10 @@ class KDFTests(unittest.TestCase):
(bytes.fromhex('9d9e9c4cd21fe4be24d5b8244c759665'), None),],
}

def _test_pbkdf2_hmac(self, pbkdf2):
def _test_pbkdf2_hmac(self, pbkdf2, supported):
for digest_name, results in self.pbkdf2_results.items():
if digest_name not in supported:
continue
for i, vector in enumerate(self.pbkdf2_test_vectors):
password, salt, rounds, dklen = vector
expected, overwrite_dklen = results[i]
Expand All @@ -946,6 +995,7 @@ def _test_pbkdf2_hmac(self, pbkdf2):
(digest_name, password, salt, rounds, dklen))
out = pbkdf2(digest_name, memoryview(password),
memoryview(salt), rounds, dklen)
self.assertEqual(out, expected)
out = pbkdf2(digest_name, bytearray(password),
bytearray(salt), rounds, dklen)
self.assertEqual(out, expected)
Expand All @@ -967,12 +1017,12 @@ def _test_pbkdf2_hmac(self, pbkdf2):
self.assertEqual(out, self.pbkdf2_results['sha1'][0][0])

def test_pbkdf2_hmac_py(self):
self._test_pbkdf2_hmac(py_hashlib.pbkdf2_hmac)
self._test_pbkdf2_hmac(py_hashlib.pbkdf2_hmac, builtin_hashes)

@unittest.skipUnless(hasattr(c_hashlib, 'pbkdf2_hmac'),
' test requires OpenSSL > 1.0')
def test_pbkdf2_hmac_c(self):
self._test_pbkdf2_hmac(c_hashlib.pbkdf2_hmac)
self._test_pbkdf2_hmac(c_hashlib.pbkdf2_hmac, openssl_md_meth_names)


@unittest.skipUnless(hasattr(c_hashlib, 'scrypt'),
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_smtplib.py
Original file line number Diff line number Diff line change
Expand Up @@ -1067,6 +1067,7 @@ def testAUTH_CRAM_MD5(self):
self.assertEqual(resp, (235, b'Authentication Succeeded'))
smtp.close()

@hashlib_helper.requires_hashdigest('md5')
def testAUTH_multiple(self):
# Test that multiple authentication methods are tried.
self.serv.add_feature("AUTH BOGUS PLAIN LOGIN CRAM-MD5")
Expand Down
2 changes: 2 additions & 0 deletions Lib/test/test_tools/test_md5sum.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
import os
import unittest
from test import support
from test.support import hashlib_helper
from test.support.script_helper import assert_python_ok, assert_python_failure

from test.test_tools import scriptsdir, skip_if_missing

skip_if_missing()

@hashlib_helper.requires_hashdigest('md5')
class MD5SumTests(unittest.TestCase):
@classmethod
def setUpClass(cls):
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_urllib2_localnet.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,14 +316,14 @@ def test_basic_auth_httperror(self):
self.assertRaises(urllib.error.HTTPError, urllib.request.urlopen, self.server_url)


@hashlib_helper.requires_hashdigest("md5")
class ProxyAuthTests(unittest.TestCase):
URL = "http://localhost"

USER = "tester"
PASSWD = "test123"
REALM = "TestRealm"

@hashlib_helper.requires_hashdigest("md5")
def setUp(self):
super(ProxyAuthTests, self).setUp()
# Ignore proxy bypass settings in the environment.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
func:`hashlib.new` passed ``usedforsecurity`` to OpenSSL EVP constructor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user reading this shouldn't need to be aware of the internal implementation details behind the Python hashlib "usedforsecurity" construct. I'd just simplify this news entry:

func:`hashlib.new` now passes the ``usedforsecurity`` flag to the internal OpenSSL hash constructors.

and probably omit the final "test_hashlib and test_smtplib handle strict security policy better." part entirely as we don't need a NEWS entry to mention updates to our internal test suite. (though it is harmless to do so)

``_hashlib.new()``. test_hashlib and test_smtplib handle strict security
policy better.