From 7efa2fa3a02167eea8f9b94d67b86d68e0b6e56a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 22 Mar 2022 10:51:54 +0100 Subject: [PATCH 1/2] Limit the length of app password names Signed-off-by: Joas Schilling --- .../lib/Controller/AuthSettingsController.php | 8 +++++ .../Authentication/Token/IProvider.php | 2 +- lib/private/Authentication/Token/Manager.php | 6 +++- .../Token/PublicKeyTokenProvider.php | 4 +++ .../lib/Authentication/Token/ManagerTest.php | 31 +++++++++++++++++++ 5 files changed, 49 insertions(+), 2 deletions(-) diff --git a/apps/settings/lib/Controller/AuthSettingsController.php b/apps/settings/lib/Controller/AuthSettingsController.php index 3255fcce56e7b..38db7be1e9187 100644 --- a/apps/settings/lib/Controller/AuthSettingsController.php +++ b/apps/settings/lib/Controller/AuthSettingsController.php @@ -145,6 +145,10 @@ public function create($name) { return $this->getServiceNotAvailableResponse(); } + if (mb_strlen($name) > 128) { + $name = mb_substr($name, 0, 120) . '…'; + } + $token = $this->generateRandomDeviceToken(); $deviceToken = $this->tokenProvider->generateToken($token, $this->uid, $loginName, $password, $name, IToken::PERMANENT_TOKEN); $tokenData = $deviceToken->jsonSerialize(); @@ -241,6 +245,10 @@ public function update($id, array $scope, string $name) { $this->publishActivity($scope['filesystem'] ? Provider::APP_TOKEN_FILESYSTEM_GRANTED : Provider::APP_TOKEN_FILESYSTEM_REVOKED, $token->getId(), ['name' => $currentName]); } + if (mb_strlen($name) > 128) { + $name = mb_substr($name, 0, 120) . '…'; + } + if ($token instanceof INamedToken && $name !== $currentName) { $token->setName($name); $this->publishActivity(Provider::APP_TOKEN_RENAMED, $token->getId(), ['name' => $currentName, 'newName' => $name]); diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index e604ac715c24c..0a145bfd7e63d 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -44,7 +44,7 @@ interface IProvider { * @param string $uid * @param string $loginName * @param string|null $password - * @param string $name + * @param string $name Name will be trimmed to 120 chars when longer * @param int $type token type * @param int $remember whether the session token should be used for remember-me * @return IToken diff --git a/lib/private/Authentication/Token/Manager.php b/lib/private/Authentication/Token/Manager.php index b718ce73ea478..cadc5f408e4bc 100644 --- a/lib/private/Authentication/Token/Manager.php +++ b/lib/private/Authentication/Token/Manager.php @@ -53,7 +53,7 @@ public function __construct(DefaultTokenProvider $defaultTokenProvider, PublicKe * @param string $uid * @param string $loginName * @param string|null $password - * @param string $name + * @param string $name Name will be trimmed to 120 chars when longer * @param int $type token type * @param int $remember whether the session token should be used for remember-me * @return IToken @@ -65,6 +65,10 @@ public function generateToken(string $token, string $name, int $type = IToken::TEMPORARY_TOKEN, int $remember = IToken::DO_NOT_REMEMBER): IToken { + if (mb_strlen($name) > 128) { + $name = mb_substr($name, 0, 120) . '…'; + } + try { return $this->publicKeyTokenProvider->generateToken( $token, diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index b7c8a8e9c2463..ddf477b346323 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -84,6 +84,10 @@ public function generateToken(string $token, string $name, int $type = IToken::TEMPORARY_TOKEN, int $remember = IToken::DO_NOT_REMEMBER): IToken { + if (mb_strlen($name) > 128) { + throw new InvalidTokenException('The given name is too long'); + } + $dbToken = $this->newToken($token, $uid, $loginName, $password, $name, $type, $remember); $this->mapper->insert($dbToken); diff --git a/tests/lib/Authentication/Token/ManagerTest.php b/tests/lib/Authentication/Token/ManagerTest.php index fb92b3e50186a..ee2b3cdc76809 100644 --- a/tests/lib/Authentication/Token/ManagerTest.php +++ b/tests/lib/Authentication/Token/ManagerTest.php @@ -127,6 +127,37 @@ public function testGenerateConflictingToken() { $this->assertSame($token, $actual); } + public function testGenerateTokenTooLongName() { + $token = $this->createMock(IToken::class); + $token->method('getName') + ->willReturn(str_repeat('a', 120) . '…'); + + + $this->publicKeyTokenProvider->expects($this->once()) + ->method('generateToken') + ->with( + 'token', + 'uid', + 'loginName', + 'password', + str_repeat('a', 120) . '…', + IToken::TEMPORARY_TOKEN, + IToken::REMEMBER + )->willReturn($token); + + $actual = $this->manager->generateToken( + 'token', + 'uid', + 'loginName', + 'password', + str_repeat('a', 200), + IToken::TEMPORARY_TOKEN, + IToken::REMEMBER + ); + + $this->assertSame(121, mb_strlen($actual->getName())); + } + public function tokenData(): array { return [ [new DefaultToken()], From 3096179190caab8f6a871a8a9e4d2e139558f500 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 23 Mar 2022 11:00:34 +0100 Subject: [PATCH 2/2] Fix unit tests Signed-off-by: Joas Schilling --- .../Token/PublicKeyTokenProviderTest.php | 83 ++++++++----------- .../TwoFactorAuth/ManagerTest.php | 48 +++++------ 2 files changed, 58 insertions(+), 73 deletions(-) diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php index 486660f17c65f..767e7897c588d 100644 --- a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php +++ b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php @@ -82,10 +82,7 @@ public function testGenerateToken() { $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -98,6 +95,22 @@ public function testGenerateToken() { $this->assertSame($password, $this->tokenProvider->getPassword($actual, $token)); } + public function testGenerateTokenInvalidName() { + $this->expectException(\OC\Authentication\Exceptions\InvalidTokenException::class); + + $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' + . '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' + . '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; + + $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); + } + public function testUpdateToken() { $tk = new PublicKeyToken(); $this->mapper->expects($this->once()) @@ -139,10 +152,7 @@ public function testGetPassword() { $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -169,10 +179,7 @@ public function testGetPasswordInvalidToken() { $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -185,10 +192,7 @@ public function testSetPassword() { $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -248,12 +252,12 @@ public function testInvalidateOldTokens() { ['session_lifetime', $defaultSessionLifetime, 150], ['remember_login_cookie_lifetime', $defaultRememberMeLifetime, 300], ]); - $this->mapper->expects($this->at(0)) - ->method('invalidateOld') - ->with($this->time - 150); - $this->mapper->expects($this->at(1)) + $this->mapper->expects($this->exactly(2)) ->method('invalidateOld') - ->with($this->time - 300); + ->withConsecutive( + [$this->time - 150], + [$this->time - 300] + ); $this->tokenProvider->invalidateOldTokens(); } @@ -263,21 +267,18 @@ public function testRenewSessionTokenWithoutPassword() { $uid = 'user'; $user = 'User'; $password = null; - $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $oldToken = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); $this->mapper - ->expects($this->at(0)) + ->expects($this->once()) ->method('getToken') ->with(hash('sha512', 'oldId' . '1f4h9s')) ->willReturn($oldToken); $this->mapper - ->expects($this->at(1)) + ->expects($this->once()) ->method('insert') ->with($this->callback(function (PublicKeyToken $token) use ($user, $uid, $name) { return $token->getUID() === $uid && @@ -288,7 +289,7 @@ public function testRenewSessionTokenWithoutPassword() { $token->getPassword() === null; })); $this->mapper - ->expects($this->at(2)) + ->expects($this->once()) ->method('delete') ->with($this->callback(function ($token) use ($oldToken) { return $token === $oldToken; @@ -302,21 +303,18 @@ public function testRenewSessionTokenWithPassword() { $uid = 'user'; $user = 'User'; $password = 'password'; - $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $oldToken = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); $this->mapper - ->expects($this->at(0)) + ->expects($this->once()) ->method('getToken') ->with(hash('sha512', 'oldId' . '1f4h9s')) ->willReturn($oldToken); $this->mapper - ->expects($this->at(1)) + ->expects($this->once()) ->method('insert') ->with($this->callback(function (PublicKeyToken $token) use ($user, $uid, $name) { return $token->getUID() === $uid && @@ -328,7 +326,7 @@ public function testRenewSessionTokenWithPassword() { $this->tokenProvider->getPassword($token, 'newId') === 'password'; })); $this->mapper - ->expects($this->at(2)) + ->expects($this->once()) ->method('delete') ->with($this->callback(function ($token) use ($oldToken) { return $token === $oldToken; @@ -372,10 +370,7 @@ public function testGetExpiredToken() { $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -440,10 +435,7 @@ public function testRotate() { $uid = 'user'; $user = 'User'; $password = 'password'; - $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); @@ -458,10 +450,7 @@ public function testRotateNoPassword() { $uid = 'user'; $user = 'User'; $password = null; - $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' - . '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' - . 'User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12'; + $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; $actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER); diff --git a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php index fc921b8016b15..ae6fadc790c57 100644 --- a/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php +++ b/tests/lib/Authentication/TwoFactorAuth/ManagerTest.php @@ -376,13 +376,13 @@ public function testVerifyChallenge() { ->method('get') ->with('two_factor_remember_login') ->willReturn(false); - $this->session->expects($this->at(1)) + $this->session->expects($this->exactly(2)) ->method('remove') - ->with('two_factor_auth_uid'); - $this->session->expects($this->at(2)) - ->method('remove') - ->with('two_factor_remember_login'); - $this->session->expects($this->at(3)) + ->withConsecutive( + ['two_factor_auth_uid'], + ['two_factor_remember_login'] + ); + $this->session->expects($this->once()) ->method('set') ->with(Manager::SESSION_UID_DONE, 'jos'); $this->session->method('getId') @@ -494,17 +494,13 @@ public function testVerifyInvalidChallenge() { public function testNeedsSecondFactor() { $user = $this->createMock(IUser::class); - $this->session->expects($this->at(0)) - ->method('exists') - ->with('app_password') - ->willReturn(false); - $this->session->expects($this->at(1)) + $this->session->expects($this->exactly(3)) ->method('exists') - ->with('two_factor_auth_uid') - ->willReturn(false); - $this->session->expects($this->at(2)) - ->method('exists') - ->with(Manager::SESSION_UID_DONE) + ->withConsecutive( + ['app_password'], + ['two_factor_auth_uid'], + [Manager::SESSION_UID_DONE], + ) ->willReturn(false); $this->session->method('getId') @@ -575,12 +571,12 @@ public function testPrepareTwoFactorLogin() { $this->user->method('getUID') ->willReturn('ferdinand'); - $this->session->expects($this->at(0)) - ->method('set') - ->with('two_factor_auth_uid', 'ferdinand'); - $this->session->expects($this->at(1)) + $this->session->expects($this->exactly(2)) ->method('set') - ->with('two_factor_remember_login', true); + ->withConsecutive( + ['two_factor_auth_uid', 'ferdinand'], + ['two_factor_remember_login', true] + ); $this->session->method('getId') ->willReturn('mysessionid'); @@ -605,12 +601,12 @@ public function testPrepareTwoFactorLoginDontRemember() { $this->user->method('getUID') ->willReturn('ferdinand'); - $this->session->expects($this->at(0)) - ->method('set') - ->with('two_factor_auth_uid', 'ferdinand'); - $this->session->expects($this->at(1)) + $this->session->expects($this->exactly(2)) ->method('set') - ->with('two_factor_remember_login', false); + ->withConsecutive( + ['two_factor_auth_uid', 'ferdinand'], + ['two_factor_remember_login', false] + ); $this->session->method('getId') ->willReturn('mysessionid');