search

Demonware / jose

Python implementation of the Javascript Object Signing and Encryption (JOSE) framework (https://datatracker.ietf.org/wg/jose/charter/)
BSD 3-Clause "New" or "Revised" License
95 stars 34 forks source link

Bug in authentication tag computation #7

Closed jaimeperez closed 8 years ago

jaimeperez commented 8 years ago

There's a bug in the way the authentication tag is computed, running all tokens issued by the library undecipherable by other libraries, since they will always fail to check the token authentication if there's authenticated data (note that for compact serialization, the header should always be authenticated).

When computing the authentication tag, first we need to concatenate the data authenticated, the IV, the ciphertext and the length of the authenticated data in 64 bits big-endian. This is the input to the HMAC algorithm that yields the authentication tag. However, the values are concatenated in the _jwe_hash_str() method using a period between them, which is against RFC7518.

This PR addresses the problem by removing the period, using just an empty string as glue for the concatenation. In order to keep backwards compatibility and still be able to decrypt old tokens, the legacy mode of _jwe_hash_str() is kept as is. The temporary version number has been increased to 2, so that all new tokens are encrypted with the correct authentication tag, while all the tokens issued before this change can still be identified and decrypted in legacy mode.

jaimeperez commented 8 years ago

You are absolutely right Demian! I'll provide a fix for this tomorrow.

demianbrecht commented 8 years ago

Cheers for the fix @jaimeperez, LGTM.

That said, I'm no longer actively maintaining this package. Perhaps the good folks at Demonware will still look at and integrate this patch. @Newky?

jaimeperez commented 8 years ago

Hi @coldeasy!

Thanks for taking a look into this and going through my PR!

I don't want to hurry you to accept this PR, but please consider this is a critical issue that makes all tokens ciphered with this library non-compliant with the standard. The library is therefore useless to cipher tokens today, and the more time we take to fix it, the more people might be using it, creating tokens that are potential sources of trouble.

Thanks again!

coldeasy commented 8 years ago

@jaimeperez yip, my comments were mainly minor - non blocking. We will look at merging this as soon as possible.

jaimeperez commented 8 years ago

Thanks a lot! I'll try to fix at least the defaults-to-none :smiley:

Newky commented 8 years ago

@jaimeperez Thanks very much for your contribution.. and sorry again for the delay in getting this put through. We have been a little oversubscribed recently.

Thanks again!

jaimeperez commented 8 years ago

No problem at all, thanks for merging and the 3.0 release!

Newky commented 8 years ago

Just a heads up in case it causes confusion. My PR was labeled 3.0 release, but its actually 1.0.0, sorry about that :)