Skip to content

Commit

Permalink
Proposed Fix for #351
Browse files Browse the repository at this point in the history
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'))
  • Loading branch information
paragonie-security committed Aug 4, 2021
1 parent d2113d9 commit 31a7c16
Show file tree
Hide file tree
Showing 3 changed files with 254 additions and 15 deletions.
75 changes: 60 additions & 15 deletions src/JWT.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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');
Expand All @@ -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');
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -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');
Expand Down
113 changes: 113 additions & 0 deletions src/Keys/JWTKey.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
<?php
namespace Firebase\JWT\Keys;

use Firebase\JWT\JWT;

class JWTKey
{
/** @var string $alg */
private $alg;

/** @var string $keyMaterial */
private $keyMaterial;

/**
* @param string $keyMaterial
* @param string|array|null $alg
*/
public function __construct($keyMaterial, $alg = null)
{
if (is_array($alg)) {
$alg = self::guessAlgFromKeyMaterial($keyMaterial, $alg);
} elseif (is_null($alg)) {
$alg = self::guessAlgFromKeyMaterial($keyMaterial);
}
$this->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';
}
}
81 changes: 81 additions & 0 deletions src/Keys/Keyring.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php
namespace Firebase\JWT\Keys;

use ArrayAccess;
use RuntimeException;

final class Keyring implements ArrayAccess
{
/** @var array<string, JWTKey> $mapping */
private $mapping;

/**
* @param array<string, JWTKey> $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]);
}
}

0 comments on commit 31a7c16

Please sign in to comment.