feat: add Key object to prevent key/algorithm type confusion (#365)

bshaffer · web-flow · commit bc0df6440dfe · 2021-11-04T09:15:22.000-07:00
diff --git a/README.md b/README.md
@@ -27,6 +27,7 @@ Example
 -------
 ```php
 use Firebase\JWT\JWT;
+use Firebase\JWT\Key;
 
 $key = "example_key";
 $payload = array(
@@ -43,7 +44,7 @@ $payload = array(
  * for a list of spec-compliant algorithms.
  */
 $jwt = JWT::encode($payload, $key);
-$decoded = JWT::decode($jwt, $key, array('HS256'));
+$decoded = JWT::decode($jwt, new Key($key, 'HS256'));
 
 print_r($decoded);
 
@@ -62,12 +63,13 @@ $decoded_array = (array) $decoded;
  * Source: http://self-issued.info/docs/draft-ietf-oauth-json-web-token.html#nbfDef
  */
 JWT::$leeway = 60; // $leeway in seconds
-$decoded = JWT::decode($jwt, $key, array('HS256'));
+$decoded = JWT::decode($jwt, new Key($key, 'HS256'));
 ```
 Example with RS256 (openssl)
 ----------------------------
 ```php
 use Firebase\JWT\JWT;
+use Firebase\JWT\Key;
 
 $privateKey = <<<EOD
 -----BEGIN RSA PRIVATE KEY-----
@@ -106,7 +108,7 @@ $payload = array(
 $jwt = JWT::encode($payload, $privateKey, 'RS256');
 echo "Encode:\n" . print_r($jwt, true) . "\n";
 
-$decoded = JWT::decode($jwt, $publicKey, array('RS256'));
+$decoded = JWT::decode($jwt, new Key($publicKey, 'RS256'));
 
 /*
  NOTE: This will now be an object instead of an associative array. To get
@@ -121,6 +123,9 @@ Example with a passphrase
 -------------------------
 
 ```php
+use Firebase\JWT\JWT;
+use Firebase\JWT\Key;
+
 // Your passphrase
 $passphrase = '[YOUR_PASSPHRASE]';
 
@@ -147,14 +152,15 @@ echo "Encode:\n" . print_r($jwt, true) . "\n";
 // Get public key from the private key, or pull from from a file.
 $publicKey = openssl_pkey_get_details($privateKey)['key'];
 
-$decoded = JWT::decode($jwt, $publicKey, array('RS256'));
+$decoded = JWT::decode($jwt, new Key($publicKey, 'RS256'));
 echo "Decode:\n" . print_r((array) $decoded, true) . "\n";
 ```
 
 Example with EdDSA (libsodium and Ed25519 signature)
 ----------------------------
 ```php
 use Firebase\JWT\JWT;
+use Firebase\JWT\Key;
 
 // Public and private keys are expected to be Base64 encoded. The last
 // non-empty line is used so that keys can be generated with
@@ -177,7 +183,7 @@ $payload = array(
 $jwt = JWT::encode($payload, $privateKey, 'EdDSA');
 echo "Encode:\n" . print_r($jwt, true) . "\n";
 
-$decoded = JWT::decode($jwt, $publicKey, array('EdDSA'));
+$decoded = JWT::decode($jwt, new Key($publicKey, 'EdDSA'));
 echo "Decode:\n" . print_r((array) $decoded, true) . "\n";
 ````
 
@@ -194,6 +200,7 @@ $jwks = ['keys' => []];
 
 // JWK::parseKeySet($jwks) returns an associative array of **kid** to private
 // key. Pass this as the second parameter to JWT::decode.
+// NOTE: The deprecated $supportedAlgorithm must be supplied when parsing from JWK.
 JWT::decode($payload, JWK::parseKeySet($jwks), $supportedAlgorithm);
 ```
 
diff --git a/src/JWT.php b/src/JWT.php
@@ -2,6 +2,7 @@
 
 namespace Firebase\JWT;
 
+use ArrayAccess;
 use DomainException;
 use Exception;
 use InvalidArgumentException;
@@ -58,11 +59,13 @@ class JWT
      * Decodes a JWT string into a PHP object.
      *
      * @param string                    $jwt            The JWT
-     * @param string|array|resource     $key            The key, or map of keys.
+     * @param Key|array<Key>            $keyOrKeyArray  The Key or array of Key objects.
      *                                                  If the algorithm used is asymmetric, this is the public key
-     * @param array                     $allowed_algs   List of supported verification algorithms
+     *                                                  Each Key object contains an algorithm and matching key.
      *                                                  Supported algorithms are 'ES384','ES256', 'HS256', 'HS384',
      *                                                  'HS512', 'RS256', 'RS384', and 'RS512'
+     * @param array                     $allowed_algs   [DEPRECATED] List of supported verification algorithms. Only
+     *                                                  should be used for backwards  compatibility.
      *
      * @return object The JWT's payload as a PHP object
      *
@@ -76,11 +79,11 @@ class JWT
      * @uses jsonDecode
      * @uses urlsafeB64Decode
      */
-    public static function decode($jwt, $key, array $allowed_algs = array())
+    public static function decode($jwt, $keyOrKeyArray, array $allowed_algs = array())
     {
         $timestamp = \is_null(static::$timestamp) ? \time() : static::$timestamp;
 
-        if (empty($key)) {
+        if (empty($keyOrKeyArray)) {
             throw new InvalidArgumentException('Key may not be empty');
         }
         $tks = \explode('.', $jwt);
@@ -103,27 +106,32 @@ public static function decode($jwt, $key, array $allowed_algs = array())
         if (empty(static::$supported_algs[$header->alg])) {
             throw new UnexpectedValueException('Algorithm not supported');
         }
-        if (!\in_array($header->alg, $allowed_algs)) {
-            throw new UnexpectedValueException('Algorithm not allowed');
+
+        list($keyMaterial, $algorithm) = self::getKeyMaterialAndAlgorithm(
+            $keyOrKeyArray,
+            empty($header->kid) ? null : $header->kid
+        );
+
+        if (empty($algorithm)) {
+            // Use deprecated "allowed_algs" to determine if the algorithm is supported.
+            // This opens up the possibility of an attack in some implementations.
+            // @see https://github.com/firebase/php-jwt/issues/351
+            if (!\in_array($header->alg, $allowed_algs)) {
+                throw new UnexpectedValueException('Algorithm not allowed');
+            }
+        } else {
+            // Check the algorithm
+            if (!self::constantTimeEquals($algorithm, $header->alg)) {
+                // See issue #351
+                throw new UnexpectedValueException('Incorrect key for this algorithm');
+            }
         }
         if ($header->alg === 'ES256' || $header->alg === 'ES384') {
             // OpenSSL expects an ASN.1 DER sequence for ES256/ES384 signatures
             $sig = self::signatureToDER($sig);
         }
 
-        if (\is_array($key) || $key instanceof \ArrayAccess) {
-            if (isset($header->kid)) {
-                if (!isset($key[$header->kid])) {
-                    throw new UnexpectedValueException('"kid" invalid, unable to lookup correct key');
-                }
-                $key = $key[$header->kid];
-            } else {
-                throw new UnexpectedValueException('"kid" empty, unable to lookup correct key');
-            }
-        }
-
-        // Check the signature
-        if (!static::verify("$headb64.$bodyb64", $sig, $key, $header->alg)) {
+        if (!static::verify("$headb64.$bodyb64", $sig, $keyMaterial, $header->alg)) {
             throw new SignatureInvalidException('Signature verification failed');
         }
 
@@ -285,18 +293,7 @@ private static function verify($msg, $signature, $key, $alg)
             case 'hash_hmac':
             default:
                 $hash = \hash_hmac($algorithm, $msg, $key, true);
-                if (\function_exists('hash_equals')) {
-                    return \hash_equals($signature, $hash);
-                }
-                $len = \min(static::safeStrlen($signature), static::safeStrlen($hash));
-
-                $status = 0;
-                for ($i = 0; $i < $len; $i++) {
-                    $status |= (\ord($signature[$i]) ^ \ord($hash[$i]));
-                }
-                $status |= (static::safeStrlen($signature) ^ static::safeStrlen($hash));
-
-                return ($status === 0);
+                return self::constantTimeEquals($signature, $hash);
         }
     }
 
@@ -384,6 +381,69 @@ public static function urlsafeB64Encode($input)
         return \str_replace('=', '', \strtr(\base64_encode($input), '+/', '-_'));
     }
 
+
+    /**
+     * Determine if an algorithm has been provided for each Key
+     *
+     * @param string|array $keyOrKeyArray
+     * @param string|null $kid
+     *
+     * @return an array containing the keyMaterial and algorithm
+     */
+    private static function getKeyMaterialAndAlgorithm($keyOrKeyArray, $kid = null)
+    {
+        if (is_string($keyOrKeyArray)) {
+            return array($keyOrKeyArray, null);
+        }
+
+        if ($keyOrKeyArray instanceof Key) {
+            return array($keyOrKeyArray->getKeyMaterial(), $keyOrKeyArray->getAlgorithm());
+        }
+
+        if (is_array($keyOrKeyArray) || $keyOrKeyArray instanceof ArrayAccess) {
+            if (!isset($kid)) {
+                throw new UnexpectedValueException('"kid" empty, unable to lookup correct key');
+            }
+            if (!isset($keyOrKeyArray[$kid])) {
+                throw new UnexpectedValueException('"kid" invalid, unable to lookup correct key');
+            }
+
+            $key = $keyOrKeyArray[$kid];
+
+            if ($key instanceof Key) {
+                return array($key->getKeyMaterial(), $key->getAlgorithm());
+            }
+
+            return array($key, null);
+        }
+
+        throw new UnexpectedValueException(
+            '$keyOrKeyArray must be a string key, an array of string keys, '
+            . 'an instance of Firebase\JWT\Key key or an array of Firebase\JWT\Key keys'
+        );
+    }
+
+    /**
+     * @param string $left
+     * @param string $right
+     * @return bool
+     */
+    public static function constantTimeEquals($left, $right)
+    {
+        if (\function_exists('hash_equals')) {
+            return \hash_equals($left, $right);
+        }
+        $len = \min(static::safeStrlen($left), static::safeStrlen($right));
+
+        $status = 0;
+        for ($i = 0; $i < $len; $i++) {
+            $status |= (\ord($left[$i]) ^ \ord($right[$i]));
+        }
+        $status |= (static::safeStrlen($left) ^ static::safeStrlen($right));
+
+        return ($status === 0);
+    }
+
     /**
      * Helper method to create a JSON error.
      *
diff --git a/src/Key.php b/src/Key.php
@@ -0,0 +1,59 @@
+<?php
+
+namespace Firebase\JWT;
+
+use InvalidArgumentException;
+use OpenSSLAsymmetricKey;
+
+class Key
+{
+    /** @var string $algorithm */
+    private $algorithm;
+
+    /** @var string|resource|OpenSSLAsymmetricKey $keyMaterial */
+    private $keyMaterial;
+
+    /**
+     * @param string|resource|OpenSSLAsymmetricKey $keyMaterial
+     * @param string $algorithm
+     */
+    public function __construct($keyMaterial, $algorithm)
+    {
+        if (
+            !is_string($keyMaterial)
+            && !is_resource($keyMaterial)
+            && !$keyMaterial instanceof OpenSSLAsymmetricKey
+        ) {
+            throw new InvalidArgumentException('Type error: $keyMaterial must be a string, resource, or OpenSSLAsymmetricKey');
+        }
+
+        if (empty($keyMaterial)) {
+            throw new InvalidArgumentException('Type error: $keyMaterial must not be empty');
+        }
+
+        if (!is_string($algorithm)|| empty($keyMaterial)) {
+            throw new InvalidArgumentException('Type error: $algorithm must be a string');
+        }
+
+        $this->keyMaterial = $keyMaterial;
+        $this->algorithm = $algorithm;
+    }
+
+    /**
+     * Return the algorithm valid for this key
+     *
+     * @return string
+     */
+    public function getAlgorithm()
+    {
+        return $this->algorithm;
+    }
+
+    /**
+     * @return string|resource|OpenSSLAsymmetricKey
+     */
+    public function getKeyMaterial()
+    {
+        return $this->keyMaterial;
+    }
+}
diff --git a/tests/JWTTest.php b/tests/JWTTest.php
@@ -344,6 +344,34 @@ public function testEncodeDecode($privateKeyFile, $publicKeyFile, $alg)
         $this->assertEquals('bar', $decoded->foo);
     }
 
+    /**
+     * @runInSeparateProcess
+     * @dataProvider provideEncodeDecode
+     */
+    public function testEncodeDecodeWithKeyObject($privateKeyFile, $publicKeyFile, $alg)
+    {
+        $privateKey = file_get_contents($privateKeyFile);
+        $payload = array('foo' => 'bar');
+        $encoded = JWT::encode($payload, $privateKey, $alg);
+
+        // Verify decoding succeeds
+        $publicKey = file_get_contents($publicKeyFile);
+        $decoded = JWT::decode($encoded, new Key($publicKey, $alg));
+
+        $this->assertEquals('bar', $decoded->foo);
+    }
+
+    public function testArrayAccessKIDChooserWithKeyObject()
+    {
+        $keys = new ArrayObject(array(
+            '1' => new Key('my_key', 'HS256'),
+            '2' => new Key('my_key2', 'HS256'),
+        ));
+        $msg = JWT::encode('abc', $keys['1']->getKeyMaterial(), 'HS256', '1');
+        $decoded = JWT::decode($msg, $keys);
+        $this->assertEquals($decoded, 'abc');
+    }
+
     public function provideEncodeDecode()
     {
         return array(
