Repository navigation
FIPS support for hashlib #53462
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2010-07-10.00:22:13.168>
labels = ['type-feature', 'library', '3.9', '3.10']
title = 'FIPS support for hashlib'
updated_at = <Date 2020-05-22.18:22:39.610>
user = 'https://github.com/davidmalcolm'bugs.python.org fields:
activity = <Date 2020-05-22.18:22:39.610>
actor = 'miss-islington'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2010-07-10.00:22:13.168>
creator = 'dmalcolm'
dependencies = []
files = ['17970', '23172', '23173', '23174', '23175', '26205']
hgrepos = []
issue_num = 9216
keywords = ['patch']
message_count = 47.0
messages = ['109808', '109891', '110124', '144152', '144153', '144154', '155688', '155741', '155846', '164308', '164328', '166575', '166576', '172302', '191130', '285593', '285640', '285643', '285676', '285677', '285678', '285679', '285681', '285683', '285684', '285706', '285895', '285897', '298454', '298484', '348186', '348747', '352231', '352253', '360716', '366436', '367189', '367193', '367194', '367195', '367197', '367668', '367960', '368065', '369430', '369622', '369625']
nosy_count = 19.0
nosy_names = ['gregory.p.smith', 'pitrou', 'vstinner', 'christian.heimes', 'rbcollins', 'rpetrov', 'doughellmann', 'dmalcolm', 'dholth', 'jpokorny', 'bkabrda', 'lukecarrier', 'icordasc', 'cstratak', 'yolanda.robla', 'hroncok', 'cheryl.sabella', 'Andrew DiPrinzio', 'miss-islington']
pr_nums = ['5548', '16044', '19520', '19524', '19703', '20258', '20320']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue9216'
versions = ['Python 3.9', 'Python 3.10']
Activity
davidmalcolm commentedon Jul 10, 2010
(taking the liberty of adding gregory.p.smith to the "nosy" list; hope that's OK)
This is a higher-level take on bpo-9146.
Some versions of OpenSSL have a FIPS mode that can refuse the use of non-certified hashes.
The idea is that FIPS mode should prevent the use of non-certified hashes for security uses. For example, MD5 shouldn't be used for signatures these days (see e.g. http://www.kb.cert.org/vuls/id/836068).
However, there are legitimate non-security uses of these hashes. For example, one might use MD5 hashes of objects to places them in bins for later retrieval, purely as a speed optimization (e.g. files in directories on a filesystem).
I'm working on a patch to hashlib which would better support this, but it involves an API expansion, and I wanted to sound things out first.
The API idea is to introduce a new keyword argument, say "usedforsecurity" to hashlib.new() and to the named hashlib constructors, such as hashlib.md5(). This would default to True. If code is using these hashes in FIPS mode, the developer needs to override this: usedforsecurity=False to mark the callsite as a non-security-sensitive location. Internally, this would lead to the EVP_MD_CTX being initialized with EVP_MD_CTX_FLAG_NON_FIPS_ALLOW.
This way, if you run unaudited code in an environment that cares about FIPS, the code will raise exceptions if it uses a non-valid hash, but during code audit the callsites can be marked clearly as "usedforsecurity=False", and be used as before.
In non-FIPS environments, the flag would be ignored.
Am I right in thinking that the _hashlib module should be treated as an implementation detail here? The entry points within _hashlib are likely to double, with a pair of pre-initialized contexts, one with the flag, one without.
Does this sound reasonable? Thanks.
gpshead commentedon Jul 10, 2010
That sounds fine to me and I do like the usedforsecurity annotation on the API. I'll gladly review any patches.
davidmalcolm commentedon Jul 12, 2010
Attached is a patch against the py3k branch which implements this.
I've checked that it builds against openssl-0.9.8o.tar.gz, openssl-1.0.0a.tar.gz, and against Fedora 12 and 13's heavily-patched openssl-1.0.0. The bulk of my testing has been against Fedora's openssl.
I've added selftests to try to verify the new API. I try to detect if the OpenSSL enforces FIPS, via trying to run "openssl md5" as a subprocess, and seeing if I can trigger an error.
With FIPS enforcement off, all tests pass when built against 0.9.8o and 1.0.0a and F13's 1.0.0, other than those for FIPS enforcement itself, which skip.
With FIPS enforcement on, all tests pass when built against F13's openssl. (I haven't yet figured out how to get the fips selftest to pass for the other builds, it's testing checksums against the wrong libcrypto for some reason; see caveat below):
$ ./python Lib/test/test_hashlib.py
$ OPENSSL_FORCE_FIPS_MODE=1 ./python Lib/test/test_hashlib.py
For all of the various contexts stored in _hashopenssl.c, we now store two: one with the override flag, one without. This required some reworking of the various preprocessor magic in that file, so I've gathered everything related to an algorithm into a structure, and moved most of the logic into functions, rather than macros. I'm assuming that these will get inlined under optimization, and that the bulk of the time that you're trying to optimize out are the EVP lookups and initializations, rather than function call overhead.
How's this looking?
Do I need to add a dummy "usedforsecurity" arg to all of the non-openssl message digest implementations within the tree?
Unfortunately, if fips mode is on, and the fips selftest fails for the openssl library, every hash use will fail, both with and without the flag:
ValueError: error:2D07D06A:FIPS routines:EVP_DigestInit_ex:fips selftest failed
and this leads to a crippled hashlib module. It's not clear to me if there's a good way to handle this. (Having said that, a site that has the technical expertise to opt-in to FIPS mode is hopefully able to diagnose this, and fix their openssl library)
davidmalcolm commentedon Sep 16, 2011
I've refreshed this patch against the latest version of the code in hg.
In an attempt to make it easier to review, I've split it up into four (so far) thematic patches, which apply in sequence.
davidmalcolm commentedon Sep 16, 2011
[and yes, I used git to generate the 4 patches; sorry ]
davidmalcolm commentedon Sep 16, 2011
The cumulative effect of the above patches (to _hashlib) are equivalent to what I've applied downstream to python 2 in RHEL 6.0 and Fedora 17 onwards, and python 3 in Fedora 17 onwards.
In those environments I've additionally patched hashlib to only use _hashlib, rather than falling back on _md5 etc, since otherwise you get confusing error messages from hashlib.md5() when it defers to _md5 due to FIPS enforcement. In my downstream builds we can be sure of building against OpenSSL, but this other part of the patch seems less appropriate for upstream python, given that upstream python tries to be flexible in terms of its dependencies.
Hope this makes sense.
gpshead commentedon Mar 13, 2012
quick summary of comments from pycon sprints discussion:
this looks pretty good. i like the 0001 refactoring cleanup. a couple things to fix in error handling (better messages and some bogus handling in the test). dmalcolm has the notes on what to do.
do it and commit away or ask for more review as you see fit.
pitrou commentedon Mar 14, 2012
Patch 0002:
Patch 0003:
Patch 0004:
Overall:
gpshead commentedon Mar 15, 2012
My summary of our discussion was pretty terse. :) dmalcolm has more detailed TODO list notes that include things like the error cases and .rst documentation.
As for how to commit it, i'd make 0001 its own commit as it is a useful refactoring otherwise unrelated to this change.
I'll leave it entirely up to dmalcolm how many commits he wants 0002 onward to be. No need to be picky.
usedforsecurity vs used_for_security, agreed, used_for_security is better.
dmalcolm was going to make an enum to index the two element array, that'd give meaningful names instead of 0 or 1. simply using two named variables would also work but it would require the loop in 0003 to be expanded or turned into a small static method for the body (not a bad idea) instead. i'm fine with either.
lukecarrier commentedon Jun 29, 2012
I've not done enough digging on the issue I'm presently experiencing to draw any conclusions make any suggestions, but this change seems to break the present distribute module (version 0.6.27). It appears it will likely break a great deal of other code too.
I've pasted the relevant output here and attached the full traceback.
File "/usr/lib64/python3.2/hashlib.py", line 112, in __get_openssl_constructor
f(usedforsecurity=False)
TypeError: openssl_md5() takes no keyword arguments
Whilst I agree with the notion behind this change, Fedora's quick actions have led to me spending the best part of an hour of the night before ship day diagnosing issues caused by undocumented (or at least under-documented) changes to code I haven't written or interfaced with. _Please_ publicise the change a little better? Pretty please!?
pitrou commentedon Jun 29, 2012
This changes haven't been committed in Python, so you probably want to post on the Fedora bug tracker instead.
dholth commentedon Jul 27, 2012
While you are at it, can you edit the docs to put md5() at the bottom of the page at the back of the list in a 2-point font and raise a DeprecationWarning("This function is totally lame, and it is slower than SHA-3, get with the program.") the first time it is used? I don't agree that md5 has a legitimate place in systems designed after 1996.
pitrou commentedon Jul 27, 2012
Please... don't make suggestions unrelated to the issue. Open a new issue instead.
30 remaining items
tiran commentedon May 3, 2020
_hashlib.get_fips_mode() is not compatible with new FIPS design in OpenSSL 3.0.0:
The function calls 'FIPS_mode()' and 'FIPS_mode_set()' are present in OpenSSL 3.0 but always fail. You should rewrite your application to not use them.
https://wiki.openssl.org/index.php/OpenSSL_3.0#Upgrading_from_the_OpenSSL_2.0_FIPS_Object_Module
vstinner commentedon May 4, 2020
I suggest to modify the code so the private function becomes unavailable in _hashlib on OpenSSL 3.0 and newer. What do you think?
tiran commentedon May 20, 2020
Memo to me: Add whatsnew
miss-islington commentedon May 22, 2020
New changeset 909b571 by Christian Heimes in branch 'master':
bpo-9216: hashlib usedforsecurity fixes (GH-20258)
909b571
miss-islington commentedon May 22, 2020
New changeset a08b7c3 by Miss Islington (bot) in branch '3.9':
bpo-9216: hashlib usedforsecurity fixes (GH-20258)
a08b7c3
arhadthedev commentedon Apr 25, 2023
@gpshead Is the issue relevant after transition to HACL*?
gpshead commentedon May 20, 2023
OpenSSL is still the default. regardless, this issue was completed ages ago as far as I can tell.
Let commercial-distro-motivated folks open new specific issues for things they want to see done for anything that remains feature wise here.