-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
gh-127298: When in FIPS mode ensure builtin hashes check for usedforsecurity=False #127301
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
base: main
Are you sure you want to change the base?
Conversation
FYI @stratakis |
82e72d9
to
9d6fd91
Compare
This comment was marked as resolved.
This comment was marked as resolved.
db5c976
to
ad0fab8
Compare
ad0fab8
to
28d8035
Compare
The built-in algorithms are based on HACL* and are therefore formally verified implementations. So I'm not entirely sure we can say that they are entirely FIPS unapproved (except that some algorithms, whatever their imementation is, might not be allowed on FIPS-only systems). So, I would like to confirm with @msprotz whether their implementations of SHA and blake2 are FIPS-friendly or not (md5 is never FIPS-friendly I think?) |
If the binary build was not tested by a NIST accredited lab, and there is no CMVP certificate issued, it is not certified. HACL* formally verified implementation or not. As in search on https://csrc.nist.gov/projects/cryptographic-module-validation-program/validated-modules/search should bring up an active certificate, for the binary build (not source-code) on a given supported operating environment (architecture & OS type).
It is irrelevant what is or isn't FIPS-friendly. In approved only mode, only certified implementation can be used, which has a CMVP certificate. Meaning you cannot even use SHA2-256 HACL implementation, unless it was NIST certified. Despite it producing the same hexdigits for the same inputs =) Hence here builtin implementations are only made available at runtime with a so called service indicator (ISO/FIPS term) when usedforsecurity=False is explicitly specified. One cannot universally know what is or isn't allowed, as it depends on which module one is using at runtime, and what its security policy stated. There is no universal statement for FIPS as active modules last for 5 years, and historical modules continue to be in use, despite submission requirements changing. Meaning an old 1.1.1 openssl fips module on a supported system can be missing shake/sha3, and allow md5. More recent 140-2 certificates may still allow MD5 as part of TLS MD5-SHA1 and HMAC, but not digest. Upcoming modules likely to not have MD5 at all (neither for TLS nor HMAC). W.r.t. blake2 it is NIST disallowed for cryptographic protection of sensitive information (it lost to KECCAK in sha3 competition). Note it is extremely useful to still have access to blake2 and md5 as "one-way non-cryptographic compression" functions. For example md5 will continue to be used to identify public cloud buckets; views in SQL databases; all US court submitted documents and so on. Thus access for non-security purposes is very relevant for users. Example of blake2 usage for non-security purposes as a tuplehash https://github.com/aiidateam/aiida-core/blob/f74adb94cc1e8439c8076f563ec112466fdd174b/src/aiida/common/hashing.py#L78 which is not security relevant Please also note diagrams & further commentry at https://discuss.python.org/t/python-and-openssl-fips-mode/51389/12 The goal of this patch to ensure that no distro modifications are required of Python when operating on a system with OpenSSL in FIPS mode. This pull request will ensure that one can download python.org distributed or self-built cpython builds; and they just work correctly as expected by FIPS users; and matches behavior of all other open source interpreters. By default, they only offer approved & CMVP certified implementations. It is very consent based. |
That's what I meant by FIPS-friendly. I wanted to confirm with the author whether this PR is needed or not. The reason is that you claimed that "It is likely that buitin implementations are FIPS unapproved" which I think should be verified by asking the relevant people first. |
@gpshead I have access to some 1.x based FIPS modules (Ubuntu & Azure Linux) and I think this will work correctly as is there too at runtime. The unit test mock is 3+ specific, but runtime implementation is 1.x compatible. As _hashlib.get_fips_mode is implemented for both 1.x & 3 ABI. Hence news entry should mention OpenSSL, but not the providers and/or major openssl version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed some changes based on what I saw in my review, there was some pedantic correctness stuff like get_fips_mode raising ValueError or returning values other than 0 or 1 to be handled. I moved the repeated logic into a single execution in the global scope instead of upon every constructor registration function call.
I also updated comment and news wording (good point about this not being 3 specific, I saw that in the code after my first news revision; fixed).
I'd like it if we got rid of OpenSSL hashlib entirely in the long run, but that isn't likely for 3.14 and some vendors needing this bureaucratic oddity is a reason to keep that around anyways so long as it isn't a major burden.
I sent this to the buildbots out of curiosity to see if any have environments that trip the new hashlib_fips test up. But I think that is well enough written with the right conditionals that it'll properly skip as needed.
The buildbots are turning up some interesting failures. is the setUpClass assertion I added about _hashlib and _ssl not having been loaded accurate? It is possible to execute the test suite such that each test is not getting its own new process, which might be the source of that. |
I see what is happening with Blake tests on a RHEL in fips mode. I think we should adjust all Blake tests to always pass in Alternatively we can wrap OpenSSL C APIs to force or lift "fips=yes" property query string. Btw the fact that Blake is accessible when host OS is in FIPS is one of the motivators for these patch series. |
@gpshead hi, my PRs had comments that you are taking over with these changes instead, but they now seem to have stalled. Are you still actively working on this and related PRs? |
- fixes chainguard-dev/internal-dev/wolfi-dev#11886 - backports from wolfi-dev#36503 (see python/cpython#127301) Signed-off-by: Dan Ryan <[email protected]>
- fixes chainguard-dev/internal-dev#11886 - backports from wolfi-dev#36503 (see python/cpython#127301) Signed-off-by: Dan Ryan <[email protected]>
- fixes chainguard-dev/internal-dev#11886 - backports from wolfi-dev#36503 (see python/cpython#127301) Signed-off-by: Dan Ryan <[email protected]>
…edforsecurity=False When _hashlib/OpenSSL is available, and OpenSSL is in FIPS mode, ensure that builtin (fallback) hash implementations are wrapped with a check for usedforsecurity=False. It is likely that buitin implementations are FIPS unapproved (either algorithm disallowed; or the implementation not certified by NIST). This enables strict approved-only compliance when usedforsecurity=True on FIPS systems only. And yet it also enables fallback access with usedforsecurity=False for any missing (historical, disallowed or missing certified implementation) algorithms (i.e. blake2, md5, shake/sha3) depending on the runtime configuration of OpenSSL.
FIPS mode is an OpenSSL feature and we don't require OpenSSL. So anyone wanting to rely on this will need to ensure their build includes Modules/_hashopenssl.c as `_hashlib` linked appropriately.
905ba7f
to
fd308b4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
🤖 New build scheduled with the buildbot fleet by @AA-Turner for commit fd308b4 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F127301%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
@AA-Turner thank you! I will work on fixing those two! |
|
When _hashlib/OpenSSL is available, and OpenSSL is in FIPS mode, ensure that builtin (fallback) hash implementations are wrapped with a check for usedforsecurity=False. It is likely that buitin implementations are FIPS unapproved (either algorithm disallowed; or the implementation not certified by NIST).
This enables strict approved-only compliance when usedforsecurity=True on FIPS systems only.
And yet it also enables fallback access with usedforsecurity=False for any missing (historical, disallowed or missing certified implementation) algorithms (i.e. blake2, md5, shake/sha3) depending on the runtime configuration of OpenSSL.