Fix several issues with JWE encryption.

[jaimeperez]

The issues, according to the JWA spec are:

• the AL field is not correctly calculated. It must represent the length in bits of the additional authenticated data, not in bytes. Additionally, it must be represented as an unsigned 64-bit octet string in big-endian, not as a simple string.

5.2.2.1.4. The octet string AL is equal to the number of bits in the additional authenticated data A expressed as a 64-bit unsigned big endian integer.

• in order to compute the authentication tag, the ciphertext must be used, not the plaintext.

5.2.2.1.5. A message authentication tag T is computed by applying HMAC [RFC2104] to the following data, in order:

• the additional authenticated data A,
• the initialization vector IV,
• the ciphertext E computed in the previous step, and
• the octet string AL defined above.
• in AES_CBC_HMAC_SHA2, the length of the input key equals to the digest size, that being 32, 48 and 64 octets for each of the three variants.

The AES_CBC_HMAC_SHA2 parameters specific to AES_128_CBC_HMAC_SHA_256 are:

• The input key K is 32 octets long.
• ENC_KEY_LEN is 16 octets.
• MAC_KEY_LEN is 16 octets.
• the integrity key and encryption key are derived as the first and second half of the input key, respectively.

The secondary keys MAC_KEY and ENC_KEY are generated from the input key K as follows. Each of these two keys is an octet string.

• MAC_KEY consists of the initial MAC_KEY_LEN octets of K, in order.
• ENC_KEY consists of the final ENC_KEY_LEN octets of K, in order.

The number of octets in the input key K MUST be the sum of MAC_KEY_LEN and ENC_KEY_LEN. The values of these parameters are specified by the Authenticated Encryption algorithms in Sections 5.2.3 through 5.2.5. Note that the MAC key comes before the encryption key in the input key K; this is in the opposite order of the algorithm names in the identifier "AES_CBC_HMAC_SHA2".

[demianbrecht]

Hi @jaimeperez, thanks for the contribution.

Looking at the sections in the RFC that you've called out, these do all seem to be valid issues and resolved with your patch. We'll have to take a closer look at this and measure potential impact as they're backwards incompatible changes. Unfortunately, things are a little slow right now as most are off for the holidays, so we likely won't be able to get to an in depth review until early in the new year.

[jaimeperez]

Hi Demian,

Thanks a lot! No worries, I understand that the dates are not the best ones, and I'll be myself out of business for a while, probably.

There's another issue I missed in the pull request, for which I don't have a patch yet, unfortunately. After taking a closer look at the specs, I noticed the encrypt() and decrypt() methods deal basically with compact serialization, as in the case of JSON serialization the number of elements in the JWT can vary up to 8 (instead of the fixed amount of 5 assumed in the code). This is ok, but the attempt to make the API not tied directly to the compact serialization unfortunately introduces a bug with the authentication tag. According to the JWE draft:

3.1. JWE Compact Serialization Overview

In the JWE Compact Serialization, no JWE Shared Unprotected Header or JWE Per-Recipient Unprotected Header are used. In this case, the JOSE Header and the JWE Protected Header are the same.

and:

5.1.14. Let the Additional Authenticated Data encryption parameter be ASCII(Encoded Protected Header). However if a JWE AAD value is present (which can only be the case when using the JWE JSON Serialization), instead let the Additional Authenticated Data encryption parameter be ASCII(Encoded Protected Header || '.' || BASE64URL(JWE AAD)).

The problem then is that the serialize_compact() method simply joins all the elements in the tuple with a dot in between, and since the encrypt() method is serialization-agnostic, nothing is calculating the authentication tag using the JOSE header as input, which is mandatory if the final output is using compact serialization.

Unfortunately this needs some major changes to the library, including the API, so I think I should not provide a patch for that as you should be the ones deciding how to proceed...

[demianbrecht]

I've taken a look at this and the change itself looks good. The problem, however, is that it's backwards incompatible. Given the library is already in use and tokens have been issued, this change will result in effectively invalidating all tokens, which may be a Bad Thing depending on application level implementation details around token handling. Optimally, the library would handle this change internally and would mark the old method as deprecated. This could perhaps be achieved by using a private claim replicated as a header parameter.

I would like to see tests added to the suite that ensure that the changes you've made are not undone (i.e. cipher() is called using the last half of the key and hash() is called with the first half). This can be achieved using mock and a custom random number generator or mocking get_random_bytes.

As far as API changes to allow for JSON serialization, this library was developed following the JOSE spec, but was limited to our immediate needs. As such, JSON serialization was not in the picture and would likely be a very low priority issue for us to tackle. If you have suggestions, feel free to log an issue. Otherwise, a patch contribution (in another PR as to not conflate this one) would likely be the most efficient way of having it addressed and released.

[demianbrecht]

This has been merged as part of the 0.2.2 release (and I've added you to CONTRIB). Thanks for the contribution!

[jaimeperez]

Thanks Demian! Hope it didn't create too much trouble with backwards compatibility in the end...

[demianbrecht]

Not at all and conformance is always worth it. Thanks again for the submission.