-
Notifications
You must be signed in to change notification settings - Fork 240
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 for CVE-2024-33664. JWE limited to 250K #352
base: master
Are you sure you want to change the base?
Conversation
Is this repository still maintained? Would be great to check and merge this PR. |
Thank you for this work @alistairwatts. Would love to see this PR go in. |
jose/jwe.py
Outdated
# data could lead to large memory usage. This helps address This addresses | ||
# CVE-2024-33664. Also see _decompress() | ||
if len(jwe_str) > JWE_SIZE_LIMIT: | ||
raise JWEError("JWE string exceeds {JWE_SIZE_LIMIT} bytes") |
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.
This should be an f-string.
if @mpdavis does not work maybe @michaeldavis-wf will? |
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 fixed the missing f-string. @alistairwatts
Can you rebase your changes onto the latest |
https://build.opensuse.org/request/show/1178245 by user dgarcia + anag+factory - Update CVE-2024-33664.patch with upstream mpdavis/python-jose#352 bsc#1223422
Any updates here? |
Right now we should be checking the length of the tokens at the API level whilst waiting for this fix? Dependabot brought me here. |
b2e97ab
to
f7e0759
Compare
f7e0759
to
b91c69c
Compare
# data could lead to large memory usage. This helps address This addresses | ||
# CVE-2024-33664. Also see _decompress() | ||
if len(jwe_str) > JWE_SIZE_LIMIT: | ||
raise JWEError(f"JWE string exceeds {JWE_SIZE_LIMIT} bytes") |
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.
it would be useful to also include the token size, more information is usually useful.
import jose.constants | ||
old_limit = jose.constants.JWE_SIZE_LIMIT | ||
try: | ||
jose.constants.JWE_SIZE_LIMIT = 1024 |
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.
it is better to use a a monkery patch here here: https://docs.pytest.org/en/stable/reference/reference.html#pytest.MonkeyPatch.setattr
reducing the need to to things manually.
encrypted = jwe.encrypt(b"Text"*64*1024, PUBLIC_KEY_PEM, enc, alg, zip=ZIPS.DEF) | ||
assert len(encrypted) < jose.constants.JWE_SIZE_LIMIT | ||
header = json.loads(base64url_decode(encrypted.split(b".")[0])) | ||
with pytest.raises(JWEError): |
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.
since JWError is kind of generic, it is worth asserting on the error message:
https://docs.pytest.org/en/7.1.x/how-to/assert.html#assertions-about-expected-exceptions
This fix for CVE-2024-33664 ensures that any incoming JWE is under 250K, which seems to be a sensible, albeit large limit. The specific fix for the "zip bomb" issue ensures that we decompress no more that 250K of data. If that limit is reached then a JWEError is raised.
There's rough symmetry here ensuring that both compressed and uncompressed JWE data is no more than 250K.