Skip to content

Check the JWE auth tag before decrypting CBC-HMAC ciphertext - #427

Open
neeraj1235476 wants to merge 2 commits into
mpdavis:masterfrom
neeraj1235476:jwe-verify-tag-before-decrypt
Open

neeraj1235476 wants to merge 2 commits into
mpdavis:masterfrom
neeraj1235476:jwe-verify-tag-before-decrypt

Conversation

@neeraj1235476

Copy link
Copy Markdown

_decrypt_and_auth in jose/jwe.py handles the A128CBC-HS256, A192CBC-HS384 and A256CBC-HS512 encryptions by running the AES-CBC decrypt and PKCS7 unpad first, and only then comparing the tag from the token against the computed HMAC, with a plain !=. A JWE whose ciphertext has been modified therefore comes back as Invalid padding bytes. from the unpadder rather than as a tag rejection, and the two failure modes are distinguishable. RFC 7516 section 5.2 step 15 requires the tag to be validated and the input rejected without emitting any decrypted output, and section 11.5 asks for the comparison to be constant time.

Compute the HMAC tag and check it with hmac.compare_digest before calling decrypt, so forged input never reaches the cipher or the unpadder. GCM is left as is because the AEAD backend already authenticates during decryption. The added test tampers with one ciphertext byte for each CBC-HMAC enc and expects Invalid JWE Auth Tag; it fails on master and passes with this change.

@TheSingulant TheSingulant left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The ordering change looks right to me. It also makes the substituted random CEK path (RFC 7516 section 11.5) fail uniformly with the tag error for CBC-HMAC, instead of usually tripping the unpadder first. One note on the test, and a heads-up that #428 adds the same base64url_encode import and a test at the same spot in tests/test_jwe.py, so whichever lands second will need a small rebase.

Comment thread tests/test_jwe.py Outdated
encrypted = jwe.encrypt(b"Live long and prosper.", key, enc, ALGORITHMS.DIR)
header, encrypted_key, iv, cipher_text, auth_tag = encrypted.split(b".")
tampered = bytearray(base64url_decode(cipher_text))
tampered[-1] ^= 0x01

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Flipping a bit in the last ciphertext block turns that whole plaintext block into noise, and with a fresh random IV each run it occasionally still ends in valid PKCS7 padding. I ran this tamper 3,000 times against master with A128CBC-HS256 and 10 runs came back as Invalid JWE Auth Tag, so the test passes on master about 0.3% of the time. Flipping the last byte of the previous block instead (tampered[-17] ^= 0x01, fine here since the 22-byte plaintext gives two blocks) flips the last padding byte deterministically: 3000/3000 Invalid padding bytes. on master and 3000/3000 Invalid JWE Auth Tag on this branch.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch, I only reasoned about the padding check and missed that the last block gets scrambled wholesale. Switched it to tampered[-17] so only the final padding byte flips. I ran that a thousand times per enc against master and got Invalid padding bytes. every time, and the branch still returns Invalid JWE Auth Tag.

@TheSingulant

Copy link
Copy Markdown

@asherf and maintainers: there have been no merges or releases since 3.5.0 in May 2025, and this PR and #428 both harden JWE decryption. If you have time to come back to it, a review and a release would help the people still relying on python-jose. If not, would you consider adding a co-maintainer, so fixes like these can land?

@neeraj1235476

Copy link
Copy Markdown
Author

Noted on the overlap with #428, the import and the test placement are the same in both. Happy to rebase whichever of the two lands second.

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.

2 participants