From ab23b37b97b1bbea5b23ec458972d482ac6833e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C5=91cze=20Bence?= Date: Sun, 5 Jul 2020 12:25:25 +0200 Subject: [PATCH] MAPG-142 redefine tokens and increase OAuth security with nonce --- .../20200705_0219_redefine_tokens.sql | 4 ++++ main.php | 2 +- src/Controller/LoginController.php | 17 +++++++++---- src/Controller/UserController.php | 11 +++++++-- src/Database/Query/Modify.php | 2 +- src/Interfaces/Session/ISessionHandler.php | 9 +++++++ src/OAuth/GoogleOAuth.php | 7 ++++-- src/Session/DatabaseSessionHandler.php | 24 +++++++++---------- tests/OAuth/GoogleOAuthTest.php | 12 ++++------ web.php | 7 +++++- 10 files changed, 63 insertions(+), 32 deletions(-) create mode 100644 database/migrations/structure/20200705_0219_redefine_tokens.sql create mode 100644 src/Interfaces/Session/ISessionHandler.php diff --git a/database/migrations/structure/20200705_0219_redefine_tokens.sql b/database/migrations/structure/20200705_0219_redefine_tokens.sql new file mode 100644 index 0000000..4f78b83 --- /dev/null +++ b/database/migrations/structure/20200705_0219_redefine_tokens.sql @@ -0,0 +1,4 @@ +UPDATE `sessions` SET id=SUBSTRING(id, 1, 32); + +ALTER TABLE `sessions` + MODIFY `id` varchar(32) CHARACTER SET ascii NOT NULL; diff --git a/main.php b/main.php index 401e86f..fbf78d9 100644 --- a/main.php +++ b/main.php @@ -16,7 +16,7 @@ class Container { static MapGuesser\Interfaces\Database\IConnection $dbConnection; static MapGuesser\Routing\RouteCollection $routeCollection; - static \SessionHandlerInterface $sessionHandler; + static MapGuesser\Interfaces\Session\ISessionHandler $sessionHandler; static MapGuesser\Interfaces\Request\IRequest $request; } diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 4328ee9..86a73aa 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -53,13 +53,16 @@ class LoginController public function getGoogleLoginRedirect(): IRedirect { $state = bin2hex(random_bytes(16)); + $nonce = bin2hex(random_bytes(16)); $this->request->session()->set('oauth_state', $state); + $this->request->session()->set('oauth_nonce', $nonce); $oAuth = new GoogleOAuth(new Request()); $url = $oAuth->getDialogUrl( $state, - $this->request->getBase() . '/' . \Container::$routeCollection->getRoute('login-google-action')->generateLink() + $this->request->getBase() . '/' . \Container::$routeCollection->getRoute('login-google-action')->generateLink(), + $nonce ); return new Redirect($url, IRedirect::TEMPORARY); @@ -214,16 +217,20 @@ class LoginController } $jwtParser = new JwtParser($tokenData['id_token']); - $userData = $jwtParser->getPayload(); + $idToken = $jwtParser->getPayload(); - if (!$userData['email_verified']) { + if ($idToken['nonce'] !== $this->request->session()->get('oauth_nonce')) { return new HtmlContent('login/google_login'); } - $user = $this->userRepository->getByGoogleSub($userData['sub']); + if (!$idToken['email_verified']) { + return new HtmlContent('login/google_login'); + } + + $user = $this->userRepository->getByGoogleSub($idToken['sub']); if ($user === null) { - $this->request->session()->set('google_user_data', $userData); + $this->request->session()->set('google_user_data', ['sub' => $idToken['sub'], 'email' => $idToken['email']]); return new Redirect(\Container::$routeCollection->getRoute('signup-google')->generateLink(), IRedirect::TEMPORARY); } diff --git a/src/Controller/UserController.php b/src/Controller/UserController.php index 18b4dee..f285318 100644 --- a/src/Controller/UserController.php +++ b/src/Controller/UserController.php @@ -59,14 +59,17 @@ class UserController implements ISecured $user = $this->request->user(); $state = bin2hex(random_bytes(16)); + $nonce = bin2hex(random_bytes(16)); $this->request->session()->set('oauth_state', $state); + $this->request->session()->set('oauth_nonce', $nonce); $oAuth = new GoogleOAuth(new Request()); $url = $oAuth->getDialogUrl( $state, $this->request->getBase() . '/' . \Container::$routeCollection->getRoute('account.googleAuthenticate-action')->generateLink(), + $nonce, $user->getEmail() ); @@ -95,9 +98,13 @@ class UserController implements ISecured } $jwtParser = new JwtParser($tokenData['id_token']); - $userData = $jwtParser->getPayload(); + $idToken = $jwtParser->getPayload(); - if ($userData['sub'] !== $user->getGoogleSub()) { + if ($idToken['nonce'] !== $this->request->session()->get('oauth_nonce')) { + return new HtmlContent('account/google_authenticate', ['success' => false]); + } + + if ($idToken['sub'] !== $user->getGoogleSub()) { return new HtmlContent('account/google_authenticate', [ 'success' => false, 'errorText' => 'This Google account is not linked to your account.' diff --git a/src/Database/Query/Modify.php b/src/Database/Query/Modify.php index b0a6f82..321c78c 100755 --- a/src/Database/Query/Modify.php +++ b/src/Database/Query/Modify.php @@ -135,6 +135,6 @@ class Modify private function generateKey(): string { - return substr(hash('sha256', serialize($this->attributes) . random_bytes(10) . microtime()), 0, 7); + return substr(hash('sha256', serialize($this->attributes) . random_bytes(5) . microtime()), 0, 7); } } diff --git a/src/Interfaces/Session/ISessionHandler.php b/src/Interfaces/Session/ISessionHandler.php new file mode 100644 index 0000000..dbee79e --- /dev/null +++ b/src/Interfaces/Session/ISessionHandler.php @@ -0,0 +1,9 @@ +request = $request; } - public function getDialogUrl(string $state, string $redirectUrl, ?string $loginHint = null): string + public function getDialogUrl(string $state, string $redirectUrl, ?string $nonce = null, ?string $loginHint = null): string { $oauthParams = [ 'response_type' => 'code', @@ -23,9 +23,12 @@ class GoogleOAuth 'scope' => 'openid email', 'redirect_uri' => $redirectUrl, 'state' => $state, - 'nonce' => hash('sha256', random_bytes(10) . microtime()), ]; + if ($nonce !== null) { + $oauthParams['nonce'] = $nonce; + } + if ($loginHint !== null) { $oauthParams['login_hint'] = $loginHint; } diff --git a/src/Session/DatabaseSessionHandler.php b/src/Session/DatabaseSessionHandler.php index a50a668..49f7f6f 100644 --- a/src/Session/DatabaseSessionHandler.php +++ b/src/Session/DatabaseSessionHandler.php @@ -4,11 +4,9 @@ use DateTime; use MapGuesser\Database\Query\Modify; use MapGuesser\Database\Query\Select; use MapGuesser\Interfaces\Database\IResultSet; -use SessionHandlerInterface; -use SessionIdInterface; -use SessionUpdateTimestampHandlerInterface; +use MapGuesser\Interfaces\Session\ISessionHandler; -class DatabaseSessionHandler implements SessionHandlerInterface, SessionIdInterface, SessionUpdateTimestampHandlerInterface +class DatabaseSessionHandler implements ISessionHandler { private bool $exists = false; @@ -28,7 +26,7 @@ class DatabaseSessionHandler implements SessionHandlerInterface, SessionIdInterf { $select = new Select(\Container::$dbConnection, 'sessions'); $select->columns(['data']); - $select->whereId($id); + $select->whereId(substr($id, 0, 32)); $result = $select->execute()->fetch(IResultSet::FETCH_ASSOC); @@ -46,16 +44,16 @@ class DatabaseSessionHandler implements SessionHandlerInterface, SessionIdInterf $modify = new Modify(\Container::$dbConnection, 'sessions'); if ($this->exists) { - $modify->setId($id); + $modify->setId(substr($id, 0, 32)); } else { - $modify->setExternalId($id); + $modify->setExternalId(substr($id, 0, 32)); } $modify->set('data', $data); $modify->set('updated', (new DateTime())->format('Y-m-d H:i:s')); $modify->save(); - $written = true; + $this->written = true; return true; } @@ -63,9 +61,11 @@ class DatabaseSessionHandler implements SessionHandlerInterface, SessionIdInterf public function destroy($id): bool { $modify = new Modify(\Container::$dbConnection, 'sessions'); - $modify->setId($id); + $modify->setId(substr($id, 0, 32)); $modify->delete(); + $this->exists = false; + return true; } @@ -88,12 +88,12 @@ class DatabaseSessionHandler implements SessionHandlerInterface, SessionIdInterf public function create_sid(): string { - return hash('sha256', random_bytes(10) . microtime()); + return bin2hex(random_bytes(16)); } public function validateId($id): bool { - return preg_match('/^[a-f0-9]{64}$/', $id); + return preg_match('/^[a-f0-9]{32}$/', $id); } public function updateTimestamp($id, $data): bool @@ -104,7 +104,7 @@ class DatabaseSessionHandler implements SessionHandlerInterface, SessionIdInterf $modify = new Modify(\Container::$dbConnection, 'sessions'); - $modify->setId($id); + $modify->setId(substr($id, 0, 32)); $modify->set('updated', (new DateTime())->format('Y-m-d H:i:s')); $modify->save(); diff --git a/tests/OAuth/GoogleOAuthTest.php b/tests/OAuth/GoogleOAuthTest.php index a40fc66..301c6ce 100644 --- a/tests/OAuth/GoogleOAuthTest.php +++ b/tests/OAuth/GoogleOAuthTest.php @@ -13,6 +13,7 @@ final class GoogleOAuthTest extends TestCase { $_ENV['GOOGLE_OAUTH_CLIENT_ID'] = 'xyz'; $state = 'random_state_string'; + $nonce = 'random_nonce_string'; $redirectUrl = 'http://example.com/oauth'; $requestMock = $this->getMockBuilder(IRequest::class) @@ -20,7 +21,7 @@ final class GoogleOAuthTest extends TestCase ->getMock(); $googleOAuth = new GoogleOAuth($requestMock); - $dialogUrl = $googleOAuth->getDialogUrl($state, $redirectUrl); + $dialogUrl = $googleOAuth->getDialogUrl($state, $redirectUrl, $nonce); $dialogUrlParsed = explode('?', $dialogUrl); $this->assertEquals('https://accounts.google.com/o/oauth2/v2/auth', $dialogUrlParsed[0]); @@ -33,15 +34,10 @@ final class GoogleOAuthTest extends TestCase 'scope' => 'openid email', 'redirect_uri' => $redirectUrl, 'state' => $state, - 'nonce' => hash('sha256', random_bytes(10) . microtime()), + 'nonce' => $nonce, ]; - $this->assertEquals($expectedQueryParams['response_type'], $dialogUrlQueryParams['response_type']); - $this->assertEquals($expectedQueryParams['client_id'], $dialogUrlQueryParams['client_id']); - $this->assertEquals($expectedQueryParams['scope'], $dialogUrlQueryParams['scope']); - $this->assertEquals($expectedQueryParams['redirect_uri'], $dialogUrlQueryParams['redirect_uri']); - $this->assertEquals($expectedQueryParams['state'], $dialogUrlQueryParams['state']); - $this->assertMatchesRegularExpression('/^[a-f0-9]{64}$/', $dialogUrlQueryParams['nonce']); + $this->assertEquals($expectedQueryParams, $dialogUrlQueryParams); } public function testCanRequestToken(): void diff --git a/web.php b/web.php index 5576441..6ebfecf 100644 --- a/web.php +++ b/web.php @@ -71,6 +71,11 @@ if (isset($_COOKIE['COOKIES_CONSENT'])) { 'cookie_httponly' => true, 'cookie_samesite' => 'Lax' ]); + + // this is needed to handle old type of session IDs + if (!Container::$sessionHandler->validateId(session_id())) { + session_regenerate_id(true); + } } else { $_SESSION = []; } @@ -78,5 +83,5 @@ if (isset($_COOKIE['COOKIES_CONSENT'])) { Container::$request = new MapGuesser\Request\Request($_SERVER['REQUEST_SCHEME'] . '://' . $_SERVER['HTTP_HOST'], $_GET, $_POST, $_SESSION); if (!Container::$request->session()->has('anti_csrf_token')) { - Container::$request->session()->set('anti_csrf_token', hash('sha256', random_bytes(10) . microtime())); + Container::$request->session()->set('anti_csrf_token', bin2hex(random_bytes(16))); }