Still vulnerable with multiple allowed algorithms

[cschomburg]

Maybe I'm missing something obvious, but I tried your library and it is still vulnerable to type confusion when using multiple allowed algorithms (i.e. your first example in firebase/php-jwt#351).

Furthermore your JWT::encode function has two problems:

• Encode does not work when you supply a keyring (keyId check is wrong)
• PHPDoc parameters are wrong for $key, $keyId and $head

Consider this test case:

<?php

use ParagonIE\PhpJwtGuard\JWT;
use ParagonIE\PhpJwtGuard\KeyRing;
use PHPUnit\Framework\TestCase;

class JWTTest extends TestCase
{
    public function testConfusion(): void
    {
        $hsKey = hash('sha256', 'phpunit-test-key-for-issue-351');

        $esPubkey = <<<END
            -----BEGIN PUBLIC KEY-----
            MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEEVs/o5+uQbTjL3chynL4wXgUg2R9
            q9UU8I5mEovUf86QZ7kOBIjJwqnzD1omageEHWwHdBO6B+dFabmdT9POxg==
            -----END PUBLIC KEY-----
            END;

        $keyring = new KeyRing();
        $keyring->with('HS256', 'foo', $hsKey);
        $keyring->with('ES256', 'bar', $esPubkey);

        $payload = ['sub' => 'phpunit'];
        $token = JWT::encode($payload, $esPubkey, 'HS256', 'bar'); // wrong algo

        $fail = false;
        try {
            JWT::decode($token, $keyring, ['HS256', 'ES256']);
            $fail = true;
        } catch (UnexpectedValueException $ex) {
        }
        $this->assertFalse($fail, 'Expected an exception');
    }

    public function testEncode(): void
    {
        $keyring = new KeyRing();
        $keyring->with('HS256', 'foo', hash('sha256', 'phpunit-test-key-for-issue-351'));

        $payload = ['sub' => 'phpunit'];
        JWT::encode($payload, $keyring, 'HS256', 'foo'); // should not throw exception
    }
}

[paragonie-security]

The first example in that example is actually a dud created in haste when trying to understand the full impact of the finding, and we need to correct it because it's confusing people.

If you are permitting both algorithms, explicitly, in the same call, then you've deliberately made the choice to permit algorithm confusion. You can permit none as well.

If your argument is, "We should prevent that in this wrapper library too," then that's worth considering.

[cschomburg]

I definitely think it's worth considering, since I ran into this issue and immediately tried the wrapper after seeing your post as a hotfix, with the impression that it also prevents the problem with multiple allowed algorithms.

Or alternatively you might want to state explicitly in the README that this is not covered, since I suspect other people will run into the same issue and may be misled into thinking it will fix the issue.

Thanks for your immediate response and making people aware of this vulnerability!

[paragonie-security]

This is fixed in v0.3.0.