Skip to content

Commit e3fb77d

Browse files
paragonie-securityparagonie-security
committed
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'))
1 parent d2113d9 commit e3fb77d

File tree

4 files changed

+263
-15
lines changed

4 files changed

+263
-15
lines changed

‎src/JWT.php

Lines changed: 61 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,12 @@
22

33
namespace Firebase\JWT;
44

5+
use ArrayAccess;
56
use DomainException;
67
use Exception;
8+
use Firebase\JWT\Keys\JWTKey;
9+
use Firebase\JWT\Keys\KeyInterface;
10+
use Firebase\JWT\Keys\Keyring;
711
use InvalidArgumentException;
812
use UnexpectedValueException;
913
use DateTime;
@@ -111,7 +115,9 @@ public static function decode($jwt, $key, array $allowed_algs = array())
111115
$sig = self::signatureToDER($sig);
112116
}
113117

114-
if (\is_array($key) || $key instanceof \ArrayAccess) {
118+
/** @var Keyring|JWTKey $key */
119+
$key = self::getKeyType($key, $allowed_algs);
120+
if ($key instanceof Keyring) {
115121
if (isset($header->kid)) {
116122
if (!isset($key[$header->kid])) {
117123
throw new UnexpectedValueException('"kid" invalid, unable to lookup correct key');
@@ -121,9 +127,16 @@ public static function decode($jwt, $key, array $allowed_algs = array())
121127
throw new UnexpectedValueException('"kid" empty, unable to lookup correct key');
122128
}
123129
}
130+
if (!($key instanceof JWTKey)) {
131+
throw new UnexpectedValueException('$key should be an instance of JWTKey');
132+
}
124133

125134
// Check the signature
126-
if (!static::verify("$headb64.$bodyb64", $sig, $key, $header->alg)) {
135+
if (!$key->isValidForAlg($header->alg)) {
136+
// See issue #351
137+
throw new UnexpectedValueException('Incorrect key for this algorithm');
138+
}
139+
if (!static::verify("$headb64.$bodyb64", $sig, $key->getKeyMaterial(), $header->alg)) {
127140
throw new SignatureInvalidException('Signature verification failed');
128141
}
129142

@@ -285,18 +298,7 @@ private static function verify($msg, $signature, $key, $alg)
285298
case 'hash_hmac':
286299
default:
287300
$hash = \hash_hmac($algorithm, $msg, $key, true);
288-
if (\function_exists('hash_equals')) {
289-
return \hash_equals($signature, $hash);
290-
}
291-
$len = \min(static::safeStrlen($signature), static::safeStrlen($hash));
292-
293-
$status = 0;
294-
for ($i = 0; $i < $len; $i++) {
295-
$status |= (\ord($signature[$i]) ^ \ord($hash[$i]));
296-
}
297-
$status |= (static::safeStrlen($signature) ^ static::safeStrlen($hash));
298-
299-
return ($status === 0);
301+
return self::constantTimeEquals($signature, $hash);
300302
}
301303
}
302304

@@ -384,6 +386,50 @@ public static function urlsafeB64Encode($input)
384386
return \str_replace('=', '', \strtr(\base64_encode($input), '+/', '-_'));
385387
}
386388

