From 034917b7900c77d0e54c3394c6bfb7839aee827a Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Thu, 29 Aug 2024 17:28:01 +0200 Subject: [PATCH] fix(oauth2): store hashed secret instead of encrypted Signed-off-by: Julien Veyssier --- apps/oauth2/appinfo/info.xml | 2 +- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../lib/Controller/OauthApiController.php | 5 +- .../lib/Controller/SettingsController.php | 4 +- .../Version011901Date20240829164356.php | 49 +++++++++++ apps/oauth2/lib/Settings/Admin.php | 5 +- apps/oauth2/src/App.vue | 8 ++ apps/oauth2/src/components/OAuthItem.vue | 3 +- .../Controller/OauthApiControllerTest.php | 81 ++++++++++--------- .../Controller/SettingsControllerTest.php | 10 +-- apps/oauth2/tests/Settings/AdminTest.php | 4 +- 12 files changed, 118 insertions(+), 55 deletions(-) create mode 100644 apps/oauth2/lib/Migration/Version011901Date20240829164356.php diff --git a/apps/oauth2/appinfo/info.xml b/apps/oauth2/appinfo/info.xml index f4eede380b188..b6f14720de1bb 100644 --- a/apps/oauth2/appinfo/info.xml +++ b/apps/oauth2/appinfo/info.xml @@ -9,7 +9,7 @@ OAuth 2.0 Allows OAuth2 compatible authentication from other web applications. The OAuth2 app allows administrators to configure the built-in authentication workflow to also allow OAuth2 compatible authentication from other web applications. - 1.19.0 + 1.19.1 agpl Lukas Reschke OAuth2 diff --git a/apps/oauth2/composer/composer/autoload_classmap.php b/apps/oauth2/composer/composer/autoload_classmap.php index 536342c075113..cba0ef43dfc2a 100644 --- a/apps/oauth2/composer/composer/autoload_classmap.php +++ b/apps/oauth2/composer/composer/autoload_classmap.php @@ -23,5 +23,6 @@ 'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => $baseDir . '/../lib/Migration/Version011601Date20230522143227.php', 'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => $baseDir . '/../lib/Migration/Version011602Date20230613160650.php', 'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => $baseDir . '/../lib/Migration/Version011603Date20230620111039.php', + 'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => $baseDir . '/../lib/Migration/Version011901Date20240829164356.php', 'OCA\\OAuth2\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php', ); diff --git a/apps/oauth2/composer/composer/autoload_static.php b/apps/oauth2/composer/composer/autoload_static.php index 1dc659778a3d3..c4905bc96c477 100644 --- a/apps/oauth2/composer/composer/autoload_static.php +++ b/apps/oauth2/composer/composer/autoload_static.php @@ -38,6 +38,7 @@ class ComposerStaticInitOAuth2 'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => __DIR__ . '/..' . '/../lib/Migration/Version011601Date20230522143227.php', 'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => __DIR__ . '/..' . '/../lib/Migration/Version011602Date20230613160650.php', 'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => __DIR__ . '/..' . '/../lib/Migration/Version011603Date20230620111039.php', + 'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => __DIR__ . '/..' . '/../lib/Migration/Version011901Date20240829164356.php', 'OCA\\OAuth2\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php', ); diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index ec54e86731450..d763779053a35 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -138,7 +138,8 @@ public function getToken( } try { - $storedClientSecret = $this->crypto->decrypt($client->getSecret()); + $storedClientSecretHash = $client->getSecret(); + $clientSecretHash = bin2hex($this->crypto->calculateHMAC($client_secret)); } catch (\Exception $e) { $this->logger->error('OAuth client secret decryption error', ['exception' => $e]); // we don't throttle here because it might not be a bruteforce attack @@ -147,7 +148,7 @@ public function getToken( ], Http::STATUS_BAD_REQUEST); } // The client id and secret must match. Else we don't provide an access token! - if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) { + if ($client->getClientIdentifier() !== $client_id || $storedClientSecretHash !== $clientSecretHash) { $response = new JSONResponse([ 'error' => 'invalid_client', ], Http::STATUS_BAD_REQUEST); diff --git a/apps/oauth2/lib/Controller/SettingsController.php b/apps/oauth2/lib/Controller/SettingsController.php index ce85598d08d04..f16b26696c4ab 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -50,8 +50,8 @@ public function addClient(string $name, $client->setName($name); $client->setRedirectUri($redirectUri); $secret = $this->secureRandom->generate(64, self::validChars); - $encryptedSecret = $this->crypto->encrypt($secret); - $client->setSecret($encryptedSecret); + $hashedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + $client->setSecret($hashedSecret); $client->setClientIdentifier($this->secureRandom->generate(64, self::validChars)); $client = $this->clientMapper->insert($client); diff --git a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php new file mode 100644 index 0000000000000..9ce9ff371cbde --- /dev/null +++ b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php @@ -0,0 +1,49 @@ +connection->getQueryBuilder(); + $qbUpdate->update('oauth2_clients') + ->set('secret', $qbUpdate->createParameter('updateSecret')) + ->where( + $qbUpdate->expr()->eq('id', $qbUpdate->createParameter('updateId')) + ); + + $qbSelect = $this->connection->getQueryBuilder(); + $qbSelect->select('id', 'secret') + ->from('oauth2_clients'); + $req = $qbSelect->executeQuery(); + while ($row = $req->fetch()) { + $id = $row['id']; + $storedEncryptedSecret = $row['secret']; + $secret = $this->crypto->decrypt($storedEncryptedSecret); + $hashedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + $qbUpdate->setParameter('updateSecret', $hashedSecret, IQueryBuilder::PARAM_STR); + $qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT); + $qbUpdate->executeStatement(); + } + $req->closeCursor(); + } +} diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php index eae10098a5964..93b6b7bcc3fda 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -12,7 +12,6 @@ use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Services\IInitialState; use OCP\IURLGenerator; -use OCP\Security\ICrypto; use OCP\Settings\ISettings; use Psr\Log\LoggerInterface; @@ -22,7 +21,6 @@ public function __construct( private IInitialState $initialState, private ClientMapper $clientMapper, private IURLGenerator $urlGenerator, - private ICrypto $crypto, private LoggerInterface $logger, ) { } @@ -33,13 +31,12 @@ public function getForm(): TemplateResponse { foreach ($clients as $client) { try { - $secret = $this->crypto->decrypt($client->getSecret()); $result[] = [ 'id' => $client->getId(), 'name' => $client->getName(), 'redirectUri' => $client->getRedirectUri(), 'clientId' => $client->getClientIdentifier(), - 'clientSecret' => $secret, + 'clientSecret' => '', ]; } catch (\Exception $e) { $this->logger->error('[Settings] OAuth client secret decryption error', ['exception' => $e]); diff --git a/apps/oauth2/src/App.vue b/apps/oauth2/src/App.vue index 5498d7ec0713a..255da9a094341 100644 --- a/apps/oauth2/src/App.vue +++ b/apps/oauth2/src/App.vue @@ -33,6 +33,10 @@ @delete="deleteClient" /> + + {{ t('oauth2', 'Make sure you store the secret key, it cannot be recovered.') }} +

{{ t('oauth2', 'Add client') }}

@@ -66,6 +70,7 @@ import { generateUrl } from '@nextcloud/router' import { getCapabilities } from '@nextcloud/capabilities' import NcSettingsSection from '@nextcloud/vue/dist/Components/NcSettingsSection.js' import NcButton from '@nextcloud/vue/dist/Components/NcButton.js' +import NcNoteCard from '@nextcloud/vue/dist/Components/NcNoteCard.js' import { loadState } from '@nextcloud/initial-state' import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js' @@ -76,6 +81,7 @@ export default { NcSettingsSection, NcButton, NcTextField, + NcNoteCard, }, props: { clients: { @@ -92,6 +98,7 @@ export default { error: false, }, oauthDocLink: loadState('oauth2', 'oauth2-doc-link'), + showSecretWarning: false, } }, computed: { @@ -119,6 +126,7 @@ export default { ).then(response => { // eslint-disable-next-line vue/no-mutating-props this.clients.push(response.data) + this.showSecretWarning = true this.newClient.name = '' this.newClient.redirectUri = '' diff --git a/apps/oauth2/src/components/OAuthItem.vue b/apps/oauth2/src/components/OAuthItem.vue index c22a2f73d747e..567cf88b63c94 100644 --- a/apps/oauth2/src/components/OAuthItem.vue +++ b/apps/oauth2/src/components/OAuthItem.vue @@ -10,7 +10,8 @@
{{ renderedSecret }} -