From 5edec45531d0c05bfbbe879b48e8f4066ebc5722 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 20 Sep 2023 20:05:22 +0200 Subject: [PATCH] feat: Refined session handling * Remove secondary persistent remember me tokens * Decouple session ID and session token Signed-off-by: Christoph Wurst --- .../Controller/ChangePasswordController.php | 5 +- core/Controller/LoginController.php | 4 - lib/base.php | 12 +- lib/private/AppFramework/Http/Request.php | 2 +- .../Login/FinishRememberedLoginCommand.php | 2 +- .../Authentication/Token/IProvider.php | 11 -- lib/private/Authentication/Token/Manager.php | 16 --- .../Token/PublicKeyTokenProvider.php | 31 ----- .../Authentication/TwoFactorAuth/Manager.php | 3 +- lib/private/Session/Internal.php | 9 +- lib/private/User/Session.php | 111 +++++------------- lib/private/legacy/OC_User.php | 1 - 12 files changed, 45 insertions(+), 162 deletions(-) diff --git a/apps/settings/lib/Controller/ChangePasswordController.php b/apps/settings/lib/Controller/ChangePasswordController.php index 10ad11935c652..2b7d1a1ccd9f7 100644 --- a/apps/settings/lib/Controller/ChangePasswordController.php +++ b/apps/settings/lib/Controller/ChangePasswordController.php @@ -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', diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index af43f2d4c4ac5..2c65049f66b08 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -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( diff --git a/lib/base.php b/lib/base.php index 26e88979a483b..251bc204de8a2 100644 --- a/lib/base.php +++ b/lib/base.php @@ -420,8 +420,8 @@ public static function initSession(): void { // 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; @@ -1142,9 +1142,11 @@ public static function handleLogin(OCP\IRequest $request): bool { 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))) { diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php index 408e88583a070..77118b7efdbc2 100644 --- a/lib/private/AppFramework/Http/Request.php +++ b/lib/private/AppFramework/Http/Request.php @@ -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; } diff --git a/lib/private/Authentication/Login/FinishRememberedLoginCommand.php b/lib/private/Authentication/Login/FinishRememberedLoginCommand.php index 56ea042a66237..c1d755e445318 100644 --- a/lib/private/Authentication/Login/FinishRememberedLoginCommand.php +++ b/lib/private/Authentication/Login/FinishRememberedLoginCommand.php @@ -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); diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index a12d3ba34d993..75310e7743bab 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -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 * diff --git a/lib/private/Authentication/Token/Manager.php b/lib/private/Authentication/Token/Manager.php index 6a1c7d4c1e7a5..70751333a4449 100644 --- a/lib/private/Authentication/Token/Manager.php +++ b/lib/private/Authentication/Token/Manager.php @@ -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 diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index 3fb1161107608..c3d49e5bbe532 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -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(); diff --git a/lib/private/Authentication/TwoFactorAuth/Manager.php b/lib/private/Authentication/TwoFactorAuth/Manager.php index ff0c33445a2ad..cc5d9d09d7a04 100644 --- a/lib/private/Authentication/TwoFactorAuth/Manager.php +++ b/lib/private/Authentication/TwoFactorAuth/Manager.php @@ -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); diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index e8e2a4f2d8e43..78de37a309824 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -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?! } } diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 82887f8d02967..7f1be9ff3f4a7 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -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; @@ -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); } else { $token = $appPassword; } - if (!$this->validateToken($token)) { + if ($token === null || !$this->validateToken($token)) { // Session was invalidated $this->logout(); } @@ -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; } @@ -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; } /** @@ -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) { @@ -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); @@ -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); @@ -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); } /** @@ -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, @@ -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, @@ -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 - } } /** @@ -1004,17 +959,14 @@ 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); } /** @@ -1022,14 +974,11 @@ public function unsetMagicInCookie() { * * @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 } } diff --git a/lib/private/legacy/OC_User.php b/lib/private/legacy/OC_User.php index caa4f5dca6512..2a4989daf2f33 100644 --- a/lib/private/legacy/OC_User.php +++ b/lib/private/legacy/OC_User.php @@ -192,7 +192,6 @@ public static function loginWithApache(\OCP\Authentication\IApacheBackend $backe $dispatcher->dispatchTyped(new BeforeUserLoggedInEvent($uid, $password, $backend)); $userSession->createSessionToken($request, $uid, $uid, $password); - $userSession->createRememberMeToken($userSession->getUser()); // setup the filesystem OC_Util::setupFS($uid); // first call the post_login hooks, the login-process needs to be