feat(login): add origin check at login

Signed-off-by: Benjamin Gaussorgues <[email protected]>
nextcloud · Dec 4, 2024 · 7150a26 · 7150a26
1 parent 3027bb5
commit 7150a26
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 17 deletions.
diff --git a/.htaccess b/.htaccess
@@ -15,11 +15,18 @@
 
   <IfModule mod_env.c>
     # 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"
+
+    <If "%{REQUEST_URI} =~ m#/login$#">
+      # 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"
+    </If>
+    <Else>
+      Header onsuccess unset Referrer-Policy
+      Header always set Referrer-Policy "no-referrer"
+    </Else>
 
     Header onsuccess unset X-Content-Type-Options
     Header always set X-Content-Type-Options "nosniff"

diff --git 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
@@ -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,38 +274,51 @@ 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();
 			return $this->createLoginFailedResponse(
 				$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/cypress/dockerNode.ts b/cypress/dockerNode.ts
@@ -127,6 +127,10 @@ export const configureNextcloud = async function() {
 	await runExec(container, ['php', 'occ', 'config:system:set', 'force_locale', '--value', 'en_US'], true)
 	await runExec(container, ['php', 'occ', 'config:system:set', 'enforce_theme', '--value', 'light'], true)
 
+	// Set IP as a trusted origin
+	const ip = await getContainerIP(container)
+	await runExec(container, ['php', 'occ', 'config:system:set', 'trusted_domains', '0', '--value', `${ip}`], true)
+
 	// Speed up test and make them less flaky. If a cron execution is needed, it can be triggered manually.
 	await runExec(container, ['php', 'occ', 'background:cron'], true)
 

diff --git 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,
 			'[email protected]',
 			'just wrong',
 			'/apps/files'