From 6a4fe4500ddd72ad4e826d9d63b2d69512bd10d1 Mon Sep 17 00:00:00 2001 From: Justin Bull Date: Sat, 17 Aug 2019 12:08:08 -0400 Subject: [PATCH] Enforce/verify state parameter of callback This fixes a security vulnerability where a malicious actor can bypass authentication via a clickjacking attack (CSRF vulnerability). Signed-off-by: schema --- SpecialOAuth2Client.php | 16 +++++++++++++--- extension.json | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/SpecialOAuth2Client.php b/SpecialOAuth2Client.php index 0c71116..a2c32e8 100644 --- a/SpecialOAuth2Client.php +++ b/SpecialOAuth2Client.php @@ -92,17 +92,27 @@ private function _redirect() { } private function _handleCallback(){ + global $wgRequest; + try { + $storedState = $wgRequest->getSession()->get('oauth2state'); + // Enforce the `state` parameter to prevent clickjacking/CSRF + if(isset($storedState) && $storedState != $_GET['state']) { + if(isset($_GET['state'])) { + throw new UnexpectedValueException("State parameter of callback does not match original state"); + } else { + throw new UnexpectedValueException("Required state parameter missing"); + } + } // Try to get an access token using the authorization code grant. $accessToken = $this->_provider->getAccessToken('authorization_code', [ 'code' => $_GET['code'] ]); } catch (\League\OAuth2\Client\Provider\Exception\IdentityProviderException $e) { - - // Failed to get the access token or user details. + exit($e->getMessage()); // Failed to get the access token or user details. + } catch (UnexpectedValueException $e) { exit($e->getMessage()); - } $resourceOwner = $this->_provider->getResourceOwner($accessToken); diff --git a/extension.json b/extension.json index 1619bed..e11dc35 100644 --- a/extension.json +++ b/extension.json @@ -1,6 +1,6 @@ { "name": "MW-OAuth2Client", - "version": "0.3", + "version": "0.4", "author": [ "[http://dekeijzer.org Joost de Keijzer]", "[https://www.mediawiki.org/wiki/User:Nischayn22 Nischay Nahata]",