From e3fb77d29625a3ccf86288cfc4c31c0b5ce2cbf5 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Wed, 4 Aug 2021 05:42:09 -0400 Subject: [PATCH] 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 | 76 ++++++++++++++++++++----- src/Keys/JWTKey.php | 113 ++++++++++++++++++++++++++++++++++++++ src/Keys/KeyInterface.php | 8 +++ src/Keys/Keyring.php | 81 +++++++++++++++++++++++++++ 4 files changed, 263 insertions(+), 15 deletions(-) create mode 100644 src/Keys/JWTKey.php create mode 100644 src/Keys/KeyInterface.php create mode 100644 src/Keys/Keyring.php diff --git a/src/JWT.php b/src/JWT.php index 99d6dcd2..fddb75cf 100644 --- a/src/JWT.php +++ b/src/JWT.php @@ -2,8 +2,12 @@ namespace Firebase\JWT; +use ArrayAccess; use DomainException; use Exception; +use Firebase\JWT\Keys\JWTKey; +use Firebase\JWT\Keys\KeyInterface; +use Firebase\JWT\Keys\Keyring; use InvalidArgumentException; use UnexpectedValueException; use DateTime; @@ -111,7 +115,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 +127,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 +298,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 +386,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 +460,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..8f0a0a1b --- /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/KeyInterface.php b/src/Keys/KeyInterface.php new file mode 100644 index 00000000..6e32ff2c --- /dev/null +++ b/src/Keys/KeyInterface.php @@ -0,0 +1,8 @@ + $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]); + } +}