Skip to content

gh-103256: Fix hmac algorithm to support fallback implementation #103286

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

Merged
merged 3 commits into from
Apr 7, 2023

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Apr 5, 2023

@corona10
Copy link
Member Author

corona10 commented Apr 6, 2023

@gpshead

Would you like to take a look at this PR?
The change will not affect to backward compatibility.

@gpshead gpshead added needs backport to 3.11 only security fixes and removed skip news labels Apr 7, 2023
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

Thanks! somewhat surprised we hadn't run into this at work, but I guess there isn't much demand for hmac-sha3 yet. the other folks who might hit this probably have openssl builds without md5 or sha1?

@corona10
Copy link
Member Author

corona10 commented Apr 7, 2023

I guess there isn't much demand for hmac-sha3 yet. the other folks who might hit this probably have openssl builds without md5 or sha1?

It looks so, and the latter looks minor case in this era :)

@corona10
Copy link
Member Author

corona10 commented Apr 7, 2023

Thanks @gpshead for reviewing and supplementing this PR!

@corona10 corona10 merged commit efb0a2c into python:main Apr 7, 2023
@miss-islington
Copy link
Contributor

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-103328 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 7, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 7, 2023
pythongh-103286)

(cherry picked from commit efb0a2c)

Co-authored-by: Dong-hee Na <donghee.na@python.org>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington added a commit that referenced this pull request Apr 7, 2023
…103286)

(cherry picked from commit efb0a2c)

Co-authored-by: Dong-hee Na <donghee.na@python.org>
Co-authored-by: Gregory P. Smith <greg@krypto.org>
@geitda
Copy link

geitda commented Apr 7, 2023

Thanks! somewhat surprised we hadn't run into this at work, but I guess there isn't much demand for hmac-sha3 yet. the other folks who might hit this probably have openssl builds without md5 or sha1?

SHA3 is fine since OpenSSL (at least in Python 3.11.2) provides it;

Python 3.11.2 (tags/v3.11.2:878ead1, Feb  7 2023, 16:38:35) [MSC v.1934 64 bit (AMD64)] on win32
>>> import hmac
>>> hmac.digest(b'key', b'message', 'sha3_256').hex()
'0f43852a24d5597a8200312a95993991581679d63264f1b1ad4b5ccac7fe8ba4'

The only hash algorithms that were a problem are ones that OpenSSL doesn't provide via named lookup. So none of the built-in algorithms would trigger it as OpenSSL provides all of them (even whirlpool, mdc2, and the composite md5-sha1 work as expected) - that's why it took me (ab)using hashlib to find it. But patching hashlib (directly in the file or on the fly) to include a new algorithm does trigger it (as that's what I was doing). But you're right that if md5 or sha1 were removed from OpenSSL while still being kept in hashlib (e.g. with a stand-alone CPython implementation), then the problem would get triggered.

@gpshead
Copy link
Member

gpshead commented Apr 7, 2023

Not everyone's OpenSSL provides it. 1.1.1 and BoringSSL do not.

warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@corona10 corona10 deleted the gh-103256 branch April 24, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants