Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(login): add origin check at login #49560

Merged
merged 2 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions .htaccess
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Altahrim marked this conversation as resolved.
Show resolved Hide resolved

Header onsuccess unset X-Content-Type-Options
Header always set X-Content-Type-Options "nosniff"
Expand Down
7 changes: 6 additions & 1 deletion build/integration/features/bootstrap/Auth.php
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

/**
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
Expand Down Expand Up @@ -203,7 +204,8 @@ public function requestingWithBrowserSession($url, $method) {
* @param bool $remember
*/
public function aNewBrowserSessionIsStarted($remember = false) {
$loginUrl = substr($this->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, [
Expand All @@ -222,6 +224,9 @@ public function aNewBrowserSessionIsStarted($remember = false) {
'requesttoken' => $this->requestToken,
],
'cookies' => $this->cookieJar,
'headers' => [
'Origin' => $baseUrl,
],
]
);
$this->extracRequestTokenFromResponse($response);
Expand Down
6 changes: 5 additions & 1 deletion build/integration/features/bootstrap/BasicStructure.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -302,6 +303,9 @@ public function loggingInUsingWebAs($user) {
'requesttoken' => $this->requestToken,
],
'cookies' => $this->cookieJar,
'headers' => [
'Origin' => $baseUrl,
],
]
);
$this->extracRequestTokenFromResponse($response);
Expand Down
35 changes: 27 additions & 8 deletions core/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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'),
Expand Down Expand Up @@ -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,
);
}

Expand Down Expand Up @@ -371,6 +389,7 @@ private function createLoginFailedResponse(
$this->session->set('loginMessages', [
[$loginMessage], []
]);

return $response;
}

Expand Down
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
"@cypress/vue2": "^2.1.1",
"@cypress/webpack-preprocessor": "^6.0.2",
"@nextcloud/babel-config": "^1.2.0",
"@nextcloud/cypress": "^1.0.0-beta.11",
"@nextcloud/cypress": "^1.0.0-beta.12",
"@nextcloud/eslint-config": "^8.4.1",
"@nextcloud/stylelint-config": "^3.0.1",
"@nextcloud/typings": "^1.9.1",
Expand Down
28 changes: 23 additions & 5 deletions tests/Core/Controller/LoginControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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'),
Expand Down Expand Up @@ -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')
Expand All @@ -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);
}
Expand All @@ -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')
Expand All @@ -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 {
Expand All @@ -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')
Expand All @@ -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);
}

Expand All @@ -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')
Expand All @@ -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);
Expand All @@ -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')
Expand All @@ -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')
Expand Down Expand Up @@ -628,6 +645,7 @@ public function testToNotLeakLoginName(): void {

$response = $this->loginController->tryLogin(
$loginChain,
$trustedDomainHelper,
'[email protected]',
'just wrong',
'/apps/files'
Expand Down
Loading