diff --git a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php index 94fc329d5c71f..5b903779b130b 100644 --- a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php @@ -20,6 +20,7 @@ use OCP\Authentication\Token\IToken; use OCP\IRequest; use OCP\ISession; +use OCP\IUser; use OCP\IUserSession; use OCP\Session\Exceptions\SessionNotAvailableException; use OCP\User\Backend\IPasswordConfirmationBackend; @@ -28,7 +29,20 @@ use ReflectionMethod; class PasswordConfirmationMiddleware extends Middleware { - private array $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true]; + private const PASSWORD_CONFIRMATION_TIMEOUT = 30 * 60; + private const PASSWORD_CONFIRMATION_GRACE_SECONDS = 15; + + /** + * Backends that cannot participate in password confirmation are exempt from both + * strict and non-strict password confirmation checks. New backends should prefer + * implementing IPasswordConfirmationBackend instead of being added here. + * + * @var array + */ + private array $excludedUserBackEnds = [ + 'user_saml' => true, + 'user_globalsiteselector' => true, + ]; public function __construct( private ControllerMethodReflector $reflector, @@ -52,57 +66,48 @@ public function beforeController(Controller $controller, string $methodName) { } $user = $this->userSession->getUser(); - $backendClassName = ''; - if ($user !== null) { - $backend = $user->getBackend(); - if ($backend instanceof IPasswordConfirmationBackend) { - if (!$backend->canConfirmPassword($user->getUID())) { - return; - } - } - - $backendClassName = $user->getBackendClassName(); + + if ($this->isBackendExemptFromPasswordConfirmation($user)) { + return; } try { $sessionId = $this->session->getId(); $token = $this->tokenProvider->getToken($sessionId); } catch (SessionNotAvailableException|InvalidTokenException|WipeTokenException|ExpiredTokenException) { - // States we do not deal with here. + // Password confirmation is only enforced for requests backed by a valid interactive session token. + // Requests without such a token are left to be rejected or otherwise handled by the normal + // authentication/session middleware stack. return; } - $scope = $token->getScopeAsArray(); - if (isset($scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION]) && $scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION] === true) { - // Users logging in from SSO backends cannot confirm their password by design + if ($this->isTokenExemptFromPasswordConfirmation($token)) { + // Some session tokens are marked to skip password validation entirely. return; } + $now = $this->timeFactory->getTime(); $reflectionMethod = new ReflectionMethod($controller, $methodName); if ($this->isPasswordConfirmationStrict($reflectionMethod)) { - $authHeader = $this->request->getHeader('Authorization'); - if (!str_starts_with(strtolower($authHeader), 'basic ')) { - throw new NotConfirmedException('Required authorization header missing'); - } - [, $password] = explode(':', base64_decode(substr($authHeader, 6)), 2); - $loginName = $this->session->get('loginname'); - $loginResult = $this->userManager->checkPassword($loginName, $password); - if ($loginResult === false) { - throw new NotConfirmedException(); - } - - $this->session->set('last-password-confirm', $this->timeFactory->getTime()); - } else { - $lastConfirm = (int)$this->session->get('last-password-confirm'); - // TODO: confirm excludedUserBackEnds can go away and remove it - if (!isset($this->excludedUserBackEnds[$backendClassName]) && $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15))) { // allow 15 seconds delay - throw new NotConfirmedException(); - } + $this->confirmPasswordFromAuthorizationHeader(); + $this->session->set('last-password-confirm', $now); + return; + } + + $lastConfirm = (int)$this->session->get('last-password-confirm'); + $minimumRequiredConfirmTime = $now + - (self::PASSWORD_CONFIRMATION_TIMEOUT + self::PASSWORD_CONFIRMATION_GRACE_SECONDS); + + if ($lastConfirm < $minimumRequiredConfirmTime) { + throw new NotConfirmedException(); } } private function needsPasswordConfirmation(): bool { - return $this->reflector->hasAnnotationOrAttribute('PasswordConfirmationRequired', PasswordConfirmationRequired::class); + return $this->reflector->hasAnnotationOrAttribute( + 'PasswordConfirmationRequired', + PasswordConfirmationRequired::class + ); } private function isPasswordConfirmationStrict(ReflectionMethod $reflectionMethod): bool { @@ -110,4 +115,60 @@ private function isPasswordConfirmationStrict(ReflectionMethod $reflectionMethod $attributes = $reflectionMethod->getAttributes(PasswordConfirmationRequired::class); return !empty($attributes) && ($attributes[0]->newInstance()->getStrict()); } + + private function isBackendExemptFromPasswordConfirmation(?IUser $user): bool { + if ($user === null) { + return false; + } + + $backend = $user->getBackend(); + + if ( + $backend instanceof IPasswordConfirmationBackend + && !$backend->canConfirmPassword($user->getUID()) + ) { + return true; + } + + return $this->isLegacyBackendExcludedFromRecentConfirmation($user); + } + + private function isLegacyBackendExcludedFromRecentConfirmation(?IUser $user): bool { + $backendClassName = $user?->getBackendClassName() ?? ''; + // TODO: confirm excludedUserBackEnds can go away and remove it + return isset($this->excludedUserBackEnds[$backendClassName]); + } + + private function isTokenExemptFromPasswordConfirmation(IToken $token): bool { + $scope = $token->getScopeAsArray(); + return isset($scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION]) + && $scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION] === true; + } + + /** + * @throws NotConfirmedException + */ + private function confirmPasswordFromAuthorizationHeader(): void { + $authHeader = $this->request->getHeader('Authorization'); + + if (!str_starts_with(strtolower($authHeader), 'basic ')) { + throw new NotConfirmedException('Required authorization header missing'); + } + + $decodedCredentials = base64_decode(substr($authHeader, 6), true); + + if ($decodedCredentials === false || !str_contains($decodedCredentials, ':')) { + throw new NotConfirmedException('Malformed authorization header'); + } + + [$ignoredUser, $password] = explode(':', $decodedCredentials, 2); + // Use the session's loginname, not the one from the Authorization header, + // to prevent credential stuffing against arbitrary usernames. + $loginName = $this->session->get('loginname'); + $loginResult = $this->userManager->checkPassword($loginName, $password); + + if ($loginResult === false) { + throw new NotConfirmedException(); + } + } } diff --git a/lib/private/Template/JSConfigHelper.php b/lib/private/Template/JSConfigHelper.php index 4907274f7304e..1d1f09e6b852a 100644 --- a/lib/private/Template/JSConfigHelper.php +++ b/lib/private/Template/JSConfigHelper.php @@ -40,8 +40,17 @@ class JSConfigHelper { - /** @var array user back-ends excluded from password verification */ - private $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true]; + /** + * Backends that cannot participate in password confirmation are exempt from both + * strict and non-strict password confirmation checks. New backends should prefer + * implementing IPasswordConfirmationBackend instead of being added here. + * + * @var array + */ + private array $excludedUserBackEnds = [ + 'user_saml' => true, + 'user_globalsiteselector' => true, + ]; public function __construct( protected ServerVersion $serverVersion, @@ -63,18 +72,15 @@ public function __construct( } public function getConfig(): string { - $userBackendAllowsPasswordConfirmation = true; + $userCanUsePasswordConfirmation = false; + $canUserValidatePassword = $this->canUserValidatePassword(); + if ($this->currentUser !== null) { $uid = $this->currentUser->getUID(); - $backend = $this->currentUser->getBackend(); - if ($backend instanceof IPasswordConfirmationBackend) { - $userBackendAllowsPasswordConfirmation = $backend->canConfirmPassword($uid) && $this->canUserValidatePassword(); - } elseif (isset($this->excludedUserBackEnds[$this->currentUser->getBackendClassName()])) { - $userBackendAllowsPasswordConfirmation = false; - } else { - $userBackendAllowsPasswordConfirmation = $this->canUserValidatePassword(); - } + $userCanUsePasswordConfirmation = + $this->canBackendConfirmPassword($this->currentUser) + && $canUserValidatePassword; } else { $uid = null; } @@ -126,7 +132,7 @@ public function getConfig(): string { } if ($this->currentUser instanceof IUser) { - if ($this->canUserValidatePassword()) { + if ($canUserValidatePassword) { $lastConfirmTimestamp = $this->session->get('last-password-confirm'); if (!is_int($lastConfirmTimestamp)) { $lastConfirmTimestamp = 0; @@ -174,7 +180,7 @@ public function getConfig(): string { $array = [ '_oc_debug' => $this->config->getSystemValue('debug', false) ? 'true' : 'false', '_oc_isadmin' => $uid !== null && $this->groupManager->isAdmin($uid) ? 'true' : 'false', - 'backendAllowsPasswordConfirmation' => $userBackendAllowsPasswordConfirmation ? 'true' : 'false', + 'backendAllowsPasswordConfirmation' => $userCanUsePasswordConfirmation ? 'true' : 'false', 'oc_dataURL' => is_string($dataLocation) ? '"' . $dataLocation . '"' : 'false', '_oc_webroot' => '"' . \OC::$WEBROOT . '"', '_oc_appswebroots' => str_replace('\\/', '/', json_encode($apps_paths)), // Ugly unescape slashes waiting for better solution @@ -304,10 +310,31 @@ protected function canUserValidatePassword(): bool { try { $token = $this->tokenProvider->getToken($this->session->getId()); } catch (ExpiredTokenException|WipeTokenException|InvalidTokenException|SessionNotAvailableException) { - // actually we do not know, so we fall back to this statement + // If token lookup fails, fall back to allowing password validation in the UI. return true; } $scope = $token->getScopeAsArray(); return !isset($scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION]) || $scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION] === false; } + + private function canBackendConfirmPassword(?IUser $user): bool { + if ($user === null) { + return false; + } + + $backend = $user->getBackend(); + if ( + $backend instanceof IPasswordConfirmationBackend + && !$backend->canConfirmPassword($user->getUID()) + ) { + return false; + } + + return !$this->isLegacyBackendExcludedFromPasswordConfirmation($user); + } + + private function isLegacyBackendExcludedFromPasswordConfirmation(?IUser $user): bool { + $backendClassName = $user?->getBackendClassName() ?? ''; + return isset($this->excludedUserBackEnds[$backendClassName]); + } } diff --git a/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php b/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php index cd1cdaa49ca33..16900069da36d 100644 --- a/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php +++ b/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php @@ -32,6 +32,35 @@ public function testAnnotation() { public function testAttribute() { } + // Non-strict — for backend capability and null-user tests + #[PasswordConfirmationRequired] + public function testLegacyBackendExempt() {} + + #[PasswordConfirmationRequired] + public function testIPasswordConfirmationBackendExempt() {} + + #[PasswordConfirmationRequired] + public function testIPasswordConfirmationBackendNotExempt() {} + + #[PasswordConfirmationRequired] + public function testNullUser() {} + + // Strict — one controller method per strict-mode test + #[PasswordConfirmationRequired(strict: true)] + public function testStrictModeValidCredentials() {} + + #[PasswordConfirmationRequired(strict: true)] + public function testStrictModeMissingAuthHeader() {} + + #[PasswordConfirmationRequired(strict: true)] + public function testStrictModeMalformedBase64() {} + + #[PasswordConfirmationRequired(strict: true)] + public function testStrictModeWrongPassword() {} + + #[PasswordConfirmationRequired(strict: true)] + public function testStrictModeLegacyBackendExempt() {} + #[PasswordConfirmationRequired] public function testSSO() { } diff --git a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php index c1c7e587fd256..1dae157a8d85a 100644 --- a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php @@ -19,6 +19,7 @@ use OCP\IUser; use OCP\IUserSession; use OCP\Server; +use OCP\User\Backend\IPasswordConfirmationBackend; use Psr\Log\LoggerInterface; use Test\AppFramework\Middleware\Security\Mock\PasswordConfirmationMiddlewareController; use Test\TestCase; @@ -93,8 +94,8 @@ public function testDifferentAnnotation(): void { $this->middleware->beforeController($this->controller, __FUNCTION__); } - #[\PHPUnit\Framework\Attributes\DataProvider('dataProvider')] - public function testAnnotation($backend, $lastConfirm, $currentTime, $exception): void { + #[\PHPUnit\Framework\Attributes\DataProvider('dataProviderNonExemptBackend')] + public function testAnnotation(string $backend, int $lastConfirm, int $currentTime, bool $exception): void { $this->reflector->reflect($this->controller, __FUNCTION__); $this->user->method('getBackendClassName') @@ -126,8 +127,8 @@ public function testAnnotation($backend, $lastConfirm, $currentTime, $exception) $this->assertSame($exception, $thrown); } - #[\PHPUnit\Framework\Attributes\DataProvider('dataProvider')] - public function testAttribute($backend, $lastConfirm, $currentTime, $exception): void { + #[\PHPUnit\Framework\Attributes\DataProvider('dataProviderNonExemptBackend')] + public function testAttribute(string $backend, int $lastConfirm, int $currentTime, bool $exception): void { $this->reflector->reflect($this->controller, __FUNCTION__); $this->user->method('getBackendClassName') @@ -159,19 +160,243 @@ public function testAttribute($backend, $lastConfirm, $currentTime, $exception): $this->assertSame($exception, $thrown); } - - - public static function dataProvider(): array { + public static function dataProviderNonExemptBackend(): array { return [ ['foo', 2000, 4000, true], ['foo', 2000, 3000, false], - ['user_saml', 2000, 4000, false], - ['user_saml', 2000, 3000, false], ['foo', 2000, 3815, false], ['foo', 2000, 3816, true], ]; } + /** + * @dataProvider dataProviderLegacyExemptBackends + */ + #[\PHPUnit\Framework\Attributes\DataProvider('dataProviderLegacyExemptBackends')] + public function testLegacyBackendExempt(string $backend): void { + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->user->method('getBackendClassName') + ->willReturn($backend); + $this->userSession->method('getUser') + ->willReturn($this->user); + + // Backend is exempt — getToken() must never be called + $this->tokenProvider->expects($this->never()) + ->method('getToken'); + + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + public static function dataProviderLegacyExemptBackends(): array { + return [ + ['user_saml'], + ['user_globalsiteselector'], + ]; + } + + public function testIPasswordConfirmationBackendExempt(): void { + $this->reflector->reflect($this->controller, __FUNCTION__); + + $backend = $this->createMock(IPasswordConfirmationBackend::class); + $backend->method('canConfirmPassword')->with('uid')->willReturn(false); + + $this->user->method('getBackend')->willReturn($backend); + $this->user->method('getUID')->willReturn('uid'); + $this->userSession->method('getUser')->willReturn($this->user); + + // Exempt before token check + $this->tokenProvider->expects($this->never())->method('getToken'); + + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + public function testIPasswordConfirmationBackendNotExempt(): void { + static $sessionId = 'mySession1d'; + $this->reflector->reflect($this->controller, __FUNCTION__); + + $backend = $this->createMock(IPasswordConfirmationBackend::class); + $backend->method('canConfirmPassword')->with('uid')->willReturn(true); + + $this->user->method('getBackend')->willReturn($backend); + $this->user->method('getUID')->willReturn('uid'); + $this->user->method('getBackendClassName')->willReturn('capable_backend'); + $this->userSession->method('getUser')->willReturn($this->user); + + $this->session->method('getId')->willReturn($sessionId); + $this->session->method('get')->with('last-password-confirm')->willReturn(2000); + $this->timeFactory->method('getTime')->willReturn(3000); // within window — no exception + + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray')->willReturn([]); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with($sessionId) + ->willReturn($token); + + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + public function testNullUser(): void { + static $sessionId = 'mySession1d'; + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->userSession->method('getUser')->willReturn(null); + + $this->session->method('getId')->willReturn($sessionId); + $this->session->method('get')->with('last-password-confirm')->willReturn(2000); + $this->timeFactory->method('getTime')->willReturn(3000); // within window — no exception + + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray')->willReturn([]); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with($sessionId) + ->willReturn($token); + + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + public function testStrictModeValidCredentials(): void { + static $sessionId = 'mySession1d'; + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->user->method('getBackendClassName')->willReturn('foo'); + $this->userSession->method('getUser')->willReturn($this->user); + $this->session->method('getId')->willReturn($sessionId); + $this->timeFactory->method('getTime')->willReturn(9999); + + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray')->willReturn([]); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with($sessionId) + ->willReturn($token); + + $this->request->method('getHeader') + ->with('Authorization') + ->willReturn('Basic ' . base64_encode('user:correctpassword')); + + $this->session->method('get') + ->with('loginname') + ->willReturn('user'); + + $loginUser = $this->createMock(IUser::class); + $this->userManager->expects($this->once()) + ->method('checkPassword') + ->with('user', 'correctpassword') + ->willReturn($loginUser); + + // Timestamp must be written after successful strict confirmation + $this->session->expects($this->once()) + ->method('set') + ->with('last-password-confirm', 9999); + + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + public function testStrictModeMissingAuthHeader(): void { + static $sessionId = 'mySession1d'; + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->user->method('getBackendClassName')->willReturn('foo'); + $this->userSession->method('getUser')->willReturn($this->user); + $this->session->method('getId')->willReturn($sessionId); + $this->timeFactory->method('getTime')->willReturn(9999); + + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray')->willReturn([]); + $this->tokenProvider->method('getToken')->willReturn($token); + + $this->request->method('getHeader') + ->with('Authorization') + ->willReturn(''); // no header + + $this->session->expects($this->never())->method('set'); + $this->userManager->expects($this->never())->method('checkPassword'); + $this->expectException(NotConfirmedException::class); + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + /** + * @dataProvider dataProviderMalformedAuthHeaders + */ + #[\PHPUnit\Framework\Attributes\DataProvider('dataProviderMalformedAuthHeaders')] + public function testStrictModeMalformedBase64(string $headerValue): void { + static $sessionId = 'mySession1d'; + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->user->method('getBackendClassName')->willReturn('foo'); + $this->userSession->method('getUser')->willReturn($this->user); + $this->session->method('getId')->willReturn($sessionId); + $this->timeFactory->method('getTime')->willReturn(9999); + + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray')->willReturn([]); + $this->tokenProvider->method('getToken')->willReturn($token); + + $this->request->method('getHeader') + ->with('Authorization') + ->willReturn($headerValue); + + $this->session->expects($this->never())->method('set'); + $this->userManager->expects($this->never())->method('checkPassword'); + $this->expectException(NotConfirmedException::class); + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + public static function dataProviderMalformedAuthHeaders(): array { + return [ + 'invalid base64' => ['Basic !!!notbase64!!!'], + 'no colon in decoded' => ['Basic ' . base64_encode('nodivider')], + ]; + } + + public function testStrictModeWrongPassword(): void { + static $sessionId = 'mySession1d'; + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->user->method('getBackendClassName')->willReturn('foo'); + $this->userSession->method('getUser')->willReturn($this->user); + $this->session->method('getId')->willReturn($sessionId); + $this->timeFactory->method('getTime')->willReturn(9999); + + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray')->willReturn([]); + $this->tokenProvider->method('getToken')->willReturn($token); + + $this->request->method('getHeader') + ->with('Authorization') + ->willReturn('Basic ' . base64_encode('user:wrongpassword')); + + $this->session->method('get') + ->with('loginname') + ->willReturn('user'); + + $this->userManager->method('checkPassword') + ->with('user', 'wrongpassword') + ->willReturn(false); + + $this->session->expects($this->never())->method('set'); + + $this->expectException(NotConfirmedException::class); + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + + public function testStrictModeLegacyBackendExempt(): void { + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->user->method('getBackendClassName')->willReturn('user_saml'); + $this->userSession->method('getUser')->willReturn($this->user); + + // Must exit before reaching the auth header or token checks + $this->tokenProvider->expects($this->never())->method('getToken'); + $this->request->expects($this->never())->method('getHeader'); + $this->userManager->expects($this->never())->method('checkPassword'); + + $this->middleware->beforeController($this->controller, __FUNCTION__); + } + public function testSSO(): void { static $sessionId = 'mySession1d';