fix(encryption): read alg/enc exclusively from protected header in JWEDecrypter

[rossaddison]

Summary

JWEDecrypter::decryptRecipientKey() builds $completeHeader by merging the shared protected header, the shared unprotected header, and the per-recipient unprotected header via array_merge(). Because array_merge() exhibits last-wins behaviour for duplicate string keys, an attacker can override the integrity-protected alg or enc parameters by placing different values in an unprotected header field.

This creates a TOCTOU split:

• HeaderCheckerManager validates alg/enc from the protected header
• getKeyEncryptionAlgorithm() / getContentEncryptionAlgorithm() previously resolved the algorithm from the merged header where unprotected values win

Related advisory: #114

What this PR does NOT change

JWSVerifier::getAlgorithm() already reads alg exclusively from getProtectedHeader() and is not affected.

Fix

Pass $jwe->getSharedProtectedHeader() — the integrity-protected portion — to both getKeyEncryptionAlgorithm() and getContentEncryptionAlgorithm() instead of the merged $completeHeader.

The merged $completeHeader is still forwarded to decryptCEK() unchanged, because ECDH-ES parameters (epk, apu, apv) may legitimately reside in per-recipient unprotected headers per RFC 7516 §4.6.

is_string() guards are added in both getter methods so a malformed non-string header value cannot reach the AlgorithmManager.

RFC references

• RFC 7516 §4.1.1 — alg MUST be integrity-protected
• RFC 7516 §4.1.2 — enc MUST be integrity-protected

Testing

Please run the existing JWE test suite. A targeted regression test demonstrating the override scenario would be a welcome addition from the maintainers.

[Spomky]

Thanks for the report and the detailed write-up. The underlying concern is legitimate: an unprotected header field must never be able to override an integrity-protected alg/enc. However, the fix as written can't be merged as-is, because it breaks spec-compliant multi-recipient JWEs.

The blocking issue

Reading alg exclusively from getSharedProtectedHeader() is incorrect for the JSON General Serialization. In a multi-recipient token, the key-management alg legitimately lives in the per-recipient (unprotected) header. That's the whole point of per-recipient headers (RFC 7516 §7.2.1): each recipient may use a different key-management algorithm, and that value cannot be part of the AAD, which only covers the shared protected header. In that layout the shared protected header carries only enc.

This is exactly the RFC 7520 §5.13 vector we already test. Applying this PR makes multipleRecipientEncryptionBis fail:

TypeError: AlgorithmManager::get(): Argument #1 ($algorithm) must be of type string, null given

With the is_string guards it instead throws "The "alg" parameter must be a non-empty string in the protected header", so decryption fails for every recipient.

The §4.1.1 "MUST be integrity-protected" requirement you cite holds for compact / single-recipient, but it does not apply to per-recipient alg in the General Serialization, which the patch overlooks. Side note: the existing test suite catches this, so it's worth running before resubmitting.

Suggested direction

The real flaw is the merge precedence in decryptRecipientKey():

array_merge($sharedProtected, $sharedUnprotected, $recipientHeader) // unprotected wins, that's the bug

A correct fix would:

• enc: resolve from the shared protected header only. It's global, and we already reject per-recipient enc via the "Inconsistent content encryption algorithm" check. This part of your patch is fine.
• alg: resolve from array_merge($recipient->getHeader(), $jwe->getSharedProtectedHeader()) (protected wins), and reject any case where an unprotected header redefines a key already present in the protected header with a different value. That's precisely the invariant JWEBuilder::checkDuplicatedHeaderParameters() enforces on the write path but the decrypter never enforces on read. That's the actual gap to close.

Worth keeping from this PR

• The error-message fix in getContentEncryptionAlgorithm ("key encryption" to "content encryption") is a genuine bugfix.
• The is_string/non-empty guards are good, just apply them on the corrected header source.
• Keeping the merged $completeHeader for decryptCEK() is correct, since ECDH-ES epk/apu/apv may live in the per-recipient header (§4.6).

Could you rework it along those lines and add a regression test that (a) keeps RFC 7520 §5.13 green and (b) asserts that an unprotected alg/enc overriding a protected value is rejected? Happy to help shape the test.

[Spomky]

Please remove these comments in the method.