Skip to content
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

Fixed #28401 -- Allowed hashlib.md5() calls to work with FIPS kernels. #14763

Merged
merged 1 commit into from Oct 12, 2021

Conversation

vakwetu
Copy link
Contributor

@vakwetu vakwetu commented Aug 11, 2021

md5 is not an approved algorithm in FIPS mode, and trying to
instantiate a hashlib.md5() will fail when the system is running in
FIPS mode.

md5 is allowed when in a non-security context. There is a plan to
add a keyword parameter (usedforsecurity) to hashlib.md5() to annotate
whether or not the instance is being used in a security context.

In the case where it is not, the instantiation of md5 will be allowed.
See https://bugs.python.org/issue9216 for more details.

Some downstream python versions already support this parameter. To
support these versions, a new encapsulation of md5() has been added to
django/utils/crypto.py. This encapsulation will pass through the
usedforsecurity parameter in the case where the parameter is supported,
and strip it if it is not.

In django, it appears that md5() is mostly used to generate cache keys
and file/db record names. These are not being used in a security
context and should be allowed. The md5() usages for the password
hashers, on the other hand, should not.

@github-actions
Copy link

Hello @vakwetu! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@cjerdonek
Copy link
Contributor

There is another usage here, FYI:

# This doesn't need to be cryptographically strong, so use what's fastest.
hash_algorithm = 'md5'

Copy link
Contributor

@cjerdonek cjerdonek left a comment

Choose a reason for hiding this comment

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

Some comments.

django/test/runner.py Outdated Show resolved Hide resolved
django/utils/crypto.py Outdated Show resolved Hide resolved
django/utils/crypto.py Outdated Show resolved Hide resolved
django/utils/crypto.py Outdated Show resolved Hide resolved
@vakwetu
Copy link
Contributor Author

vakwetu commented Oct 1, 2021

Hi - what else needs to be done to merge this code?

@vakwetu
Copy link
Contributor Author

vakwetu commented Oct 5, 2021

Hi - what else needs to be done to merge this code?

@kezabelle
Copy link
Contributor

Hi @vakwetu,
I suspect that the ticket itself is in a somewhat gray area. It was originally accepted and then moved to someday/maybe which takes it off some of the report views in Trac, even with a supplied patch.

@felixxm Sorry to ping, but as you touched on the ticket at one point, can you say what's next? Should the ticket move to accepted, or does it need to go to the dev mailing list, or wait until 3.9 is the minimum version, or?

@jacobtylerwalls
Copy link
Member

Hi - what else needs to be done to merge this code?

This was maybe'd on Trac back before the CPython changes landed, but at that time Tim Graham suggested the approach in this PR if those changes were to land. I anticipate this will be an easy triage decision to move to Accepted, but the usual pathway for that is to ask for opinions on the mailing list. Simple as just bumping the old thread, adding a bit of context for the use case, explaining what's changed since then (CPython feature in 3.9), explaining your intended approach, and asking if people support it.

@jacobtylerwalls
Copy link
Member

Thanks for jumping in, Keryn--didn't mean to be having the last word, just didn't see your comment while I was composing mine at the same time!

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@vakwetu Thanks for this patch 👍

django/contrib/auth/hashers.py Outdated Show resolved Hide resolved
django/contrib/auth/hashers.py Outdated Show resolved Hide resolved
django/core/cache/backends/filebased.py Outdated Show resolved Hide resolved
django/utils/cache.py Outdated Show resolved Hide resolved
django/utils/cache.py Outdated Show resolved Hide resolved
django/utils/cache.py Outdated Show resolved Hide resolved
django/utils/crypto.py Outdated Show resolved Hide resolved
django/utils/crypto.py Outdated Show resolved Hide resolved
django/utils/crypto.py Outdated Show resolved Hide resolved
django/utils/crypto.py Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Oct 7, 2021

@felixxm Sorry to ping, but as you touched on the ticket at one point, can you say what's next? Should the ticket move to accepted, or does it need to go to the dev mailing list, or wait until 3.9 is the minimum version, or?

Accepted.

md5 is not an approved algorithm in FIPS mode, and trying to instantiate
a hashlib.md5() will fail when the system is running in FIPS mode.

md5 is allowed when in a non-security context. There is a plan to add a
keyword parameter (usedforsecurity) to hashlib.md5() to annotate whether
or not the instance is being used in a security context.

In the case where it is not, the instantiation of md5 will be allowed.
See https://bugs.python.org/issue9216 for more details.

Some downstream python versions already support this parameter. To
support these versions, a new encapsulation of md5() has been added.
This encapsulation will pass through the usedforsecurity parameter in
the case where the parameter is supported, and strip it if it is not.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
@felixxm felixxm changed the title Fixed #28401 - Allow hashlib.md5() calls to work with FIPS kernels Fixed #28401 -- Allowed hashlib.md5() calls to work with FIPS kernels. Oct 12, 2021
@felixxm
Copy link
Member

felixxm commented Oct 12, 2021

@vakwetu Thanks for updates 👍 Welcome aboard ⛵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants