MAPG-142 redefine tokens and increase OAuth security with nonce

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;

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;
 }
 

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);
         }

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.'

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);
     }
 }

src/Interfaces/Session/ISessionHandler.php

@@ -0,0 +1,9 @@
+<?php namespace MapGuesser\Interfaces\Session;
+
+use SessionHandlerInterface;
+use SessionIdInterface;
+use SessionUpdateTimestampHandlerInterface;
+
+interface ISessionHandler extends SessionHandlerInterface, SessionIdInterface, SessionUpdateTimestampHandlerInterface
+{
+}

src/OAuth/GoogleOAuth.php

@@ -15,7 +15,7 @@ class GoogleOAuth
         $this->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;
         }

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 MapGuesser\Interfaces\Session\ISessionHandler;
-use SessionIdInterface;
-use SessionUpdateTimestampHandlerInterface;
 
-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();
 

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, $dialogUrlQueryParams);
-        $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']);
     }
 
     public function testCanRequestToken(): void

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)));
 }