Skip to content

Commit

Permalink
Mitigation of security issue CVE-2021-46743
Browse files Browse the repository at this point in the history
  • Loading branch information
dakujem committed Apr 19, 2023
1 parent 6b47d8c commit 859fc8d
Show file tree
Hide file tree
Showing 13 changed files with 187 additions and 163 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/php-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
strategy:
matrix:
include:
- php: '7.4'
- php: '8.0'
composer-flags: '--prefer-lowest'
- php: '8.0'
- php: '8.1'
Expand Down
17 changes: 17 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,23 @@ Auth-middleware follows semantic versioning.\
Any issues should be reported.


## v2.0

This update reflects security vulnerability patch for **CVE-2021-46743** from `firebase/php-jwt` package in version 5.5 and 6.
As a result, the interface of `FirebaseJwtDecoder` and certain `AuthWizard` methods that create the decoder have been changed.

- [BC break] dropped PHP 7 support; PHP 8 is now required
- [BC break] removed `AuthFactory::defaultDecoderFactory`, please use `fn() => AuthWizard::defaultDecoder( new Secret($secret,$algo) )` instead
- [BC break] changed the constructor of `FirebaseJwtDecoder` to only accept `SecretContract` implementations
- [BC break] using `AuthWizard::defaultDecoder`, `AuthWizard::decodeTokens()` and `AuthWizard::factory()->decodeTokens()` with `string` `$secret` argument will now only decode tokens using the single default `HS256` algorithm
- previously the same calls resulted in use of any one of the three `HS256`, `HS512`, `HS384` algorithms (the attack vector)
- to mitigate the issue, use an array of key-algo pairs (`Secret[]`) along with `kid` header parameter (see [section 4.5 of RFC 7517](https://www.ietf.org/rfc/rfc7517.txt))
- the default `FirebaseJwtDecoder` now only works with `firebase/php-jwt` versions 5.5 and 6+ (`5.5.* - 6.*`)
- added `AuthWizard::defaultDecoder` method that directly returns an instance of the `FirebaseJwtDecoder` decoder

> For more details, see [this issue](https://github.com/firebase/php-jwt/issues/351) or [release notes](https://github.com/firebase/php-jwt/releases/tag/v6.0.0).

## v1.2

Provides means for mitigation of security vulnerability **CVE-2021-46743** by using the new `Secret` configuration object.
Expand Down
4 changes: 2 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@
}
],
"require": {
"php": "^7.4 || ^8.0",
"php": "^8.0",
"dakujem/generic-middleware": "^1",
"psr/http-factory": "^1.0",
"psr/http-server-middleware": "^1.0",
"psr/log": "^1.0"
},
"require-dev": {
"ext-json": "*",
"firebase/php-jwt": "^5",
"firebase/php-jwt": "^6.0|^5.5",
"nette/tester": "^2.4.1",
"slim/psr7": "^1.2",
"slim/slim": "^4.5",
Expand Down
20 changes: 0 additions & 20 deletions src/Factory/AuthFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@

namespace Dakujem\Middleware\Factory;

use Dakujem\Middleware\FirebaseJwtDecoder;
use Dakujem\Middleware\GenericMiddleware;
use Dakujem\Middleware\SecretContract;
use Dakujem\Middleware\TokenManipulators as Man;
use Dakujem\Middleware\TokenMiddleware;
use LogicException;
Expand Down Expand Up @@ -165,22 +163,4 @@ public function inspectTokens(
);
});
}

