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

Fix CVE-2024-33663 #349

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix CVE-2024-33663 #349

wants to merge 1 commit into from

Conversation

danigm
Copy link

@danigm danigm commented May 2, 2024

@milliesolem
Copy link

I recommend throwing an exception if algorithms is None, rather than setting to ALL. Not specifying the algorithms field is the source of algorithm confusion issues.

jose/jwt.py Outdated
@@ -141,6 +141,9 @@ def decode(token, key, algorithms=None, options=None, audience=None, issuer=None

verify_signature = defaults.get("verify_signature", True)

if algorithms is None:
algorithms = ALGORITHMS.ALL
Copy link

@smittysmee smittysmee May 2, 2024

Choose a reason for hiding this comment

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

Why specify? Inline comment & exception would be helpful here. Don't know if there will be downstream impacts:

the algorithms field in jwt.decode is not mandatory, allowing developers to shoot themselves in the foot
inadequate protections in the cryptography backend allowing for HMAC verification with an asymmetric public key

https://github.com/danigm/python-jose/blob/4638c12780636a66b118e699f5b70ce193555106/jose/jws.py#L258C5-L259C65

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request May 6, 2024
https://build.opensuse.org/request/show/1172135
by user dgarcia + anag+factory
- Add upstream patches:
   * CVE-2024-33663.patch, bsc#1223417, gh#mpdavis/python-jose#349
   * CVE-2024-33664.patch, bsc#1223422, gh#mpdavis/python-jose#345
   * fix-tests-ecdsa-019.patch, gh#mpdavis/python-jose#350
@CharlesPerrotMinotHCHB
Copy link

Let's try pinging @asherf and @mpdavis

@smittysmee
Copy link

@mpdavis @asherf
following up on this

@twwildey
Copy link
Collaborator

Can you rebase your changes onto the latest master branch and force-update your branch for this PR?

Would you mind collapsing your commits to a single commit as well?

This change should fix mpdavis#346
security issue.

The code is based on pyjwt change:
jpadilla/pyjwt@9c52867
@danigm
Copy link
Author

danigm commented May 31, 2024

Can you rebase your changes onto the latest master branch and force-update your branch for this PR?

Would you mind collapsing your commits to a single commit as well?

Done

@CharlesPerrotMinotHCHB
Copy link

@twwildey

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.

None yet

5 participants