Encrypt generates corrupt signature

[mwillbanks]

When utilizing the encrypt method there are times when you want to decrypt and verify the token. Unfortunately this is not possible as the encryption passes the claims encrypted to the encode method. This causes a corrupt signature thus rendering the verification of the token useless.

There are a few different pathways to a solution here...

1. Generate the signature always leveraging the set claims (aka encode does not utilized the passed in claims to generate the signature but rather the classes assigned claims).
2. Pass in a signature and generate it within the encrypt method.

Other Thought and Better Approach
The library should likely treat decode and encode as a pre-processor and a post-processor. Meaning that they should only be run on a generated token. This would then mean the encode and decode have the responsibility of finding their area (aka claims by delimiter), and encoding the result from that standpoint vs. encoding the raw data. This would make it significantly more clean whilst not destroying the signature.

It would be up to the clients in this case to be able to understand how to decode the item.

[enygma]

Are you saying that the decrypt method isn't working correctly or that tools in other languages are having problems with the resulting encrypted token this library creates?

I'm not sure what you mean by "classes assigned to claims" here either.

[mwillbanks]

The decrypt method works fine - but you never validate the token and therefore the signature is invalid. Take a look at the following code changes to see what I mean:

diff --git a/src/Psecio/Jwt/Jwt.php b/src/Psecio/Jwt/Jwt.php
index fe84edf..bb02ff8 100644
--- a/src/Psecio/Jwt/Jwt.php
+++ b/src/Psecio/Jwt/Jwt.php
@@ -168,13 +168,17 @@ class Jwt
            throw new \RuntimeException('Cannot encrypt data, OpenSSL not enabled');
        }

-       $data = json_encode($this->getClaims()->toArray());
+       $token = $this->encode();
+       $token = explode('.', $token);
+       $data  = $token[1];

-       $claims = $this->base64Encode(openssl_encrypt(
+       $token[1] = $this->base64Encode(openssl_encrypt(
            $data, $algorithm, $key, false, $iv
        ));

-       return $this->encode($claims);
+       $token = implode('.', $token);
+
+       return $token;
    }

    /**
@@ -199,11 +204,17 @@ class Jwt
            throw new Exception\DecodeException('Invalid number of sections (<3)');
        }

-       $claims = openssl_decrypt(
+       $sections[1] = openssl_decrypt(
            $this->base64Decode($sections[1]), $algorithm, $key, false, $iv
        );

-       return json_decode($claims);
+       if (!$sections[1]) {
+           return false;
+       }
+
+       $token = implode('.', $sections);
+
+       return $token;
    }

    /**

Basically, what should be happening is that the encryption should happen after the encoding vs. before the encoding because otherwise the signature is corrupted. Right now you can encrypt and decrypt but you cannot encrypt, and then decrypt and verify because the signature is no longer what the claims would have been.

In the example above, this would change the implementation to work slightly different:

$encrypted = $jwt->encrypt(....);
$token = $jwt->decrypt(....);
if ($jwt->verify($token, ...)) {
    var_dump($jwt->decode($token, ....));
}

[enygma]

Ah, I see what you mean now - I remember this decision coming up. I ended up deferring to how some other libraries were handling this. It did make it difficult to do that token verification on the decrypt.

So you're proposing adding the verify to the encryption handling as well then essentially?

[mwillbanks]

Not really, what I really propose is that:

1. Encryption should only happen on a token itself and encrypt the claims after they have been encoded.
2. Decryption should only happen on a token itself and return the token to be decoded or verified.
   a. Right now decryption would return back the claims payload. No verification at this point could take place on the signature nor the headers. The signature is also incomplete as it is the signature of the encrypted claims vs. a signature of the encoded claims.

[mwillbanks]

For transparency purposes, node.js app for decrypting and verifying a token:
https://gist.github.com/mwillbanks/0bcd17b02ac2726bbe33

This will show the error as the signature will not be valid. From that standpoint to fix it temporarily means off of the library I would do:

$token = $jwt->encrypt(...);
$signatureToken = $jwt->encode();
$token = explode('.', $token);
$signatureToken = explode('.', $signatureToken);

$token[2] = $signatureToken[2];
$token = implode('.', $token);

Although it still leaves a double base64 encoding on the decryption side :)

[enygma]

I think part of the confusion here is where the encryption comes into play. Right now the encrypt/decrypt methods are trying to encrypt the claims themselves and not just use the OpenSSL stuff for the signature (much like this library does).

From what I can tell in your example code, you're essentially wanting to verify that the encrypted token is correct. My library is a little mixed up on this right now, but I think I'm seeing a way out. The encode handling needs to just use whatever hash method is defined in the header and, if it's not HMAC, use the data given to encrypt. The encrypt and decrypt methods, however, need to make the extra step of encrypting the claims too (really this is a JWE, not just a JWT/JWS). I'm trying to figure out the best course of action here to split out the JWT, JWS and JWE functionality and still maintain at least a somewhat similar interface.

[enygma]

Okay, so i've pretty heavily refactored things. The hashing has been corrected and now signs with OpenSSL more correctly. I'm still working on the encryption (JWE) part, so stay tuned...