lexik / LexikJWTAuthenticationBundle

JWT authentication for your Symfony API
MIT License
2.53k stars 610 forks source link

Change JWT signature verification to use alg specified in JWT header #1241

Closed nuryagdym closed 2 hours ago

nuryagdym commented 3 hours ago

In the LcobucciJWSProvider https://github.com/lexik/LexikJWTAuthenticationBundle/blob/ab7493df8cffb191dbd6910f972726c120d8e577/Services/JWSProvider/LcobucciJWSProvider.php#L171 we currently use $this->signer that is set during service initialisation.

what do you think if we update verify method so that it decides signer according to JWT header:

    private function verify(Token $jwt): bool
    {
        $signer = $this->signer;
        if (null !== $jwt->headers()->get('alg') && $this->signer->algorithmId() !== $jwt->headers()->get('alg')) {
            $signer = $this->getSignerForAlgorithm( $jwt->headers()->get('alg'));
        }
        // replace all usages of $this->signer with $signer
        $key = InMemory::plainText($signer instanceof Hmac ? $this->keyLoader->loadKey(KeyLoaderInterface::TYPE_PRIVATE) : $this->keyLoader->loadKey(KeyLoaderInterface::TYPE_PUBLIC));
       // ...
   }

Use case: I have a project where currently JWTs signed with Hmac algorithms are support and I need to add support for the new JWT signed with RSA algorithm. New JWTs will be generated from different micro service. Both of the JWTs should be supported for a time until all Hmac JWTs are invalid, then at some point we can switch completely to RSA JWTs.

Note that, currently with the support only for Hmac signature_algorithm: 'HS256' config is set in lexic bundle config.

And these change in verify method and setting public key in lexik config allows me to authenticate both Hmac and RSA JWTs.

Another solution would to implement custom LcobucciJWSProvider to override existing one. Extending LcobucciJWSProvider is also does not help as all internal methods are private, I will need to copy whole class.

chalasr commented 3 hours ago

Hey,

what do you think if we update verify method so that it decides signer according to JWT header:

Trusting the alg header is a known attack vector so that's a no-go for this bundle, and I highly suggest not to take this path on your end.

Verifying two sorts of algorithms (one symmetric and one asymmetric) also opens up the door to vulnerabilities. So I recommend to be very careful with your implementation.

Both of the JWTs should be supported for a time until all Hmac JWTs are invalid, then at some point we can switch completely to RSA JWTs.

If that's temporary as it should definitely be, then writing some temporary implementation on your own to wire another JWT encoder + provider through dependency-injection is probably the way to go, eventually setting up a chain encoder that tries 2 encoders using their respective algorithms, with all the required carefulness mentioned above. Or you could maybe rely on events dispatched by the bundle instead.

I hope this helps.

nuryagdym commented 2 hours ago

@chalasr thanks for the quick response. Hmm, did not know about this vulnerabilities, thanks for pointing them out.

I guess encoder + provider with chain encoder will work for.