From f226974c508b6870e836e09072ed0a9878211961 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 fd17afbb11f..65e0886c68a 100644 --- a/apps/oauth2/appinfo/info.xml +++ b/apps/oauth2/appinfo/info.xml @@ -5,7 +5,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.16.3 + 1.16.4 agpl Lukas Reschke OAuth2 diff --git a/apps/oauth2/composer/composer/autoload_classmap.php b/apps/oauth2/composer/composer/autoload_classmap.php index 536342c0751..cba0ef43dfc 100644 --- a/apps/oauth2/composer/composer/autoload_classmap.php +++ b/apps/oauth2/composer/composer/autoload_classmap.php @@ -23,5 +23,6 @@ return array( '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 1dc659778a3..c4905bc96c4 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 46b68b1d585..0ce607edb5c 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -156,7 +156,8 @@ class OauthApiController extends Controller { } 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 @@ -165,7 +166,7 @@ class OauthApiController extends Controller { ], 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 d49dd6da038..5d00e233828 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -72,8 +72,8 @@ class SettingsController extends Controller { $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 00000000000..9ce9ff371cb --- /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 5cba6805f7c..a44b0ff05b5 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -30,7 +30,6 @@ use OCA\OAuth2\Db\ClientMapper; 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; @@ -40,7 +39,6 @@ class Admin implements ISettings { private IInitialState $initialState, private ClientMapper $clientMapper, private IURLGenerator $urlGenerator, - private ICrypto $crypto, private LoggerInterface $logger, ) { } @@ -51,13 +49,12 @@ class Admin implements ISettings { 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 71cf22dbf89..411643168f4 100644 --- a/apps/oauth2/src/App.vue +++ b/apps/oauth2/src/App.vue @@ -50,6 +50,10 @@ @delete="deleteClient" /> + + {{ t('oauth2', 'Make sure you store the secret key, it cannot be recovered.') }} +

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

@@ -83,6 +87,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' @@ -93,6 +98,7 @@ export default { NcSettingsSection, NcButton, NcTextField, + NcNoteCard, }, props: { clients: { @@ -109,6 +115,7 @@ export default { error: false, }, oauthDocLink: loadState('oauth2', 'oauth2-doc-link'), + showSecretWarning: false, } }, computed: { @@ -136,6 +143,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 2a2eced7701..4fc0d63b8e7 100644 --- a/apps/oauth2/src/components/OAuthItem.vue +++ b/apps/oauth2/src/components/OAuthItem.vue @@ -27,7 +27,8 @@
{{ renderedSecret }} -