Outstanding MultiEncrypter + MultiDecrypter issues: General API, key / alg checks

[Vladimir Dzhuvinov]

(1)

The MultiEncrypter must not generate kid values for the supplied keys because the key identifiers are meant to be set and managed by the key owner (the JWE recipient). The encrypting side must not mess with the kids. Fixed here: 7625405

‌

(2)

The MultiDecrypter seems unable to handle correctly encrypted JWE objects without a kid:

https://bitbucket.org/connect2id/nimbus-jose-jwt/commits/c7b22dab3dd2d70ff5a421090b6fab60aafcefdb#Lsrc/test/java/com/nimbusds/jose/crypto/JWEMultipleRecipientsTest.javaT249

‌

(3)

The MultiEncrypter silently skips a recipient in the JWK set if it cannot resolve the JWE alg:

https://bitbucket.org/connect2id/nimbus-jose-jwt/commits/c7b22dab3dd2d70ff5a421090b6fab60aafcefdb#Lsrc/test/java/com/nimbusds/jose/crypto/JWEMultipleRecipientsTest.javaT293

‌

(4)

The current API to encrypt to multiple JWE recipients where each recipient expects a different JWE alg is not right:

// Compose the JWE JSON secured object to encrypt
JWEObjectJSON jweObjectJson = new JWEObjectJSON(
    new JWEHeader(
        JWEAlgorithm.RSA_OAEP_256,
        EncryptionMethod.A128GCM
    ),
    new Payload("Hello, world!")
);

with a JWK set looking like:

List<JWK> keys = new ArrayList<>();

keys.add(new ECKeyGenerator(Curve.P_256)
    .keyID("ECRecipient")
    .algorithm(JWEAlgorithm.ECDH_ES_A128KW)
    .generate());

keys.add(new RSAKeyGenerator(2048)
    .keyID("RSARecipient")
    .algorithm(JWEAlgorithm.RSA_OAEP_256)
    .generate());

JWKSet jwkSet = new JWKSet(keys);

Considering an API change to allow code like:

new JWEHeader(
        EncryptionMethod.A128GCM
);

‌

(5)

The MultiEncrypter API may need concrete upfront checks for supplied key / (alg via JWE header or JWK) consistency.

The above issues are highlighted in commit c7b22da .

[Egor Puzanov]

The issues 1 and 2 are related to the same problem. How to identify recipient entry for decryption without using key id. The possible solutions is try to decrypt encrypted key of each recipient until successful key decryption for specific Alg. This potentially opens the DoS attack vector.

I can provide fix for the issue 3.

The issues 4 and 5 can be solve if we made alg attribute in the JWEHeader class optional. I can evaluate this possibility.

‌

[Vladimir Dzhuvinov]

The MultiEncrypter and MultiDecrypter need some sort of an explicit “contract” (locked with tests) how they will handle JWKs with and without kid. The contract or parts of this “contract” may be an argument to the Encrypter / Decrypter. When the “contract” is broken this must produce a clear cut exception, ideally upfront and not when the actual en / decryption is taking place.

Per JWE and JWK RFC the kid is optional. Application protocols that use JWEs may or may not place further requirements on the kid. OIDC for example defines the kid as required when the entity has published multiple keys in its JWK set. Otherwise it’s not. Specs can use other params to identify keys, e.g. x5c or x5t. Do you know of any formal specs that use JWE JSON with multiple recipients with normative text about how the JWEs must be structured? I would like to see such specs and what they do in practise. The JOSE and JWT RFCs are "framework" specs, based on which concrete profiles / app specs are then defined.

Processing JWEs or JWSes directly, without some sort of a good framework, can be dangerous. For this reason the lib has gotten these packages:

https://www.javadoc.io/doc/com.nimbusds/nimbus-jose-jwt/latest/com/nimbusds/jose/proc/package-summary.html

https://www.javadoc.io/doc/com.nimbusds/nimbus-jose-jwt/latest/com/nimbusds/jwt/proc/package-summary.html

Because the multi-recipient JWEs are not bounded in recipient size this can indeed be a (DoS) issue. When constructing. And when decrypting (with missing / insufficient key identification). So that was a good point.

About (4) I see two possibilities:

1. A new JWEHeader class that allows alg to be optional and intended specifically for the JWE JSON. Ideally it will reuse all other JWEHeader code, to prevent duplication and inconsistencies over time as the lib receives updates in future.
2. Allowing the alg in the current JWEHeader to be null. This will require upfront alg != null checks in all places where the JWEHeader is used for non-JSON JWEs.

Could there be any other possibilities?

‌

[Egor Puzanov]

Please check the PR https://bitbucket.org/connect2id/nimbus-jose-jwt/pull-requests/107

Issues 1-2: I’ve added support for x5c, x5u, x5t, x5tS256 and kid, this can be used to determine the private key needed to decrypt the JWE. I private key can not be resolve the JOSEException will be raised.

issues 3,5: alg will be validated in the MultiEncrypter constructor. The Exception will be raised if it cannot resolve the JWE alg.
issue 4: not yet solved

‌

[Egor Puzanov]

About(4) I like the second possiblility. It is easy to implement and it doesn’t change the API so much.

[Vladimir Dzhuvinov]

Yeah, a special alg could be a neat work around! We could name it JWK_ALG_DRIVEN or similar as none is taken for JOSE algs without signature.

[Vladimir Dzhuvinov]

Nice, things are getting into final shape. I merged your last PR. I removed the Algorithm.NONE changes that would allow its use in JWEHeaders and instead introduced a new special constant JWEAlgorithm.JWK_ALG to be used in multi-recipient JWEs: ed8925eb

My feeling is that the alg=none is confusing enough already to make it even more so. A dedicated JWE constant that won’t affect any of the existing lib code seems the best approach.

Would you be able to wire the new JWEAlgorithm.JWK_ALG constant into the new code? With that the API issue will be solved. If you think there are other potentially problematic areas let me know before we make it final.

[Egor Puzanov]

Sorry, I can not follow your idea with a new JWEAlgorithm.JWK_ALG constant. How it can help to implement proposed API changes?

new JWEHeader(
        EncryptionMethod.A128GCM
);

I’d prefer to make alg optional in the JWEHeader and add the ‘merge', 'extend' or 'join’ method to the it. To allow merging protected and recipients JWE headers.

[Vladimir Dzhuvinov]

That’s alright. The other day I put together the explanatory article, assuming we have the special new JWEAlgorithm.JWK_ALG const as API “signal” to say “use the alg from the public JWK”:

https://connect2id.com/products/nimbus-jose-jwt/examples/jwe-json-multiple-recipients

Where do you see those merge, extend and join used, in what places in the code? Do you foresee any public API changes / additions to the JWEObjectJSON?

[Vladimir Dzhuvinov]

The multi-recipient JWE is now officially out as part of the new 9.32 release.

Thank you for your generous contribution and spending the time to iron out the last issues.

If you wish to add an affiliation or some project link to your credit just let me know.

https://connect2id.com/products/nimbus-jose-jwt#thanks

Happy coding & programmers day!