/**
* @deprecated please use AuthWizard::defaultDecoder instead
*
* Creates a default decoder factory.
* The factory can be used for the constructor.
*
* @param string|SecretContract[]|SecretContract $secret secret key for JWT decoder
* @param string|null $algo optional algorithm; only used when $secret is a string
* @return callable fn():FirebaseJwtDecoder
*/
public static function defaultDecoderFactory(
$secret,
?string $algo = null
): callable {
$decoder = AuthWizard::defaultDecoder($secret, $algo);
return fn(): FirebaseJwtDecoder => $decoder;
}
}
25 changes: 12 additions & 13 deletions src/Factory/AuthWizard.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ final class AuthWizard
* @return TokenMiddleware
*/
public static function decodeTokens(
$secret,
string|array|SecretContract $secret,
?string $tokenAttribute = null,
?string $headerName = Man::HEADER_NAME,
?string $cookieName = Man::COOKIE_NAME,
?string $errorAttribute = null,
?Logger $logger = null
?Logger $logger = null,
): MiddlewareInterface {
return self::factory($secret, null)->decodeTokens(
$tokenAttribute,
Expand All @@ -64,7 +64,7 @@ public static function decodeTokens(
public static function assertTokens(
ResponseFactory $responseFactory,
?string $tokenAttribute = null,
?string $errorAttribute = null
?string $errorAttribute = null,
): MiddlewareInterface {
return self::factory(null, $responseFactory)->assertTokens($tokenAttribute, $errorAttribute);
}
Expand All @@ -82,7 +82,7 @@ public static function inspectTokens(
ResponseFactory $responseFactory,
callable $inspector,
?string $tokenAttribute = null,
?string $errorAttribute = null
?string $errorAttribute = null,
): MiddlewareInterface {
return self::factory(null, $responseFactory)->inspectTokens($inspector, $tokenAttribute, $errorAttribute);
}
Expand All @@ -95,8 +95,8 @@ public static function inspectTokens(
* @return AuthFactory
*/
public static function factory(
$secret,
?ResponseFactory $responseFactory
string|array|SecretContract|null $secret,
?ResponseFactory $responseFactory,
): AuthFactory {
$decoder = $secret !== null ? self::defaultDecoder($secret) : null;
return new AuthFactory(
Expand All @@ -115,21 +115,20 @@ public static function factory(
* @throws
*/
public static function defaultDecoder(
$secret,
?string $algo = null
string|array|SecretContract $secret,
?string $algo = null,
): callable {
if (!class_exists(JWT::class)) {
throw new LogicException(
'Firebase JWT is not installed. ' .
'Requires firebase/php-jwt package (`composer require firebase/php-jwt:"^5.5"`).'
'Requires firebase/php-jwt package (`composer require firebase/php-jwt:"^6.0|^5.5"`).'
);
}
if ($algo !== null && is_string($secret)) {
$secret = new Secret($secret, $algo);
if (is_string($secret)) {
$secret = new Secret($secret, $algo ?? self::$defaultAlgo);
}
return new FirebaseJwtDecoder(
$secret,
$algo,
...(is_iterable($secret) ? $secret : [$secret])
);
}
}
106 changes: 18 additions & 88 deletions src/FirebaseJwtDecoder.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,116 +8,47 @@
use Firebase\JWT\JWT;
use Firebase\JWT\Key;
use InvalidArgumentException;
use LogicException;
use Psr\Log\LoggerInterface;
use Psr\Log\LogLevel;
use ReflectionClass;
use ReflectionException;
use UnexpectedValueException;

/**
* A callable decoder that uses Firebase JWT implementation.
*
* Notes:
* firebase/php-jwt is a peer dependency, you need to install it separately:
* `composer require firebase/php-jwt:"^5.5"`
* This decoder works with all v5.* branches of firebase/php-jwt,
* but we recommend using version "^5.5" to mitigate a possible security issue CVE-2021-46743.
*
* Alternatively, update dakujem/auth-middleware to v2 and firebase/php-jwt to v6 to prevent the issue completely:
* `composer require dakujem/auth-middleware:"^2" firebase/php-jwt:"^6.0"`
* Note:
* firebase/php-jwt is a peer dependency, it needs to be installed separately:
* `composer require firebase/php-jwt:"^6.0 | ^5.5"`
*
* Usage with TokenMiddleware:
* $mw = new TokenMiddleware(new FirebaseJwtDecoder(new Secret('my-secret-is-not-committed-to-the-repo')));
* $mw = new TokenMiddleware(new FirebaseJwtDecoder(new Secret('my-secret-is-not-committed-to-the-repo', 'HS256')));
*
* Warning:
* This decoder _only_ ensures that the token has been signed by the given secret key
* This decoder ensures _only_ that the token has been signed by the given secret key
* and that it is not expired (`exp` claim) or used before intended (`nbf` and `iat` claims).
* The rest of the authorization process is entirely up to your app's logic.
*
* @author Andrej Rypak <[email protected]>
*/
final class FirebaseJwtDecoder
{
/** @var Key|Key[]|string */
private $secret;
private ?array $algos;
/** @var Key|Key[] */
private Key|array $keys;

public function __construct($secret, ?array $algos = null)
public function __construct(SecretContract ...$secret)
{
if (empty($secret)) {
throw new InvalidArgumentException('Invalid configuration: The secret key may not be empty.');
if (!class_exists(Key::class)) {
throw new LogicException('Peer dependency version mismatch. Please upgrade the `firebase/php-jwt` package.');
}
$algos ??= ['HS256', 'HS512', 'HS384'];

if (count($algos) === 0) {
throw new InvalidArgumentException('Invalid configuration: No encryption algorithms provided.');
if (count($secret) === 0) {
throw new InvalidArgumentException('No keys passed to the decoder.');
}

if (!is_string($secret) && !class_exists(Key::class)) {
throw new UnexpectedValueException(
'Unsupported configuration. To use the `Secret` objects, upgrade peer library `firebase/php-jwt` to version 5.5 or 6 and above.'
);
}
if (!is_string($secret)) {
$key = fn(SecretContract $s): Key => new Key($s->keyMaterial(), $s->algorithm());
if (is_array($secret) && count($secret) === 1) {
$secret = array_pop($secret);
}
if ($secret instanceof SecretContract) {
$this->secret = $key($secret);
} elseif (is_array($secret)) {
$this->secret = array_map($key, $secret);
} else {
throw new UnexpectedValueException(
'Invalid configuration: The secret must ether be a string, a `SecretContract` object or an array of such objects.'
);
}
$key = fn(SecretContract $s): Key => new Key($s->keyMaterial(), $s->algorithm());
if (count($secret) === 1) {
$this->keys = $key(array_pop($secret));
} else {
$this->secret = $secret;
}

// In certain configurations, the decoding will fail. To prevent the failure, we throw an exception here.
if (is_string($secret) && class_exists(Key::class)) {
if (count($algos) > 1) {
try {
// The following detects v6 of firebase/php-jwt lib.
if ((new ReflectionClass(JWT::class))->getMethod('decode')->getNumberOfParameters() < 3) {
//
// If this is happening to you, there are 3 options:
// 1. use a single secret+algorithm combination either using the `Secret` object or passing an array with a single algorithm to the `$algos` parameter
// 2. use multiple `Secret` objects and pass them to the `$secret` parameter AND use "kid" JWT header parameter when encoding the JWT
// 3. downgrade firebase/php-jwt to version v5.5 or below (not recommended)
//
// This is done to mitigate a possible security issue CVE-2021-46743.
// For more details, see https://github.com/firebase/php-jwt/issues/351.
//
throw new UnexpectedValueException(
'Peer library `firebase/php-jwt` has been updated to version v6 or above, which does not work with the current secret+algorithm configuration combination. Refer to the documentation od dakujem/auth-middleware for this version to solve the configuration issue.'
);
}
} catch (ReflectionException $e) {
// ignore
}
//
// If this is happening to you, there are 3 options:
// 1. use a single secret+algorithm combination either using the `Secret` object or passing an array with a single algorithm to the `$algos` parameter
// 2. use multiple `Secret` objects and pass them to the `$secret` parameter AND use "kid" JWT header parameter when encoding the JWT
// 3. ignore this warning or downgrade dakujem/auth-middleware (not recommended)
//
// This is done to mitigate a possible security issue CVE-2021-46743.
// For more details, see https://github.com/firebase/php-jwt/issues/351.
//
trigger_error(
'Peer library `firebase/php-jwt` has been updated to a version able to circumvent security vulnerability CVE-2021-46743. Please use the `Secret` objects instead of string constants: `new FirebaseJwtDecoder(new Secret($secretString, $algorithm))`.',
E_USER_WARNING,
);
}
if (count($algos) === 1) {
$algorithm = array_pop($algos);
$this->secret = new Key($secret, $algorithm);
}
$this->keys = array_map($key, $secret);
}
$this->algos = $algos;
}

/**
Expand All @@ -134,8 +65,7 @@ public function __invoke(string $token, ?LoggerInterface $logger = null): object
try {
return JWT::decode(
$token,
$this->secret,
$this->algos
$this->keys,
);
} catch (UnexpectedValueException $throwable) {
$logger && $logger->log(LogLevel::DEBUG, $throwable->getMessage(), [$token, $throwable]);
Expand Down
5 changes: 2 additions & 3 deletions src/Secret.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@
final class Secret implements SecretContract
{
private string $algorithm;

private $keyMaterial;

public function __construct($keyMaterial, string $algorithm)
public function __construct(mixed $keyMaterial, string $algorithm)
{
if (empty($keyMaterial)) {
throw new InvalidArgumentException('Type error: $keyMaterial must not be empty');
Expand All @@ -29,7 +28,7 @@ public function __construct($keyMaterial, string $algorithm)
/**
* Return the key material - the secret.
*/
public function keyMaterial()
public function keyMaterial(): mixed
{
return $this->keyMaterial;
}
Expand Down
2 changes: 1 addition & 1 deletion src/SecretContract.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ interface SecretContract
/**
* Return the key material - the secret.
*/
public function keyMaterial();
public function keyMaterial(): mixed;

/**
* Return the algorithm valid for this key.
Expand Down
7 changes: 6 additions & 1 deletion tests/FactoryTest.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,14 @@ use Tester\TestCase;
*/
class _FactoryTest extends TestCase
{
public function testWizardDefaultAlgo()
{
Assert::same('HS256', AuthWizard::$defaultAlgo);
}

public function testAuthFactoryReturnsCorrectMiddleware()
{
$f = new AuthFactory(AuthFactory::defaultDecoderFactory('whatever'), new ResponseFactory());
$f = new AuthFactory(fn() => fn() => null, new ResponseFactory());
Assert::type(TokenMiddleware::class, $f->decodeTokens());
Assert::type(GenericMiddleware::class, $f->assertTokens());
Assert::type(GenericMiddleware::class, $f->inspectTokens(fn() => null));
Expand Down
Loading

0 comments on commit 859fc8d

Please sign in to comment.