Author kscguru
Recipients christian.heimes, dmalcolm, dstufft, gregory.p.smith, jpokorny, kscguru, pitrou
Date 2017-03-30.01:26:00
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1490837162.72.0.8859133816.issue9146@psf.upfronthosting.co.za>
In-reply-to
Content
I tripped over this exact issue a few months ago, while working on a FIPSified OpenSSL library (custom platform).

Attached a different (more minimal) patch; this one focuses more narrowly on the exact failure mode. It's based on 'master' (~3.7), applies cleanly / tests_hashlib passes. My employer has been using this patch successfully (in a FIPS environment) for ~3 months now, and I'm looking to upstream.

Earlier, dmalcolm identified that OpenSSL's EVP_DigestUpdate dereferences a NULL in FIPS mode on non-FIPS algorithms. OpenSSL is not quite that bad ... turns out, EVP_DigestInit returns an error if FIPS disallows an algorithm, and ignoring the error (which _hashopenssl.c currently does) results in later dereferencing NULL. It's straightforward to add the right error checks and convert them to Python exceptions, which seems the most Pythonic way to handle the error - and it also minimizes the FIPS-only codepaths.

There's one catch, _hashopenssl.c likes to pre-construct several algorithms at module import and raising an exception during import leads hashlib.py to silently drop the exception (hashlib.py:158-161). So I made the pre-constructors lazy: the first use of any constructor will attempt to initialize it, and raise the exception on first use if FIPS mode makes it unusable. The reason for choosing this approach is Lib/hashlib.py and __get_openssl_constructor(), which already has logic to handle exactly this ValueError by falling back to __get_builtin_constructor() (and the less-optimized _md5/_sha1/_sha256 modules).

In the context of issue9216 (a usedforsecurity flag which can be passed to OpenSSL to allow non-FIPS algorithms), this change has the net effect of silently falling back from OpenSSL's MD5 implementation to python's _md5 C code when in FIPS mode. That's not exactly the intent of FIPS mode (python's _md5 module is certainly not FIPS-compliant), but converting a crash to a catchable exception is a major improvement. I like the discussion in issue9216, and see this change as complementary: it avoids crashing and moves policy handling of how to handle FIPS mode _hashopenssl.c to hashlib.py, and my gut feeling is that policy logic is better placed in a .py library than in a .c library.

The advantage of this patch is that the new exceptions are obviously correct regardless of FIPS considerations, and the lazy constructors maintain the spirit of amortizing EVP_DigestInit calls while also reporting any error at a more useful point.
History
Date User Action Args
2017-03-30 01:26:02kscgurusetrecipients: + kscguru, gregory.p.smith, pitrou, christian.heimes, dmalcolm, jpokorny, dstufft
2017-03-30 01:26:02kscgurusetmessageid: <1490837162.72.0.8859133816.issue9146@psf.upfronthosting.co.za>
2017-03-30 01:26:02kscgurulinkissue9146 messages
2017-03-30 01:26:01kscgurucreate