Add the PBES2-HS256+A128KW, PBES2-HS384+A192KW and PBES2-HS512+A256KW key encryption algorithms (RFC 7518 section 4.8)

[zandbelt]

Summary

Adds the PBES2 key encryption algorithms PBES2-HS256+A128KW, PBES2-HS384+A192KW and PBES2-HS512+A256KW of RFC 7518 section 4.8. A key encryption key of the size the alg names is derived from the password, the octets of the oct key, with PBKDF2 over HMAC SHA-256, SHA-384 or SHA-512 (PKCS5_PBKDF2_HMAC); the salt is the algorithm name, a zero octet and the p2s header parameter, the iteration count the p2c header parameter. The CEK is then wrapped with AES Key Wrap through the existing helpers. With #188, cjose covers every algorithm of RFC 7518 (refs OpenIDC#28).

• Parameters. The salt input is generated for every encryption, 16 octets from the RNG, as RFC 7518 section 4.8.1.1 requires, and a caller-supplied p2s is refused rather than quietly dropped, before any key derivation runs. This is the treatment iv, tag and epk already get, through the same _cjose_jwe_reject_generated_param. The iteration count is a policy choice rather than a value that has to be fresh, so a caller-supplied p2c is used as it is and the count can be chosen per JWE, and per recipient, without new API. Both parameters are published like iv and tag: the protected header for a single recipient, the recipient's own header with several, and on decrypt they are read from the recipient, shared and protected headers in that order.
• Policy, the part worth a look. Of the bounds only the 8 octet minimum salt input is an RFC requirement. The rest is cjose policy, documented as such in jwe_int.h and in the README, and each of the four values can be overridden at build time: at least 1000 iterations (the RFC's recommendation, refused below rather than silently accepted), at most 1000000, at most 1024 salt octets, and 8192 iterations by default. _Static_asserts keep an overridden set consistent and inside what OpenSSL takes as an int, rather than letting a misconfiguration fail every encryption at run time. On decrypt p2c is attacker-controlled, so the bounds and the encrypted key length (CEK plus 8) are checked, and the encoded length of p2s is checked before it is decoded, all before any PBKDF2 work: a JWE can demand at most a million HMAC iterations per recipient from cjose_jwe_decrypt. The ceiling has to stay at or above 100000, the highest default a mainstream producer ships. The encrypt default of 8192 is an interoperability choice: several widely used implementations refuse tokens above roughly 10000 to 16384 iterations by default, so a higher cjose default would produce JWEs they cannot read.
• API. Macro-only constants CJOSE_HDR_ALG_PBES2_HS256_A128KW, CJOSE_HDR_ALG_PBES2_HS384_A192KW, CJOSE_HDR_ALG_PBES2_HS512_A256KW, CJOSE_HDR_P2S and CJOSE_HDR_P2C. A README row and a note on the parameters.
• Tests. Round trips over the three algorithms and both content encryption families at 1000 iterations plus one at the default, checking the published parameters; python-jwcrypto vectors (three compact serializations, one with 16384 iterations, a JSON serialization with the parameters in the recipient header); the RFC 7517 Appendix C example, which must decrypt to the C.1 RSA private JWK (length, SHA-256, key type and kid checked); a two-recipient JSON round trip with a different count and a different salt per recipient; negative cases for a non-oct key, the wrong password, an iteration count above the maximum, below the minimum or not an integer, a missing or short salt on decrypt, a salt at and beyond the maximum, an encrypted key of the wrong length, and a caller-supplied salt input in each of the three header locations, including the shape the RFC requirement is about: feeding the protected header of a produced JWE into the next encryption.

Testing

• -pedantic -Wall -Werror build and the full check_cjose run with CJOSE_ENABLE_RSA1_5 OFF and ON, and once more with the four PBES2 constants overridden, to check that the suite follows them.
• valgrind over the whole suite: no leaks, no errors.
• The clang-format target produces no diff.

🤖 Generated with Claude Code

[zandbelt]

@TheStormN rebased onto master (3c153f7) and ready for review.

Two things the rebase had to settle, both from the crit work:

• this PR added p2s and p2c to the set of accepted critical parameters, and master has no such set any more, so the addition is dropped and a crit list naming them is refused like any other;
• the key type check in _cjose_jwe_validate_decrypt_key() keeps master's _cjose_jwe_alg_is_aes_gcm_kw() helper, with the three PBES2 algorithms alongside it.

It is also one commit shorter: it was stacked on #184, whose commits are in master now, so what is left is the PBES2 work alone.

One interaction I checked rather than assumed, since master now refuses a header parameter that the algorithm produces itself: p2s and p2c are not that case. PBES2 uses a caller-supplied salt or iteration count when there is one and only generates them otherwise, so there is no silent replacement and no duplicate name, and the assembled header passes the new disjointness check. Verified by hand: single recipient, a shared unprotected header alongside, two recipients with the parameters in the recipient's own header, and a caller-pinned p2c all encrypt, re-import and decrypt.

Full check_cjose with RSA1_5 off and on, valgrind clean, clang-format no diff.

[TheStormN]

Btw @zandbelt from now on could you please run a separate agent to review your PRs to save some effort.

Findings:

1. P1 — Valid shared alg headers cannot be encrypted
   At src/jwe.c:2283, _cjose_jwe_validate_alg() receives the recipient header as its “shared” header argument instead of shared_unprotected_header. Therefore a valid JWE configuration with alg in the shared unprotected header is rejected before AES-GCM-KW runs. RFC 7516 explicitly permits alg to be shared across recipients. RFC 7516 §7.2.1
   Pass shared_unprotected_header there.
2. P1 — Encryption can emit duplicate iv/tag header parameters
   At src/jwe.c:932-937, generated iv and tag are written to the protected header for a single recipient, or the recipient header for multiple recipients, without rejecting caller-supplied values in the other header locations. This can produce duplicate names across protected/shared/per-recipient headers, and can make the resulting JWE fail to decrypt even with the correct key. RFC 7516 requires these locations to be disjoint. RFC 7516 §7.2.1
   Reject caller-supplied iv/tag, or validate the assembled headers after key wrapping.
3. P2 — iv and tag are accepted as critical parameters for unrelated algorithms
   At src/jwe.c:416, iv and tag are added to one global supported-critical-header list. Consequently, an A128KW or RSA JWE can mark iv critical and cjose will accept and ignore it. Critical parameters must be understood and processed; crit is intended for extensions, not globally accepted standard parameters. RFC 7515 §4.1.11
   Scope critical-parameter handling by algorithm, or reject crit since cjose does not implement extensions.

[zandbelt]

Happy to run a review agent over these before submitting — I have started doing that, and the rest of this comment is the result.

First, though, I think this review was made against the state of the PR before it was rebased. All three findings were real in the version that was stacked on #184, and all three are in master now, two of them written in answer to your earlier reviews on #184 and #188. Against the current head, 05d344a:

finding	at 05d344a
jwe.c:2283 passes the recipient header where the shared one belongs	line 2622 passes shared_unprotected_header; line 2283 is return false;
jwe.c:932-937 writes iv and tag without refusing caller-supplied ones	_cjose_jwe_reject_generated_param() is used five times, and the assembled headers are validated again after every encrypt_ek; line 932 is a key length check
jwe.c:416 puts iv and tag in a global critical-header list	supported_crit_headers does not occur in the file at all — crit is refused outright since #188; line 416 is an enc comparison

So there is nothing here for me to change. Worth pinning a re-review to the head commit rather than the PR as a whole, which is what I take from this: an agent reviewing a rebased PR needs to be pointed at the current head.

On my side I ran one over the three open PRs against master 3c153f7, and it earned its keep on the other two rather than on this one:

• Refuse an RSA key whose private members are present but carry no value #189 — it found that my own fix was incomplete. Base64url padding on its own decodes to zero octets with a non-NULL buffer, so {"kty":"RSA",...,"d":"=="} walked past a check that only tested the pointer and the key still imported as public. Fixed in e189ce8, with "==" and "====" added to the test.
• Add X25519 and X448 to the ECDH-ES key agreement (RFC 8037 section 3.2) #186 — nothing wrong in what it adds, after a run of hostile epk shapes. It did surface a crash next door: cjose_jwe_decrypt(jwe, jwk, &len, NULL) on any ECDH-ES+A*KW JWE segfaults on master, because _cjose_jwe_decrypt_ek_ecdh_es_kw() writes through err in its first statement while the plain ECDH-ES sibling was given a fallback and the key wrapping variant was missed. That is Survive a NULL cjose_err when decrypting with ECDH-ES and a key wrapping algorithm #191, separate from Add X25519 and X448 to the ECDH-ES key agreement (RFC 8037 section 3.2) #186 since it is a bug in released code.
• Add the PBES2-HS256+A128KW, PBES2-HS384+A192KW and PBES2-HS512+A256KW key encryption algorithms (RFC 7518 section 4.8) #185, this PR — no defect found. It checked the encrypt-emits-import-accepts invariant across all 24 combinations of one or two recipients, p2s/p2c in each of the three header locations, and each of them supplied or not; hostile p2c and p2s values on decrypt, including INT64_MAX, a JSON real and a 700 KB salt; the bounds before every OpenSSL call; and that the RFC 7517 Appendix C vector still decrypts.

It did raise one hardening point I would rather fix than leave: there is a minimum on the decoded p2s but no maximum, and salt_len is passed to PKCS5_PBKDF2_HMAC as an int, so a multi-gigabyte salt would convert to a negative value that OpenSSL widens back. It needs a JWE of a size nobody will send, but every other attacker-controlled length in this file is bounded before the call, and this one is one clause. Say the word and I will add it here, or leave it if you would rather keep the PR as reviewed.

[TheStormN]

Thanks! I did synced up before running the review again, but I will start with fresh context and check again.

Regarding the hardening point - sure lets have it fixed in this PR as it have to me synced again with master anyway.

[zandbelt]

Synced to master (40a5767) and the hardening is in, as d9a4179.

The salt bound. CJOSE_JWE_PBES2_MAX_SALT_LEN, 1024 octets, enforced on the encrypt path, on the decrypt path, and inside _cjose_jwe_pbes2_derive_kek() before it allocates or copies anything. Alongside it the four lengths PKCS5_PBKDF2_HMAC() takes as int are all guarded, so none of them can convert to a negative value however the caps are built. Both sides of the bound are tested now: 1024 octets is accepted and round-trips, 1025 is refused, on encryption and on import.

The rebase itself was clean — one conflict, and an additive one: the OKP and PBES2 test registrations landed on the same line.

While in there I also took the other points from the review of this PR: a caller-supplied p2s is now asserted to be published verbatim and to decrypt, the two wrong octet counts in the test comments are gone ("AAAAAAA" is 5 octets, not 7, and the old long-salt string was 1028, not 1025), the bound constants moved to jwe_int.h so the tests assert against the real values rather than copies, and CJOSE_HDR_P2S now carries a warning that each recipient needs its own salt input and iteration count, which RFC 7518 section 4.8.1.1 requires and RFC 7520 section 5.3 spells out for the multi-recipient case.

One thing I want your decision on rather than my own. At the current CJOSE_JWE_PBES2_MAX_ITERATIONS of 1000000, an unauthenticated JWE of about 300 bytes costs a recipient roughly 900 ms of CPU per decrypt attempt with PBES2-HS384+A192KW or PBES2-HS512+A256KW, measured on a current desktop core; at 100000 it is about 90 ms.

I tried lowering it and put it back, because the same constant bounds encryption: at 100000 a caller could no longer produce a JWE at the iteration counts current password hashing guidance asks for (OWASP is at 210000 for HMAC-SHA-512 and 600000 for SHA-256, and NIST SP 800-132 section 5.2 contemplates far higher). Splitting the two would let encryption exceed what decryption accepts, and then cjose could emit a JWE it refuses to read back, which is the invariant #188 established. So the options are a lower symmetric cap, the current one, or a deliberate split, and the choice is a policy one. Happy to implement whichever you prefer.

For context on the numbers, none of this is close to the caps in practice: go-jose defaults to exactly 100000, which the test now pins as accepted since the check is > MAX; jose4j, jwcrypto, node-jose and jose2go default to 8192; latchset/jose to 32768 and refuses more than that on decrypt; panva/jose refuses more than 10000 on decrypt, four times stricter than cjose would be at 100000. The salt input is 16 octets in every vector I decoded, including RFC 7517 Appendix C and RFC 7520 section 5.3, so the 1024 cap is 64 times the universal value.

Reviewed by an independent agent before each push, per your request. It is what found the wrong octet counts, the one-sided iteration test, and the encrypt-side argument that made me revert the cap.

[TheStormN]

Ok, ran again with fresh context from fresh master. For 2 while there is no recommended maximum as you've already mentioned, it seems to be stricter than required.

Findings:

1. High — caller-supplied p2s can violate RFC 7518’s mandatory fresh-random-salt requirement.
   src/jwe.c:1224 reuses an existing p2s. Reusing the same header across encryptions therefore reuses the PBKDF2 salt/derived KEK, contrary to RFC 7518 §4.8.1.1. The encrypt path should generate a fresh salt and reject caller-supplied p2s, or explicitly redesign this as caller-provided random entropy.
   The same path preserves the supplied encoding verbatim, so padded values such as ...= can also produce non-JOSE base64url output; RFC 7515 §2 requires padding to be omitted.
2. Medium — parameter bounds are stricter than RFC requirements.
   The implementation rejects p2c below 1000 and above 1,000,000, and p2s above 1024 bytes (jwe_int.h:81). RFC 7518 requires a positive p2c and only recommends 1000; it specifies no maximum for either value (§4.8.1.1, §4.8.1.2). These may be reasonable DoS-policy limits, but they should be documented as implementation limits rather than presented as RFC validation.
3. Low — the salt-size limit is enforced after decoding.
   src/jwe.c:1344 base64url-decodes attacker-controlled p2s before checking the 1024-byte limit, allowing an unnecessarily large temporary allocation. Check the encoded length before decoding.

[zandbelt]

Thanks, all three are addressed in 350a962, and I did run a separate agent over the change before pushing it.

1. Fresh salt. You are right, and it is a MUST: RFC 7518 section 4.8.1.1 says a new salt input must be generated randomly for every encryption operation, and reusing a header object was enough to break it. The salt is now always generated, and a caller-supplied p2s is refused through the existing _cjose_jwe_reject_generated_param, the same treatment iv, tag and epk already get from #188. A caller-supplied p2c is still honoured: an iteration count is a policy value with no freshness requirement, and it is the only way to choose the count without new API. To be precise about what does what, the freshness comes from always generating; the refusal is there so that caller input is not quietly discarded, and so that it fails before any key derivation runs. That also disposes of the padding half of the finding, since the encrypt path no longer echoes a caller's encoding.

What is lost is the ability to reproduce a known-answer vector with a fixed salt through cjose's own encrypt API. Verifying such vectors is unaffected, and the RFC 7517 Appendix C test still covers that direction.

The regression test is the shape you described: encrypt, feed the resulting protected header into the next encryption, and it is now refused where it used to hand back the same salt and the same KEK. Both it and a plain well-formed p2s fail on the previous commit; a caller-supplied p2s is also refused in the shared and the per-recipient header.

2. Bounds. Fair point, they were presented as validation and they are policy. jwe_int.h now says per bound what is an RFC requirement (only the 8 octet salt input) and what is cjose, the README carries the same in user-facing terms, and all four values are overridable at build time rather than the two that were. _Static_asserts reject an inconsistent set of overrides at compile time, including one that exceeds what PKCS5_PBKDF2_HMAC takes as an int, instead of letting it fail at run time on every encryption. I kept the values themselves: the 1000 floor refuses a weak count rather than silently accepting it, and lowering it is now a documented one-liner for anyone who needs to read such JWEs.

Two things the documentation now also says, which were true before but unwritten: the iteration ceiling bounds the work per recipient, since cjose_jwe_decrypt_multi derives a key for every recipient the locator matches and p2c sits in an unprotected header there; and the 16 octet generated salt is policy but deliberately not a knob.

3. Decode before check. Done, the encoded length is checked first. The bound is the padded length, 4 * ceil(max / 3), rather than the unpadded one: cjose's base64url decoder tolerates padding everywhere, so a tighter bound here would have made this one parameter stricter than the rest of the parsing, and it would also have made the post-decode check unreachable. As it stands the pre-check bounds the allocation and the decoded length is still what decides.

Verified with -pedantic -Wall -Werror and the full suite with CJOSE_ENABLE_RSA1_5 OFF and ON, once more with the four constants overridden so the suite follows them, valgrind over the whole suite with no leaks or errors, and no clang-format diff.

[zandbelt]

The review you left on #193 is about this code, so I have answered it here, in 393e25b.

p2c rejects RFC-valid values. You are right, and this is the point I got wrong in the last round by keeping the floor and only documenting it. The floor now applies to what cjose writes and not to what it reads: cjose_jwe_encrypt* will not produce a JWE below CJOSE_JWE_PBES2_MIN_ITERATIONS, still 1000, and cjose_jwe_decrypt* accepts any positive count up to the ceiling, which is section 4.8.1.2 exactly. Refusing to read a low count never made the producer's JWE stronger, the count is theirs to choose, and what protects the recipient is the ceiling on the work an unauthenticated JWE can demand. CJOSE_JWE_PBES2_MIN_ACCEPTED_ITERATIONS is there for a deployment that would rather fail closed on a weak count; it defaults to 1.

There is a jwcrypto vector at p2c=100 in the suite, decrypted where the accepted floor admits it and asserted to be refused where a build has raised it. It fails on the previous commit.

Upper bounds. All five PBES2 bounds are CMake variables now, next to CJOSE_ENABLE_RSA1_5, with a row each in the README build options table rather than only a define on an internal header:

cmake -S . -B build -DCJOSE_JWE_PBES2_MAX_ITERATIONS=10000000

A value has to be a positive integer without a leading zero, or configuration fails: 0100000 would otherwise reach the compiler as the octal 32768 and quietly give you a ceiling three times lower than you asked for. The ordering MIN_ACCEPTED <= MIN <= DEFAULT <= MAX <= INT_MAX is a _Static_assert, so an inconsistent set fails to compile rather than failing every encryption at run time, and the test suite follows the configured values: the suite passes at the defaults and with each of the five changed.

Padded p2s. Already gone in 037e4bf, from your review here: a caller-supplied salt input is refused outright, so the encrypt path has nothing verbatim to preserve.

Verified with -pedantic -Wall -Werror, the full suite with CJOSE_ENABLE_RSA1_5 off and on, the jwe suite across six bound configurations, valgrind over the whole suite with no leaks or errors, and no clang-format diff.