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

Indicate that md5 is not being used for secure purposes—and that's okay (FIPS fix) #10192

Merged
merged 1 commit into from Apr 25, 2023

Conversation

nutjob4life
Copy link
Contributor

Merge this to fix #10184 wherein calls to hashlib.md5 would raise an exception on security-restricted systems (such as FIPS mode systems used in newer U.S. Government computers). MD5 is disabled by default on such systems but a flag may be passed to indicate that it's being used for non-secure reasons (such as fingerprinting).

@nutjob4life nutjob4life marked this pull request as draft March 6, 2023 14:37
@squash-labs
Copy link

squash-labs bot commented Mar 6, 2023

Manage this branch in Squash

Test this branch here: https://nutjob4lifemain-ij6k5.squash.io

Comment on lines 42 to 47
try:
# On security-restricted systems, indicate we're just fingerprinting:
hashed = hashlib.md5(email_bytes, usedforsecurity=False).hexdigest()
except TypeError:
# On older Pythons, we can't make such an indication:
hashed = hashlib.md5(email_bytes).hexdigest()
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this to a shared utility function, e.g.

wagtail.coreutils.safe_md5

def safe_md5(data=b"", usedforsecurity=True):
    try:
        return hashlib.md5(data, usedforsecurity=usedforsecurity)
    except TypeError:
        return hashlib.md5(data)

and in here (and wagtail.embeds.embeds.get_embed_hash) we do something like

Suggested change
try:
# On security-restricted systems, indicate we're just fingerprinting:
hashed = hashlib.md5(email_bytes, usedforsecurity=False).hexdigest()
except TypeError:
# On older Pythons, we can't make such an indication:
hashed = hashlib.md5(email_bytes).hexdigest()
hashed = safe_md5(email_bytes, usedforsecurity=False).hexdigest()

We also have an accepts_kwarg function in coreutils that can be used in place of the try/except.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea @laymonage. I've pushed changes to refactor the use of md5 into a common safe_md5 utility function.

Also made use of accepts_kwargs—even if that does feel a bit more like asking permission than forgiveness 😅

@laymonage laymonage self-assigned this Apr 12, 2023
@gasman gasman marked this pull request as ready for review April 24, 2023 13:59
@gasman
Copy link
Collaborator

gasman commented Apr 24, 2023

Thanks @nutjob4life! Merged in 4dea702.

@gasman gasman closed this Apr 24, 2023
gasman added a commit that referenced this pull request Apr 24, 2023
@gasman gasman reopened this Apr 24, 2023
@gasman
Copy link
Collaborator

gasman commented Apr 24, 2023

Reverted in f6781a2 (CI was failing against Python 3.7)

@nutjob4life
Copy link
Contributor Author

This is curious:

$ docker container run --rm python:3.11 python -c 'import inspect, hashlib; signature = inspect.signature(hashlib.md5)'
$ docker container run --rm python:3.10 python -c 'import inspect, hashlib; signature = inspect.signature(hashlib.md5)'
$ docker container run --rm python:3.9 python -c 'import inspect, hashlib; signature = inspect.signature(hashlib.md5)'
$ docker container run --rm python:3.8 python -c 'import inspect, hashlib; signature = inspect.signature(hashlib.md5)'
$ docker container run --rm python:3.7 python -c 'import inspect, hashlib; signature = inspect.signature(hashlib.md5)'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/lib/python3.7/inspect.py", line 3083, in signature
    return Signature.from_callable(obj, follow_wrapped=follow_wrapped)
  File "/usr/local/lib/python3.7/inspect.py", line 2833, in from_callable
    follow_wrapper_chains=follow_wrapped)
  File "/usr/local/lib/python3.7/inspect.py", line 2288, in _signature_from_callable
    skip_bound_arg=skip_bound_arg)
  File "/usr/local/lib/python3.7/inspect.py", line 2112, in _signature_from_builtin
    raise ValueError("no signature found for builtin {!r}".format(func))
ValueError: no signature found for builtin <built-in function openssl_md5>
$ echo \U+1F62E
😮

Looks like using try/except is the right way to go after all.

@laymonage laymonage merged commit d68baeb into wagtail:main Apr 25, 2023
15 of 16 checks passed
@laymonage
Copy link
Member

It seems Python 3.7 doesn't include the signature object for md5 to be used with inspect.signature.

Sorry this fell off my radar before the 5.0 feature freeze, so it looks like this'll be in 5.1. Thanks @nutjob4life!

@laymonage laymonage added Compatibility For issues that explicitly relate to compatibility with dependencies and other packages and removed status:Needs Work labels Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compatibility For issues that explicitly relate to compatibility with dependencies and other packages
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Admin cannot be accessed on FIPS-mode system
3 participants