diff --git a/.htaccess b/.htaccess index 8a287cec2145d..164a630b75f9b 100644 --- a/.htaccess +++ b/.htaccess @@ -15,11 +15,18 @@ # Add security and privacy related headers - # Avoid doubled headers by unsetting headers in "onsuccess" table, # then add headers to "always" table: https://github.com/nextcloud/server/pull/19002 - Header onsuccess unset Referrer-Policy - Header always set Referrer-Policy "no-referrer" + + + # Only on the login page we need any Origin or Referer header set. + Header onsuccess unset Referrer-Policy + Header always set Referrer-Policy "same-origin" + + + Header onsuccess unset Referrer-Policy + Header always set Referrer-Policy "no-referrer" + Header onsuccess unset X-Content-Type-Options Header always set X-Content-Type-Options "nosniff" diff --git a/build/integration/features/bootstrap/Auth.php b/build/integration/features/bootstrap/Auth.php index e620af4d530eb..aeaade8538352 100644 --- a/build/integration/features/bootstrap/Auth.php +++ b/build/integration/features/bootstrap/Auth.php @@ -1,4 +1,5 @@ baseUrl, 0, -5) . '/login'; + $baseUrl = substr($this->baseUrl, 0, -5); + $loginUrl = $baseUrl . '/login'; // Request a new session and extract CSRF token $client = new Client(); $response = $client->get($loginUrl, [ @@ -222,6 +224,9 @@ public function aNewBrowserSessionIsStarted($remember = false) { 'requesttoken' => $this->requestToken, ], 'cookies' => $this->cookieJar, + 'headers' => [ + 'Origin' => $baseUrl, + ], ] ); $this->extracRequestTokenFromResponse($response); diff --git a/build/integration/features/bootstrap/BasicStructure.php b/build/integration/features/bootstrap/BasicStructure.php index 8500eca6b0fa9..b51c0a6a34ccf 100644 --- a/build/integration/features/bootstrap/BasicStructure.php +++ b/build/integration/features/bootstrap/BasicStructure.php @@ -279,7 +279,8 @@ private function extracRequestTokenFromResponse(ResponseInterface $response) { * @param string $user */ public function loggingInUsingWebAs($user) { - $loginUrl = substr($this->baseUrl, 0, -5) . '/index.php/login'; + $baseUrl = substr($this->baseUrl, 0, -5); + $loginUrl = $baseUrl . '/index.php/login'; // Request a new session and extract CSRF token $client = new Client(); $response = $client->get( @@ -302,6 +303,9 @@ public function loggingInUsingWebAs($user) { 'requesttoken' => $this->requestToken, ], 'cookies' => $this->cookieJar, + 'headers' => [ + 'Origin' => $baseUrl, + ], ] ); $this->extracRequestTokenFromResponse($response); diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index d4d6a17db796a..19d5aae9613a3 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -41,12 +41,14 @@ use OCP\IUserManager; use OCP\Notification\IManager; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\ITrustedDomainHelper; use OCP\Util; class LoginController extends Controller { public const LOGIN_MSG_INVALIDPASSWORD = 'invalidpassword'; public const LOGIN_MSG_USERDISABLED = 'userdisabled'; public const LOGIN_MSG_CSRFCHECKFAILED = 'csrfCheckFailed'; + public const LOGIN_MSG_INVALID_ORIGIN = 'invalidOrigin'; public function __construct( ?string $appName, @@ -167,6 +169,9 @@ public function showLoginForm(?string $user = null, ?string $redirect_url = null Util::addHeader('meta', ['property' => 'og:type', 'content' => 'website']); Util::addHeader('meta', ['property' => 'og:image', 'content' => $this->urlGenerator->getAbsoluteURL($this->urlGenerator->imagePath('core', 'favicon-touch.png'))]); + // Add same-origin referrer policy so we can check for valid requests + Util::addHeader('meta', ['name' => 'referrer', 'content' => 'same-origin']); + $parameters = [ 'alt_login' => OC_App::getAlternativeLogIns(), 'pageTitle' => $this->l10n->t('Login'), @@ -269,29 +274,42 @@ private function generateRedirect(?string $redirectUrl): RedirectResponse { return new RedirectResponse($this->urlGenerator->linkToDefaultPageUrl()); } - /** - * @return RedirectResponse - */ #[NoCSRFRequired] #[PublicPage] #[BruteForceProtection(action: 'login')] #[UseSession] #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)] #[FrontpageRoute(verb: 'POST', url: '/login')] - public function tryLogin(Chain $loginChain, + public function tryLogin( + Chain $loginChain, + ITrustedDomainHelper $trustedDomainHelper, string $user = '', string $password = '', ?string $redirect_url = null, string $timezone = '', - string $timezone_offset = ''): RedirectResponse { - if (!$this->request->passesCSRFCheck()) { + string $timezone_offset = '', + ): RedirectResponse { + $error = ''; + + $origin = $this->request->getHeader('Origin'); + $throttle = true; + if ($origin === '' || !$trustedDomainHelper->isTrustedUrl($origin)) { + // Login attempt not from the same origin, + // We only allow this on the login flow but not on the UI login page. + // This could have come from someone malicious who tries to block a user by triggering the bruteforce protection. + $error = self::LOGIN_MSG_INVALID_ORIGIN; + $throttle = false; + } elseif (!$this->request->passesCSRFCheck()) { if ($this->userSession->isLoggedIn()) { // If the user is already logged in and the CSRF check does not pass then // simply redirect the user to the correct page as required. This is the // case when a user has already logged-in, in another tab. return $this->generateRedirect($redirect_url); } + $error = self::LOGIN_MSG_CSRFCHECKFAILED; + } + if ($error !== '') { // Clear any auth remnants like cookies to ensure a clean login // For the next attempt $this->userSession->logout(); @@ -299,8 +317,8 @@ public function tryLogin(Chain $loginChain, $user, $user, $redirect_url, - self::LOGIN_MSG_CSRFCHECKFAILED, - false, + $error, + $throttle, ); } @@ -371,6 +389,7 @@ private function createLoginFailedResponse( $this->session->set('loginMessages', [ [$loginMessage], [] ]); + return $response; } diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index a5ff6fd61a55e..a6438a35af08e 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -30,6 +30,7 @@ use OCP\IUserManager; use OCP\Notification\IManager; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\ITrustedDomainHelper; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -105,6 +106,9 @@ protected function setUp(): void { $this->request->method('getRemoteAddress') ->willReturn('1.2.3.4'); + $this->request->method('getHeader') + ->with('Origin') + ->willReturn('domain.example.com'); $this->throttler->method('getDelay') ->with( $this->equalTo('1.2.3.4'), @@ -437,6 +441,8 @@ public function testLoginWithInvalidCredentials(): void { $password = 'secret'; $loginPageUrl = '/login?redirect_url=/apps/files'; $loginChain = $this->createMock(LoginChain::class); + $trustedDomainHelper = $this->createMock(ITrustedDomainHelper::class); + $trustedDomainHelper->method('isTrustedUrl')->willReturn(true); $this->request ->expects($this->once()) ->method('passesCSRFCheck') @@ -463,7 +469,7 @@ public function testLoginWithInvalidCredentials(): void { $expected = new RedirectResponse($loginPageUrl); $expected->throttle(['user' => 'MyUserName']); - $response = $this->loginController->tryLogin($loginChain, $user, $password, '/apps/files'); + $response = $this->loginController->tryLogin($loginChain, $trustedDomainHelper, $user, $password, '/apps/files'); $this->assertEquals($expected, $response); } @@ -472,6 +478,8 @@ public function testLoginWithValidCredentials(): void { $user = 'MyUserName'; $password = 'secret'; $loginChain = $this->createMock(LoginChain::class); + $trustedDomainHelper = $this->createMock(ITrustedDomainHelper::class); + $trustedDomainHelper->method('isTrustedUrl')->willReturn(true); $this->request ->expects($this->once()) ->method('passesCSRFCheck') @@ -492,7 +500,7 @@ public function testLoginWithValidCredentials(): void { ->willReturn('/default/foo'); $expected = new RedirectResponse('/default/foo'); - $this->assertEquals($expected, $this->loginController->tryLogin($loginChain, $user, $password)); + $this->assertEquals($expected, $this->loginController->tryLogin($loginChain, $trustedDomainHelper, $user, $password)); } public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn(): void { @@ -504,6 +512,8 @@ public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn(): void { $password = 'secret'; $originalUrl = 'another%20url'; $loginChain = $this->createMock(LoginChain::class); + $trustedDomainHelper = $this->createMock(ITrustedDomainHelper::class); + $trustedDomainHelper->method('isTrustedUrl')->willReturn(true); $this->request ->expects($this->once()) ->method('passesCSRFCheck') @@ -517,9 +527,10 @@ public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn(): void { $this->userSession->expects($this->never()) ->method('createRememberMeToken'); - $response = $this->loginController->tryLogin($loginChain, 'Jane', $password, $originalUrl); + $response = $this->loginController->tryLogin($loginChain, $trustedDomainHelper, 'Jane', $password, $originalUrl); $expected = new RedirectResponse(''); + $expected->throttle(['user' => 'Jane']); $this->assertEquals($expected, $response); } @@ -533,6 +544,8 @@ public function testLoginWithoutPassedCsrfCheckAndLoggedIn(): void { $originalUrl = 'another url'; $redirectUrl = 'http://localhost/another url'; $loginChain = $this->createMock(LoginChain::class); + $trustedDomainHelper = $this->createMock(ITrustedDomainHelper::class); + $trustedDomainHelper->method('isTrustedUrl')->willReturn(true); $this->request ->expects($this->once()) ->method('passesCSRFCheck') @@ -554,7 +567,7 @@ public function testLoginWithoutPassedCsrfCheckAndLoggedIn(): void { ->with('remember_login_cookie_lifetime') ->willReturn(1234); - $response = $this->loginController->tryLogin($loginChain, 'Jane', $password, $originalUrl); + $response = $this->loginController->tryLogin($loginChain, $trustedDomainHelper, 'Jane', $password, $originalUrl); $expected = new RedirectResponse($redirectUrl); $this->assertEquals($expected, $response); @@ -565,6 +578,8 @@ public function testLoginWithValidCredentialsAndRedirectUrl(): void { $password = 'secret'; $redirectUrl = 'https://next.cloud/apps/mail'; $loginChain = $this->createMock(LoginChain::class); + $trustedDomainHelper = $this->createMock(ITrustedDomainHelper::class); + $trustedDomainHelper->method('isTrustedUrl')->willReturn(true); $this->request ->expects($this->once()) ->method('passesCSRFCheck') @@ -589,13 +604,15 @@ public function testLoginWithValidCredentialsAndRedirectUrl(): void { ->willReturn($redirectUrl); $expected = new RedirectResponse($redirectUrl); - $response = $this->loginController->tryLogin($loginChain, $user, $password, '/apps/mail'); + $response = $this->loginController->tryLogin($loginChain, $trustedDomainHelper, $user, $password, '/apps/mail'); $this->assertEquals($expected, $response); } public function testToNotLeakLoginName(): void { $loginChain = $this->createMock(LoginChain::class); + $trustedDomainHelper = $this->createMock(ITrustedDomainHelper::class); + $trustedDomainHelper->method('isTrustedUrl')->willReturn(true); $this->request ->expects($this->once()) ->method('passesCSRFCheck') @@ -628,6 +645,7 @@ public function testToNotLeakLoginName(): void { $response = $this->loginController->tryLogin( $loginChain, + $trustedDomainHelper, 'john@doe.com', 'just wrong', '/apps/files'