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.

Sign up for GitHub

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: Refined session handling #40543

Closed
wants to merge 1 commit into from
Closed

feat: Refined session handling #40543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion apps/settings/lib/Controller/ChangePasswordController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ public function changePersonalPassword(string $oldpassword = '', string $newpass
]);
}

$this->userSession->updateSessionTokenPassword($newpassword);
$this->userSession->updateSessionTokenPassword(
$this->request->getCookie(Session::COOKIE_SESSION_ID),
$newpassword,
);

return new JSONResponse([
'status' => 'success',
Expand Down
4 changes: 0 additions & 4 deletions core/Controller/LoginController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,6 @@ public function __construct(
*/
#[UseSession]
public function logout() {
$loginToken = $this->request->getCookie('nc_token');
if (!is_null($loginToken)) {
$this->config->deleteUserValue($this->userSession->getUser()->getUID(), 'login_token', $loginToken);
}
$this->userSession->logout();

$response = new RedirectResponse($this->urlGenerator->linkToRouteAbsolute(
Expand Down
12 changes: 7 additions & 5 deletions lib/base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@
// slash which is required by URL generation.
if (isset($_SERVER['REQUEST_URI']) && $_SERVER['REQUEST_URI'] === \OC::$WEBROOT &&
substr($_SERVER['REQUEST_URI'], -1) !== '/') {
header('Location: '.\OC::$WEBROOT.'/');

Check failure on line 204 in lib/base.php

View workflow job for this annotation

GitHub Actions / static-code-analysis-security

TaintedHeader 

lib/base.php:204:12: TaintedHeader: Detected tainted header (see https://psalm.dev/256)
exit();
}
}
Expand Down Expand Up @@ -283,7 +283,7 @@
throw new Exception('Not installed');
} else {
$url = OC::$WEBROOT . '/index.php';
header('Location: ' . $url);

Check failure on line 286 in lib/base.php

View workflow job for this annotation

GitHub Actions / static-code-analysis-security

TaintedHeader 

lib/base.php:286:12: TaintedHeader: Detected tainted header (see https://psalm.dev/256)
}
exit();
}
Expand Down Expand Up @@ -420,8 +420,8 @@
// TODO: See https://github.com/nextcloud/server/issues/37277#issuecomment-1476366147 and the other comments
// TODO: for further information.
// $isDavRequest = strpos($request->getRequestUri(), '/remote.php/dav') === 0 || strpos($request->getRequestUri(), '/remote.php/webdav') === 0;
// if ($request->getHeader('Authorization') !== '' && is_null($request->getCookie('cookie_test')) && $isDavRequest && !isset($_COOKIE['nc_session_id'])) {
// setcookie('cookie_test', 'test', time() + 3600);
// if ($request->getHeader('Authorization') !== '' && is_null($request->getCookie(\OC\User\Session::COOKIE_TEST)) && $isDavRequest)) {
// setcookie(\OC\User\Session::COOKIE_TEST, 'test', time() + 3600);
// // Do not initialize the session if a request is authenticated directly
// // unless there is a session cookie already sent along
// return;
Expand Down Expand Up @@ -1142,9 +1142,11 @@
return true;
}
if (isset($_COOKIE['nc_username'])
&& isset($_COOKIE['nc_token'])
&& isset($_COOKIE['nc_session_id'])
&& $userSession->loginWithCookie($_COOKIE['nc_username'], $_COOKIE['nc_token'], $_COOKIE['nc_session_id'])) {
&& isset($_COOKIE[\OC\User\Session::COOKIE_SESSION_ID])
&& $userSession->loginWithCookie(
$_COOKIE['nc_username'],
$_COOKIE[\OC\User\Session::COOKIE_SESSION_ID])
) {
return true;
}
if ($userSession->tryBasicAuthLogin($request, Server::get(IThrottler::class))) {
Expand Down
2 changes: 1 addition & 1 deletion lib/private/AppFramework/Http/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ private function cookieCheckRequired(): bool {
if ($this->getHeader('OCS-APIREQUEST')) {
return false;
}
if ($this->getCookie(session_name()) === null && $this->getCookie('nc_token') === null) {
if ($this->getCookie(session_name()) === null) {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/private/Authentication/Login/FinishRememberedLoginCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public function __construct(Session $userSession, IConfig $config) {

public function process(LoginData $loginData): LoginResult {
if ($loginData->isRememberLogin() && !$this->config->getSystemValueBool('auto_logout', false)) {
$this->userSession->createRememberMeToken($loginData->getUser());
// TODO: finish login here? don't set cookies earlier
}

return $this->processNextOrFinishSuccessfully($loginData);
Expand Down
11 changes: 0 additions & 11 deletions lib/private/Authentication/Token/IProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,6 @@ public function getToken(string $tokenId): IToken;
*/
public function getTokenById(int $tokenId): IToken;

/**
* Duplicate an existing session token
*
* @param string $oldSessionId
* @param string $sessionId
* @throws InvalidTokenException
* @throws \RuntimeException when OpenSSL reports a problem
* @return IToken The new token
*/
public function renewSessionToken(string $oldSessionId, string $sessionId): IToken;

/**
* Invalidate (delete) the given session token
*
Expand Down
16 changes: 0 additions & 16 deletions lib/private/Authentication/Token/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,22 +159,6 @@ public function getTokenById(int $tokenId): IToken {
}
}

/**
* @param string $oldSessionId
* @param string $sessionId
* @throws InvalidTokenException
* @return IToken
*/
public function renewSessionToken(string $oldSessionId, string $sessionId): IToken {
try {
return $this->publicKeyTokenProvider->renewSessionToken($oldSessionId, $sessionId);
} catch (ExpiredTokenException $e) {
throw $e;
} catch (InvalidTokenException $e) {
throw $e;
}
}

/**
* @param IToken $savedToken
* @param string $tokenId session token
Expand Down
31 changes: 0 additions & 31 deletions lib/private/Authentication/Token/PublicKeyTokenProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,37 +218,6 @@ public function getTokenById(int $tokenId): IToken {
return $token;
}

public function renewSessionToken(string $oldSessionId, string $sessionId): IToken {
$this->cache->clear();

return $this->atomic(function () use ($oldSessionId, $sessionId) {
$token = $this->getToken($oldSessionId);

if (!($token instanceof PublicKeyToken)) {
throw new InvalidTokenException("Invalid token type");
}

$password = null;
if (!is_null($token->getPassword())) {
$privateKey = $this->decrypt($token->getPrivateKey(), $oldSessionId);
$password = $this->decryptPassword($token->getPassword(), $privateKey);
}
$newToken = $this->generateToken(
$sessionId,
$token->getUID(),
$token->getLoginName(),
$password,
$token->getName(),
IToken::TEMPORARY_TOKEN,
$token->getRemember()
);

$this->mapper->delete($token);

return $newToken;
}, $this->db);
}

public function invalidateToken(string $token) {
$this->cache->clear();

Expand Down
3 changes: 1 addition & 2 deletions lib/private/Authentication/TwoFactorAuth/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,7 @@ public function verifyChallenge(string $providerId, IUser $user, string $challen
$passed = $provider->verifyChallenge($user, $challenge);
if ($passed) {
if ($this->session->get(self::REMEMBER_LOGIN) === true) {
// TODO: resolve cyclic dependency and use DI
\OC::$server->getUserSession()->createRememberMeToken($user);
// TODO: finish login here? don't set cookies earlier
}
$this->session->remove(self::SESSION_UID_KEY);
$this->session->remove(self::REMEMBER_LOGIN);
Expand Down
9 changes: 1 addition & 8 deletions lib/private/Session/Internal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,7 @@ public function regenerateId(bool $deleteOldSession = true, bool $updateToken =
// Get the new id to update the token
$newId = $this->getId();

/** @var IProvider $tokenProvider */
$tokenProvider = \OCP\Server::get(IProvider::class);

try {
$tokenProvider->renewSessionToken($oldId, $newId);
} catch (InvalidTokenException $e) {
// Just ignore
}
// TODO: refresh cookie?!
}
}

Expand Down
111 changes: 30 additions & 81 deletions lib/private/User/Session.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@
* @package OC\User
*/
class Session implements IUserSession, Emitter {
const COOKIE_SESSION_ID = 'nc_session_id';
const COOKIE_TEST = 'cookie_test';
const COOKIE_USERNAME = 'nc_username';

/** @var Manager $manager */
private $manager;

Expand Down Expand Up @@ -246,16 +250,12 @@ protected function validateSession() {
$appPassword = $this->session->get('app_password');

if (is_null($appPassword)) {
try {
$token = $this->session->getId();
} catch (SessionNotAvailableException $ex) {
return;
}
$token = \OCP\Server::get(IRequest::class)->getCookie(self::COOKIE_SESSION_ID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit scared ;)
Isn't that a cyclic dependency?

} else {
$token = $appPassword;
}

if (!$this->validateToken($token)) {
if ($token === null || !$this->validateToken($token)) {
// Session was invalidated
$this->logout();
}
Expand Down Expand Up @@ -485,10 +485,10 @@ private function handleLoginFailed(IThrottler $throttler, int $currentDelay, str
}

protected function supportsCookies(IRequest $request) {
if (!is_null($request->getCookie('cookie_test'))) {
if (!is_null($request->getCookie(self::COOKIE_TEST))) {
return true;
}
setcookie('cookie_test', 'test', $this->timeFactory->getTime() + 3600);
setcookie(self::COOKIE_TEST, 'test', $this->timeFactory->getTime() + 3600);
return false;
}

Expand Down Expand Up @@ -680,18 +680,10 @@ public function createSessionToken(IRequest $request, $uid, $loginName, $passwor
return false;
}
$name = isset($request->server['HTTP_USER_AGENT']) ? mb_convert_encoding($request->server['HTTP_USER_AGENT'], 'UTF-8', 'ISO-8859-1') : 'unknown browser';
try {
$sessionId = $this->session->getId();
$pwd = $this->getPassword($password);
// Make sure the current sessionId has no leftover tokens
$this->tokenProvider->invalidateToken($sessionId);
$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember);
return true;
} catch (SessionNotAvailableException $ex) {
// This can happen with OCC, where a memory session is used
// if a memory session is used, we shouldn't create a session token anyway
return false;
}
$token = $this->random->generate(32);
$pwd = $this->getPassword($password);
$this->tokenProvider->generateToken($token, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember);
$this->setMagicInCookie($uid, $token); return true;
}

/**
Expand Down Expand Up @@ -775,11 +767,10 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
*
* Invalidates the token if checks fail
*
* @param string $token
* @param string $user login name
* @return boolean
*/
private function validateToken($token, $user = null) {
private function validateToken(string $token, $user = null) {
try {
$dbToken = $this->tokenProvider->getToken($token);
} catch (InvalidTokenException $ex) {
Expand Down Expand Up @@ -858,11 +849,10 @@ public function tryTokenLogin(IRequest $request) {
* perform login using the magic cookie (remember login)
*
* @param string $uid the username
* @param string $currentToken
* @param string $oldSessionId
* @param string $sessionId
* @return bool
*/
public function loginWithCookie($uid, $currentToken, $oldSessionId) {
public function loginWithCookie($uid, $sessionId) {
$this->session->regenerateId();
$this->manager->emit('\OC\User', 'preRememberedLogin', [$uid]);
$user = $this->manager->get($uid);
Expand All @@ -871,36 +861,17 @@ public function loginWithCookie($uid, $currentToken, $oldSessionId) {
return false;
}

// get stored tokens
$tokens = $this->config->getUserKeys($uid, 'login_token');
// test cookies token against stored tokens
if (!in_array($currentToken, $tokens, true)) {
$this->logger->info('Tried to log in {uid} but could not verify token', [
'app' => 'core',
'uid' => $uid,
]);
return false;
}
// replace successfully used token with a new one
$this->config->deleteUserValue($uid, 'login_token', $currentToken);
$newToken = $this->random->generate(32);
$this->config->setUserValue($uid, 'login_token', $newToken, (string)$this->timeFactory->getTime());

try {
$sessionId = $this->session->getId();
$token = $this->tokenProvider->renewSessionToken($oldSessionId, $sessionId);
} catch (SessionNotAvailableException $ex) {
$this->logger->warning('Could not renew session token for {uid} because the session is unavailable', [
$token = $this->tokenProvider->getToken($sessionId);
} catch (InvalidTokenException $ex) {
$this->logger->warning('Could not find token for web session: ' . $ex->getMessage(), [
'app' => 'core',
'uid' => $uid,
'exception' => $ex,
]);
return false;
} catch (InvalidTokenException $ex) {
$this->logger->warning('Renewing session token failed', ['app' => 'core']);
return false;
}

$this->setMagicInCookie($user->getUID(), $newToken);
$this->setMagicInCookie($user->getUID(), $sessionId);

//login
$this->setUser($user);
Expand All @@ -923,8 +894,6 @@ public function loginWithCookie($uid, $currentToken, $oldSessionId) {
*/
public function createRememberMeToken(IUser $user) {
$token = $this->random->generate(32);
$this->config->setUserValue($user->getUID(), 'login_token', $token, (string)$this->timeFactory->getTime());
$this->setMagicInCookie($user->getUID(), $token);
}

/**
Expand Down Expand Up @@ -962,7 +931,7 @@ public function setMagicInCookie($username, $token) {

$maxAge = $this->config->getSystemValueInt('remember_login_cookie_lifetime', 60 * 60 * 24 * 15);
\OC\Http\CookieHelper::setCookie(
'nc_username',
self::COOKIE_USERNAME,
$username,
$maxAge,
$webRoot,
Expand All @@ -972,7 +941,7 @@ public function setMagicInCookie($username, $token) {
\OC\Http\CookieHelper::SAMESITE_LAX
);
\OC\Http\CookieHelper::setCookie(
'nc_token',
self::COOKIE_SESSION_ID,
$token,
$maxAge,
$webRoot,
Expand All @@ -981,20 +950,6 @@ public function setMagicInCookie($username, $token) {
true,
\OC\Http\CookieHelper::SAMESITE_LAX
);
try {
\OC\Http\CookieHelper::setCookie(
'nc_session_id',
$this->session->getId(),
$maxAge,
$webRoot,
'',
$secureCookie,
true,
\OC\Http\CookieHelper::SAMESITE_LAX
);
} catch (SessionNotAvailableException $ex) {
// ignore
}
}

/**
Expand All @@ -1004,32 +959,26 @@ public function unsetMagicInCookie() {
//TODO: DI for cookies and IRequest
$secureCookie = OC::$server->getRequest()->getServerProtocol() === 'https';

unset($_COOKIE['nc_username']); //TODO: DI
unset($_COOKIE['nc_token']);
unset($_COOKIE['nc_session_id']);
setcookie('nc_username', '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT, '', $secureCookie, true);
setcookie('nc_token', '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT, '', $secureCookie, true);
setcookie('nc_session_id', '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT, '', $secureCookie, true);
unset($_COOKIE[self::COOKIE_USERNAME]); //TODO: DI
unset($_COOKIE[self::COOKIE_SESSION_ID]);
setcookie(self::COOKIE_USERNAME, '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT, '', $secureCookie, true);
setcookie(self::COOKIE_SESSION_ID, '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT, '', $secureCookie, true);
// old cookies might be stored under /webroot/ instead of /webroot
// and Firefox doesn't like it!
setcookie('nc_username', '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT . '/', '', $secureCookie, true);
setcookie('nc_token', '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT . '/', '', $secureCookie, true);
setcookie('nc_session_id', '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT . '/', '', $secureCookie, true);
setcookie(self::COOKIE_USERNAME, '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT . '/', '', $secureCookie, true);
setcookie(self::COOKIE_SESSION_ID, '', $this->timeFactory->getTime() - 3600, OC::$WEBROOT . '/', '', $secureCookie, true);
}

/**
* Update password of the browser session token if there is one
*
* @param string $password
*/
public function updateSessionTokenPassword($password) {
public function updateSessionTokenPassword($sessionId, $password) {
try {
$sessionId = $this->session->getId();
$token = $this->tokenProvider->getToken($sessionId);
$this->tokenProvider->setPassword($token, $sessionId, $password);
} catch (SessionNotAvailableException $ex) {
// Nothing to do
} catch (InvalidTokenException $ex) {
} catch (InvalidTokenException) {
// Nothing to do
}
}
Expand Down
Loading
Loading