Merge pull request #713 from nextcloud/fix/709/require-jwk-alg

Fix potentially missing "alg" in jwks
nextcloud · Nov 22, 2023 · 173067f · 173067f
2 parents 9f591c6 + 6406891
commit 173067f
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 8 deletions.
diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php
@@ -414,7 +414,7 @@ public function code(string $state = '', string $code = '', string $scope = '',
 
 		// TODO: proper error handling
 		$idTokenRaw = $data['id_token'];
-		$jwks = $this->discoveryService->obtainJWK($provider);
+		$jwks = $this->discoveryService->obtainJWK($provider, $idTokenRaw);
 		JWT::$leeway = 60;
 		$idTokenPayload = JWT::decode($idTokenRaw, $jwks);
 
@@ -615,7 +615,7 @@ public function backChannelLogout(string $providerIdentifier, string $logout_tok
 		}
 
 		// decrypt the logout token
-		$jwks = $this->discoveryService->obtainJWK($provider);
+		$jwks = $this->discoveryService->obtainJWK($provider, $logout_token);
 		JWT::$leeway = 60;
 		$logoutTokenPayload = JWT::decode($logout_token, $jwks);
 

diff --git a/lib/Service/DiscoveryService.php b/lib/Service/DiscoveryService.php
@@ -27,6 +27,7 @@
 
 use OCA\UserOIDC\Db\Provider;
 use OCA\UserOIDC\Vendor\Firebase\JWT\JWK;
+use OCA\UserOIDC\Vendor\Firebase\JWT\JWT;
 use OCP\Http\Client\IClientService;
 use OCP\ICache;
 use OCP\ICacheFactory;
@@ -36,6 +37,20 @@ class DiscoveryService {
 	public const INVALIDATE_DISCOVERY_CACHE_AFTER_SECONDS = 3600;
 	public const INVALIDATE_JWKS_CACHE_AFTER_SECONDS = 3600;
 
+	/**
+	 *
+	 * See https://www.imsglobal.org/spec/security/v1p1#approved-jwt-signing-algorithms.
+	 * @var string[]
+	 */
+	private const SUPPORTED_JWK_ALGS = [
+		'RS256' => 'RSA',
+		'RS384' => 'RSA',
+		'RS512' => 'RSA',
+		'ES256' => 'EC',
+		'ES384' => 'EC',
+		'ES512' => 'EC'
+	];
+
 	/** @var LoggerInterface */
 	private $logger;
 
@@ -71,23 +86,29 @@ public function obtainDiscovery(Provider $provider): array {
 		return json_decode($cachedDiscovery, true, 512, JSON_THROW_ON_ERROR);
 	}
 
-	public function obtainJWK(Provider $provider): array {
+	/**
+	 * @param Provider $provider
+	 * @param string $tokenToDecode This is used to potentially fix the missing alg in
+	 * @return array
+	 * @throws \JsonException
+	 */
+	public function obtainJWK(Provider $provider, string $tokenToDecode): array {
 		$lastJwksRefresh = $this->providerService->getSetting($provider->getId(), ProviderService::SETTING_JWKS_CACHE_TIMESTAMP);
 		if ($lastJwksRefresh !== '' && (int) $lastJwksRefresh > time() - self::INVALIDATE_JWKS_CACHE_AFTER_SECONDS) {
 			$rawJwks = $this->providerService->getSetting($provider->getId(), ProviderService::SETTING_JWKS_CACHE);
 			$rawJwks = json_decode($rawJwks, true);
-			$jwks = JWK::parseKeySet($rawJwks);
 		} else {
 			$discovery = $this->obtainDiscovery($provider);
 			$client = $this->clientService->newClient();
 			$responseBody = $client->get($discovery['jwks_uri'])->getBody();
-			$result = json_decode($responseBody, true);
-			$jwks = JWK::parseKeySet($result);
+			$rawJwks = json_decode($responseBody, true);
 			// cache jwks
 			$this->providerService->setSetting($provider->getId(), ProviderService::SETTING_JWKS_CACHE, $responseBody);
 			$this->providerService->setSetting($provider->getId(), ProviderService::SETTING_JWKS_CACHE_TIMESTAMP, strval(time()));
 		}
 
+		$fixedJwks = $this->fixJwksAlg($rawJwks, $tokenToDecode);
+		$jwks = JWK::parseKeySet($fixedJwks, 'RS256');
 		$this->logger->debug('Parsed the jwks');
 		return $jwks;
 	}
@@ -117,4 +138,41 @@ public function buildAuthorizationUrl(string $authorizationEndpoint, array $extr
 		return htmlentities(filter_var($urlWithoutParams, FILTER_SANITIZE_URL), ENT_QUOTES)
 			. (empty($queryParams) ? '' : '?' . http_build_query($queryParams));
 	}
+
+	/**
+	 * Inspired by https://github.com/snake/moodle/compare/880462a1685...MDL-77077-master
+	 *
+	 * @param array $jwks
+	 * @param string $jwt
+	 * @return array
+	 * @throws \Exception
+	 */
+	private function fixJwksAlg(array $jwks, string $jwt): array {
+		$jwtParts = explode('.', $jwt);
+		$jwtHeader = json_decode(JWT::urlsafeB64Decode($jwtParts[0]), true);
+		if (!isset($jwtHeader['kid'])) {
+			throw new \Exception('Error: kid must be provided in JWT header.');
+		}
+
+		foreach ($jwks['keys'] as $index => $key) {
+			// Only fix the key being referred to in the JWT.
+			if ($jwtHeader['kid'] != $key['kid']) {
+				continue;
+			}
+
+			// Only fix the key if the alg is missing.
+			if (!empty($key['alg'])) {
+				continue;
+			}
+
+			// The header alg must match the key type (family) specified in the JWK's kty.
+			if (!isset(self::SUPPORTED_JWK_ALGS[$jwtHeader['alg']]) || self::SUPPORTED_JWK_ALGS[$jwtHeader['alg']] !== $key['kty']) {
+				throw new \Exception('Error: Alg specified in the JWT header is incompatible with the JWK key type');
+			}
+
+			$jwks['keys'][$index]['alg'] = $jwtHeader['alg'];
+		}
+
+		return $jwks;
+	}
 }
diff --git a/lib/User/Provisioning/SelfEncodedTokenProvisioning.php b/lib/User/Provisioning/SelfEncodedTokenProvisioning.php
@@ -30,7 +30,8 @@ public function __construct(ProvisioningService $provisioningService, DiscoveryS
 	public function provisionUser(Provider $provider, string $tokenUserId, string $bearerToken): ?IUser {
 		JWT::$leeway = 60;
 		try {
-			$payload = JWT::decode($bearerToken, $this->discoveryService->obtainJWK($provider));
+			$jwks = $this->discoveryService->obtainJWK($provider, $bearerToken);
+			$payload = JWT::decode($bearerToken, $jwks);
 		} catch (Throwable $e) {
 			$this->logger->error('Impossible to decode OIDC token:' . $e->getMessage());
 			return null;

diff --git a/lib/User/Validator/SelfEncodedValidator.php b/lib/User/Validator/SelfEncodedValidator.php
@@ -57,7 +57,8 @@ public function isValidBearerToken(Provider $provider, string $bearerToken): ?st
 		// try to decode the bearer token
 		JWT::$leeway = 60;
 		try {
-			$payload = JWT::decode($bearerToken, $this->discoveryService->obtainJWK($provider));
+			$jwks = $this->discoveryService->obtainJWK($provider, $bearerToken);
+			$payload = JWT::decode($bearerToken, $jwks);
 		} catch (Throwable $e) {
 			$this->logger->error('Impossible to decode OIDC token:' . $e->getMessage());
 			return null;