From 31a7c1692907a10dfdb2ea88115ac7164fe53d58 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Wed, 4 Aug 2021 05:42:09 -0400 Subject: [PATCH 1/2] Proposed Fix for #351 This aims to provide backwards compatibility by guessing the algorithm for a key based on the key's contents, but it will likely fail in corner cases. If this is merged, users **SHOULD** be explicit about the algorithms they're using. i.e. Instead of $keyAsString, pass in (new JWTKey($keyAsString, 'ES384')) --- src/JWT.php | 75 ++++++++++++++++++++++------ src/Keys/JWTKey.php | 113 +++++++++++++++++++++++++++++++++++++++++++ src/Keys/Keyring.php | 81 +++++++++++++++++++++++++++++++ 3 files changed, 254 insertions(+), 15 deletions(-) create mode 100644 src/Keys/JWTKey.php create mode 100644 src/Keys/Keyring.php diff --git a/src/JWT.php b/src/JWT.php index 99d6dcd2..36abd0bd 100644 --- a/src/JWT.php +++ b/src/JWT.php @@ -2,8 +2,11 @@ namespace Firebase\JWT; +use ArrayAccess; use DomainException; use Exception; +use Firebase\JWT\Keys\JWTKey; +use Firebase\JWT\Keys\Keyring; use InvalidArgumentException; use UnexpectedValueException; use DateTime; @@ -111,7 +114,9 @@ public static function decode($jwt, $key, array $allowed_algs = array()) $sig = self::signatureToDER($sig); } - if (\is_array($key) || $key instanceof \ArrayAccess) { + /** @var Keyring|JWTKey $key */ + $key = self::getKeyType($key, $allowed_algs); + if ($key instanceof Keyring) { if (isset($header->kid)) { if (!isset($key[$header->kid])) { throw new UnexpectedValueException('"kid" invalid, unable to lookup correct key'); @@ -121,9 +126,16 @@ public static function decode($jwt, $key, array $allowed_algs = array()) throw new UnexpectedValueException('"kid" empty, unable to lookup correct key'); } } + if (!($key instanceof JWTKey)) { + throw new UnexpectedValueException('$key should be an instance of JWTKey'); + } // Check the signature - if (!static::verify("$headb64.$bodyb64", $sig, $key, $header->alg)) { + if (!$key->isValidForAlg($header->alg)) { + // See issue #351 + throw new UnexpectedValueException('Incorrect key for this algorithm'); + } + if (!static::verify("$headb64.$bodyb64", $sig, $key->getKeyMaterial(), $header->alg)) { throw new SignatureInvalidException('Signature verification failed'); } @@ -285,18 +297,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 +385,50 @@ public static function urlsafeB64Encode($input) return \str_replace('=', '', \strtr(\base64_encode($input), '+/', '-_')); } + /** + * @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); + } + + /** + * @param string|array|ArrayAccess $oldType + * @param string[] $algs + * @return KeyInterface + */ + public static function getKeyType($oldType, $algs) + { + if ($oldType instanceof KeyInterface) { + return $oldType; + } + if (is_string($oldType)) { + return new JWTKey($oldType, $algs); + } + if (is_array($oldType) || $oldType instanceof ArrayAccess) { + $keyring = new Keyring(array()); + foreach ($oldType as $kid => $key) { + $keyring[$kid] = new JWTKey($key, $algs); + } + return $keyring; + } + throw new InvalidArgumentException('Invalid type: Must be string or array'); + } + /** * Helper method to create a JSON error. * @@ -414,7 +459,7 @@ private static function handleJsonError($errno) * * @return int */ - private static function safeStrlen($str) + public static function safeStrlen($str) { if (\function_exists('mb_strlen')) { return \mb_strlen($str, '8bit'); diff --git a/src/Keys/JWTKey.php b/src/Keys/JWTKey.php new file mode 100644 index 00000000..6e095ad7 --- /dev/null +++ b/src/Keys/JWTKey.php @@ -0,0 +1,113 @@ +keyMaterial = $keyMaterial; + $this->alg = $alg; + } + + /** + * Is the header algorithm valid for this key? + * + * @param string $headerAlg + * @return bool + */ + public function isValidForAlg($headerAlg) + { + return JWT::constantTimeEquals($this->alg, $headerAlg); + } + + /** + * @return string + */ + public function getKeyMaterial() + { + return $this->keyMaterial; + } + + /** + * This is a best-effort attempt to guess the algorithm for a given key + * based on its contents. + * + * It will probably be wrong in a lot of corner cases. + * + * If it is, construct a JWTKey object and/or Keyring of JWTKey objects + * with the correct algorithms. + * + * @param string $keyMaterial + * @param array $candidates + * @return string + */ + public static function guessAlgFromKeyMaterial($keyMaterial, array $candidates = array()) + { + $length = JWT::safeStrlen($keyMaterial); + if ($length >= 720) { + // RSA keys + if (preg_match('#^-+BEGIN.+(PRIVATE|PUBLIC) KEY-+#', $keyMaterial)) { + if (in_array('RS512', $candidates)) { + return 'RS512'; + } + if (in_array('RS384', $candidates)) { + return 'RS384'; + } + return 'RS256'; + } + } elseif ($length >= 220) { + // ECDSA private keys + if (preg_match('#^-+BEGIN EC PRIVATE KEY-+#', $keyMaterial)) { + if (in_array('ES512', $candidates)) { + return 'ES512'; + } + if (in_array('ES384', $candidates)) { + return 'ES384'; + } + return 'ES256'; + } + } elseif ($length >= 170) { + // ECDSA public keys + if (preg_match('#^-+BEGIN EC PUBLICY-+#', $keyMaterial)) { + if (in_array('ES512', $candidates)) { + return 'ES512'; + } + if (in_array('ES384', $candidates)) { + return 'ES384'; + } + return 'ES256'; + } + } elseif ($length >= 40 && $length <= 88) { + // Likely base64-encoded EdDSA key + if (in_array('EdDSA', $candidates)) { + return 'EdDSA'; + } + } + + // Last resort: HMAC + if (in_array('HS512', $candidates)) { + return 'HS512'; + } + if (in_array('HS384', $candidates)) { + return 'HS384'; + } + return 'HS256'; + } +} diff --git a/src/Keys/Keyring.php b/src/Keys/Keyring.php new file mode 100644 index 00000000..72572a86 --- /dev/null +++ b/src/Keys/Keyring.php @@ -0,0 +1,81 @@ + $mapping */ + private $mapping; + + /** + * @param array $mapping + */ + public function __construct(array $mapping = array()) + { + $this->mapping = $mapping; + } + + /** + * @param string $keyId + * @param JWTKey $key + * @return $this + */ + public function mapKeyId($keyId, JWTKey $key) + { + $this->mapping[$keyId] = $key; + return $this; + } + + /** + * @param mixed $offset + * @return bool + */ + public function offsetExists($offset) + { + if (!is_string($offset)) { + throw new RuntimeException('Type error: argument 1 must be a string'); + } + return array_key_exists($offset, $this->mapping); + } + + /** + * @param mixed $offset + * @return JWTKey + */ + public function offsetGet($offset) + { + $value = $this->mapping[$offset]; + if (!($value instanceof JWTKey)) { + throw new RuntimeException('Type error: return value not an instance of JWTKey'); + } + return $value; + } + + /** + * @param string $offset + * @param JWTKey $value + */ + public function offsetSet($offset, $value) + { + if (!is_string($offset)) { + throw new RuntimeException('Type error: argument 1 must be a string'); + } + if (!($value instanceof JWTKey)) { + throw new RuntimeException('Type error: argument 2 must be an instance of JWT'); + } + $this->mapKeyId($offset, $value); + } + + /** + * @param string $offset + */ + public function offsetUnset($offset) + { + if (!is_string($offset)) { + throw new RuntimeException('Type error: argument 1 must be a string'); + } + unset($this->mapping[$offset]); + } +} From e08160a44d4fb7b639c791672419a91d557750e1 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Wed, 4 Aug 2021 06:03:00 -0400 Subject: [PATCH 2/2] Add unit tests for current behavior --- tests/Keys/JWTKeyTest.php | 87 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 tests/Keys/JWTKeyTest.php diff --git a/tests/Keys/JWTKeyTest.php b/tests/Keys/JWTKeyTest.php new file mode 100644 index 00000000..6084fa7a --- /dev/null +++ b/tests/Keys/JWTKeyTest.php @@ -0,0 +1,87 @@ +getEcdsaPublicKey(); + $rsa = $this->getRsaPublicKey(); + $misc = 'maybe_use_paseto_instead'; + + $this->assertSame( + 'RS512', + JWTKey::guessAlgFromKeyMaterial($rsa, array('RS512')) + ); + $this->assertSame( + 'RS384', + JWTKey::guessAlgFromKeyMaterial($rsa, array('RS384')) + ); + $this->assertSame( + 'RS256', + JWTKey::guessAlgFromKeyMaterial($rsa, array('RS256')) + ); + $this->assertSame( + 'RS256', + JWTKey::guessAlgFromKeyMaterial($rsa) + ); + $this->assertSame( + 'ES384', + JWTKey::guessAlgFromKeyMaterial($ecc384, array('ES384')) + ); + $this->assertSame( + 'ES256', + JWTKey::guessAlgFromKeyMaterial($ecc384) + ); + $this->assertSame( + 'EdDSA', + JWTKey::guessAlgFromKeyMaterial($eddsa, array('EdDSA')) + ); + $this->assertSame( + 'HS256', + JWTKey::guessAlgFromKeyMaterial($eddsa) + ); + $this->assertSame( + 'HS384', + JWTKey::guessAlgFromKeyMaterial($misc, array('HS512')) + ); + $this->assertSame( + 'HS384', + JWTKey::guessAlgFromKeyMaterial($misc, array('HS384')) + ); + $this->assertSame( + 'HS256', + JWTKey::guessAlgFromKeyMaterial($misc, array('HS256')) + ); + $this->assertSame( + 'HS256', + JWTKey::guessAlgFromKeyMaterial($misc) + ); + } + + public function getRsaPublicKey() + { + $privKey = openssl_pkey_new(array('digest_alg' => 'sha256', + 'private_key_bits' => 1024, + 'private_key_type' => OPENSSL_KEYTYPE_RSA)); + $pubKey = openssl_pkey_get_details($privKey); + return $pubKey['key']; + } + + public function getEcdsaPublicKey() + { + $privKey = openssl_pkey_new( + array( + 'curve_name' => 'secp384r1', + 'digest_alg' => 'sha384', + 'private_key_bits' => 384, + 'private_key_type' => OPENSSL_KEYTYPE_EC + ) + ); + $pubKey = openssl_pkey_get_details($privKey); + return $pubKey['key']; + } +}