389+
/**
390+
* @param string $left
391+
* @param string $right
392+
* @return bool
393+
*/
394+
public static function constantTimeEquals($left, $right)
395+
{
396+
if (\function_exists('hash_equals')) {
397+
return \hash_equals($left, $right);
398+
}
399+
$len = \min(static::safeStrlen($left), static::safeStrlen($right));
400+
401+
$status = 0;
402+
for ($i = 0; $i < $len; $i++) {
403+
$status |= (\ord($left[$i]) ^ \ord($right[$i]));
404+
}
405+
$status |= (static::safeStrlen($left) ^ static::safeStrlen($right));
406+
407+
return ($status === 0);
408+
}
409+
410+
/**
411+
* @param string|array|ArrayAccess $oldType
412+
* @param string[] $algs
413+
* @return KeyInterface
414+
*/
415+
public static function getKeyType($oldType, $algs)
416+
{
417+
if ($oldType instanceof KeyInterface) {
418+
return $oldType;
419+
}
420+
if (is_string($oldType)) {
421+
return new JWTKey($oldType, $algs);
422+
}
423+
if (is_array($oldType) || $oldType instanceof ArrayAccess) {
424+
$keyring = new Keyring(array());
425+
foreach ($oldType as $kid => $key) {
426+
$keyring[$kid] = new JWTKey($key, $algs);
427+
}
428+
return $keyring;
429+
}
430+
throw new InvalidArgumentException('Invalid type: Must be string or array');
431+
}
432+
387433
/**
388434
* Helper method to create a JSON error.
389435
*
@@ -414,7 +460,7 @@ private static function handleJsonError($errno)
414460
*
415461
* @return int
416462
*/
417-
private static function safeStrlen($str)
463+
public static function safeStrlen($str)
418464
{
419465
if (\function_exists('mb_strlen')) {
420466
return \mb_strlen($str, '8bit');

‎src/Keys/JWTKey.php

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
<?php
2+
namespace Firebase\JWT\Keys;
3+
4+
use Firebase\JWT\JWT;
5+
6+
class JWTKey implements KeyInterface
7+
{
8+
/** @var string $alg */
9+
private $alg;
10+
11+
/** @var string $keyMaterial */
12+
private $keyMaterial;
13+
14+
/**
15+
* @param string $keyMaterial
16+
* @param string|array|null $alg
17+
*/
18+
public function __construct($keyMaterial, $alg = null)
19+
{
20+
if (is_array($alg)) {
21+
$alg = self::guessAlgFromKeyMaterial($keyMaterial, $alg);
22+
} elseif (is_null($alg)) {
23+
$alg = self::guessAlgFromKeyMaterial($keyMaterial);
24+
}
25+
$this->keyMaterial = $keyMaterial;
26+
$this->alg = $alg;
27+
}
28+
29+
/**
30+
* Is the header algorithm valid for this key?
31+
*
32+
* @param string $headerAlg
33+
* @return bool
34+
*/
35+
public function isValidForAlg($headerAlg)
36+
{
37+
return JWT::constantTimeEquals($this->alg, $headerAlg);
38+
}
39+
40+
/**
41+
* @return string
42+
*/
43+
public function getKeyMaterial()
44+
{
45+
return $this->keyMaterial;
46+
}
47+
48+
/**
49+
* This is a best-effort attempt to guess the algorithm for a given key
50+
* based on its contents.
51+
*
52+
* It will probably be wrong in a lot of corner cases.
53+
*
54+
* If it is, construct a JWTKey object and/or Keyring of JWTKey objects
55+
* with the correct algorithms.
56+
*
57+
* @param string $keyMaterial
58+
* @param array $candidates
59+
* @return string
60+
*/
61+
public static function guessAlgFromKeyMaterial($keyMaterial, array $candidates = array())
62+
{
63+
$length = JWT::safeStrlen($keyMaterial);
64+
if ($length >= 720) {
65+
// RSA keys
66+
if (preg_match('#^-+BEGIN.+(PRIVATE|PUBLIC) KEY-+#', $keyMaterial)) {
67+
if (in_array('RS512', $candidates)) {
68+
return 'RS512';
69+
}
70+
if (in_array('RS384', $candidates)) {
71+
return 'RS384';
72+
}
73+
return 'RS256';
74+
}
75+
} elseif ($length >= 220) {
76+
// ECDSA private keys
77+
if (preg_match('#^-+BEGIN EC PRIVATE KEY-+#', $keyMaterial)) {
78+
if (in_array('ES512', $candidates)) {
79+
return 'ES512';
80+
}
81+
if (in_array('ES384', $candidates)) {
82+
return 'ES384';
83+
}
84+
return 'ES256';
85+
}
86+
} elseif ($length >= 170) {
87+
// ECDSA public keys
88+
if (preg_match('#^-+BEGIN EC PUBLICY-+#', $keyMaterial)) {
89+
if (in_array('ES512', $candidates)) {
90+
return 'ES512';
91+
}
92+
if (in_array('ES384', $candidates)) {
93+
return 'ES384';
94+
}
95+
return 'ES256';
96+
}
97+
} elseif ($length >= 40 && $length <= 88) {
98+
// Likely base64-encoded EdDSA key
99+
if (in_array('EdDSA', $candidates)) {
100+
return 'EdDSA';
101+
}
102+
}
103+
104+
// Last resort: HMAC
105+
if (in_array('HS512', $candidates)) {
106+
return 'HS512';
107+
}
108+
if (in_array('HS384', $candidates)) {
109+
return 'HS384';
110+
}
111+
return 'HS256';
112+
}
113+
}

‎src/Keys/KeyInterface.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?php
2+
3+
namespace Firebase\JWT\Keys;
4+
5+
interface KeyInterface
6+
{
7+
8+
}

‎src/Keys/Keyring.php

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
<?php
2+
namespace Firebase\JWT\Keys;
3+
4+
use ArrayAccess;
5+
use RuntimeException;
6+
7+
final class Keyring implements ArrayAccess, KeyInterface
8+
{
9+
/** @var array<string, JWTKey> $mapping */
10+
private $mapping;
11+
12+
/**
13+
* @param array<string, JWTKey> $mapping
14+
*/
15+
public function __construct(array $mapping = array())
16+
{
17+
$this->mapping = $mapping;
18+
}
19+
20+
/**
21+
* @param string $keyId
22+
* @param JWTKey $key
23+
* @return $this
24+
*/
25+
public function mapKeyId($keyId, JWTKey $key)
26+
{
27+
$this->mapping[$keyId] = $key;
28+
return $this;
29+
}
30+
31+
/**
32+
* @param mixed $offset
33+
* @return bool
34+
*/
35+
public function offsetExists($offset)
36+
{
37+
if (!is_string($offset)) {
38+
throw new RuntimeException('Type error: argument 1 must be a string');
39+
}
40+
return array_key_exists($offset, $this->mapping);
41+
}
42+
43+
/**
44+
* @param mixed $offset
45+
* @return JWTKey
46+
*/
47+
public function offsetGet($offset)
48+
{
49+
$value = $this->mapping[$offset];
50+
if (!($value instanceof JWTKey)) {
51+
throw new RuntimeException('Type error: return value not an instance of JWTKey');
52+
}
53+
return $value;
54+
}
55+
56+
/**
57+
* @param string $offset
58+
* @param JWTKey $value
59+
*/
60+
public function offsetSet($offset, $value)
61+
{
62+
if (!is_string($offset)) {
63+
throw new RuntimeException('Type error: argument 1 must be a string');
64+
}
65+
if (!($value instanceof JWTKey)) {
66+
throw new RuntimeException('Type error: argument 2 must be an instance of JWT');
67+
}
68+
$this->mapKeyId($offset, $value);
69+
}
70+
71+
/**
72+
* @param string $offset
73+
*/
74+
public function offsetUnset($offset)
75+
{
76+
if (!is_string($offset)) {
77+
throw new RuntimeException('Type error: argument 1 must be a string');
78+
}
79+
unset($this->mapping[$offset]);
80+
}
81+
}

0 commit comments

Comments
 (0)