From cdf3b60555eb559ea5f9b141903054afbc273062 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 5 Jul 2022 11:25:44 +0200 Subject: [PATCH 1/3] Handle one time passwords This adds an option to disable storing passwords in the database. This might be desirable when using single use token as passwords or very large passwords. Signed-off-by: Carl Schwan --- config/config.sample.php | 15 +++++ .../Token/PublicKeyTokenProvider.php | 2 +- .../Token/PublicKeyTokenProviderTest.php | 60 +++++++++++++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index 9856aeba4d7..025cf1105a0 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -308,6 +308,21 @@ $CONFIG = [ */ 'auth.webauthn.enabled' => true, +/** + * Whether encrypted password should be stored in the database + * + * The passwords are only decrypted using the login token stored uniquely in the + * clients and allow to connect to external storages, autoconfigure mail account in + * the mail app and periodically check if the password it still valid. + * + * This might be desirable to disable this functionality when using one time + * passwords or when having a password policy enforcing long passwords (> 300 + * characters). + * + * By default the passwords are stored encrypted in the database. + */ +'auth.storeCryptedPassword' => true, + /** * By default the login form is always available. There are cases (SSO) where an * admin wants to avoid users entering their credentials to the system if the SSO diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index a1d75828e27..16425a2e1c3 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -368,7 +368,7 @@ class PublicKeyTokenProvider implements IProvider { $dbToken->setPublicKey($publicKey); $dbToken->setPrivateKey($this->encrypt($privateKey, $token)); - if (!is_null($password)) { + if (!is_null($password) && $this->config->getSystemValueBool('auth.storeCryptedPassword', true)) { $dbToken->setPassword($this->encryptPassword($password, $publicKey)); } diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php index 6ad57515c16..8e6f699f0b8 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php @@ -25,6 +25,7 @@ namespace Test\Authentication\Token; use OC\Authentication\Exceptions\ExpiredTokenException; use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Token\IToken; use OC\Authentication\Token\PublicKeyToken; use OC\Authentication\Token\PublicKeyTokenMapper; @@ -83,6 +84,10 @@ class PublicKeyTokenProviderTest extends TestCase { $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); $this->assertInstanceOf(PublicKeyToken::class, $actual); @@ -93,6 +98,29 @@ class PublicKeyTokenProviderTest extends TestCase { $this->assertSame($password, $this->tokenProvider->getPassword($actual, $token)); } + public function testGenerateTokenNoPassword() { + $token = 'token'; + $uid = 'user'; + $user = 'User'; + $password = 'passme'; + $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, false], + ]); + $this->expectException(PasswordlessTokenException::class); + + $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); + + $this->assertInstanceOf(PublicKeyToken::class, $actual); + $this->assertSame($uid, $actual->getUID()); + $this->assertSame($user, $actual->getLoginName()); + $this->assertSame($name, $actual->getName()); + $this->assertSame(IToken::DO_NOT_REMEMBER, $actual->getRemember()); + $this->tokenProvider->getPassword($actual, $token); + } + public function testGenerateTokenInvalidName() { $token = 'token'; $uid = 'user'; @@ -103,6 +131,10 @@ class PublicKeyTokenProviderTest extends TestCase { . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12' . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -157,6 +189,10 @@ class PublicKeyTokenProviderTest extends TestCase { $password = 'passme'; $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -185,6 +221,10 @@ class PublicKeyTokenProviderTest extends TestCase { $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); $this->tokenProvider->getPassword($actual, 'wrongtoken'); @@ -197,6 +237,10 @@ class PublicKeyTokenProviderTest extends TestCase { $password = 'passme'; $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -301,7 +345,7 @@ class PublicKeyTokenProviderTest extends TestCase { $this->tokenProvider->renewSessionToken('oldId', 'newId'); } - public function testRenewSessionTokenWithPassword() { + public function testRenewSessionTokenWithPassword(): void { $token = 'oldId'; $uid = 'user'; $user = 'User'; @@ -309,6 +353,10 @@ class PublicKeyTokenProviderTest extends TestCase { $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); $oldToken = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); $this->mapper @@ -319,7 +367,7 @@ class PublicKeyTokenProviderTest extends TestCase { $this->mapper ->expects($this->once()) ->method('insert') - ->with($this->callback(function (PublicKeyToken $token) use ($user, $uid, $name) { + ->with($this->callback(function (PublicKeyToken $token) use ($user, $uid, $name): bool { return $token->getUID() === $uid && $token->getLoginName() === $user && $token->getName() === $name && @@ -331,14 +379,14 @@ class PublicKeyTokenProviderTest extends TestCase { $this->mapper ->expects($this->once()) ->method('delete') - ->with($this->callback(function ($token) use ($oldToken) { + ->with($this->callback(function ($token) use ($oldToken): bool { return $token === $oldToken; })); $this->tokenProvider->renewSessionToken('oldId', 'newId'); } - public function testGetToken() { + public function testGetToken(): void { $token = new PublicKeyToken(); $this->config->method('getSystemValue') @@ -441,6 +489,10 @@ class PublicKeyTokenProviderTest extends TestCase { $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); $new = $this->tokenProvider->rotate($actual, 'oldtoken', 'newtoken'); From 1c23c029af1ef83935badb8b63cb4dffac59b1e4 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 5 Jul 2022 11:37:14 +0200 Subject: [PATCH 2/3] Handler large passwords For passwords bigger than 250 characters, use a bigger key since the performance impact is minor (around one second to encrypt the password). For passwords bigger than 470 characters, give up earlier and throw exeception recommanding admin to either enable the previously enabled configuration or use smaller passwords. Signed-off-by: Carl Schwan --- .../Token/PublicKeyTokenProvider.php | 5 ++++- .../Token/PublicKeyTokenProviderTest.php | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index 16425a2e1c3..96bf9a86087 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -346,7 +346,7 @@ class PublicKeyTokenProvider implements IProvider { $config = array_merge([ 'digest_alg' => 'sha512', - 'private_key_bits' => 2048, + 'private_key_bits' => $password !== null && strlen($password) > 250 ? 4096 : 2048, ], $this->config->getSystemValue('openssl', [])); // Generate new key @@ -369,6 +369,9 @@ class PublicKeyTokenProvider implements IProvider { $dbToken->setPrivateKey($this->encrypt($privateKey, $token)); if (!is_null($password) && $this->config->getSystemValueBool('auth.storeCryptedPassword', true)) { + if (strlen($password) > 469) { + throw new \RuntimeException('Trying to save a password with more than 469 characters is not supported. If you want to use big passwords, disable the auth.storeCryptedPassword option in config.php'); + } $dbToken->setPassword($this->encryptPassword($password, $publicKey)); } diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php index 8e6f699f0b8..db61244db5b 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php @@ -121,6 +121,25 @@ class PublicKeyTokenProviderTest extends TestCase { $this->tokenProvider->getPassword($actual, $token); } + public function testGenerateTokenLongPassword() { + $token = 'token'; + $uid = 'user'; + $user = 'User'; + $password = ''; + for ($i = 0; $i < 500; $i++) { + $password .= 'e'; + } + $name = 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $type = IToken::PERMANENT_TOKEN; + $this->config->method('getSystemValueBool') + ->willReturnMap([ + ['auth.storeCryptedPassword', true, true], + ]); + $this->expectException(\RuntimeException::class); + + $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); + } + public function testGenerateTokenInvalidName() { $token = 'token'; $uid = 'user'; From f99a06c89a116cbc447b5fb5d2ec27462b9fba51 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 5 Jul 2022 11:47:25 +0200 Subject: [PATCH 3/3] Don't allow setting password bigger than 469 characters Signed-off-by: Carl Schwan --- .../lib/Controller/ChangePasswordController.php | 11 ++++++++++- apps/settings/src/components/UserList/UserRow.vue | 1 + .../templates/settings/personal/security/password.php | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/apps/settings/lib/Controller/ChangePasswordController.php b/apps/settings/lib/Controller/ChangePasswordController.php index 7c3ab9546bc..41f2584721c 100644 --- a/apps/settings/lib/Controller/ChangePasswordController.php +++ b/apps/settings/lib/Controller/ChangePasswordController.php @@ -107,7 +107,7 @@ class ChangePasswordController extends Controller { } try { - if ($newpassword === null || $user->setPassword($newpassword) === false) { + if ($newpassword === null || strlen($newpassword) > 469 || $user->setPassword($newpassword) === false) { return new JSONResponse([ 'status' => 'error', 'data' => [ @@ -158,6 +158,15 @@ class ChangePasswordController extends Controller { ]); } + if (strlen($password) > 469) { + return new JSONResponse([ + 'status' => 'error', + 'data' => [ + 'message' => $this->l->t('Unable to change password. Password too long.'), + ], + ]); + } + $currentUser = $this->userSession->getUser(); $targetUser = $this->userManager->get($username); if ($currentUser === null || $targetUser === null || diff --git a/apps/settings/src/components/UserList/UserRow.vue b/apps/settings/src/components/UserList/UserRow.vue index de0a09f2221..f2947019f40 100644 --- a/apps/settings/src/components/UserList/UserRow.vue +++ b/apps/settings/src/components/UserList/UserRow.vue @@ -107,6 +107,7 @@ ref="password" :disabled="loading.password || loading.all" :minlength="minPasswordLength" + maxlength="469" :placeholder="t('settings', 'Add new password')" autocapitalize="off" autocomplete="new-password" diff --git a/apps/settings/templates/settings/personal/security/password.php b/apps/settings/templates/settings/personal/security/password.php index 88536ab6b23..85959e252cc 100644 --- a/apps/settings/templates/settings/personal/security/password.php +++ b/apps/settings/templates/settings/personal/security/password.php @@ -46,6 +46,7 @@ if ($_['passwordChangeSupported']) {