From a566ae37301580d0bcd188f2e80b300cda7f0014 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Feb 2014 16:39:24 -0800 Subject: [PATCH] Require a CSRF code for Twitter and JIRA (OAuth 1) logins Summary: OAuth1 doesn't have anything like the `state` parameter, and I overlooked that we need to shove one in there somewhere. Append it to the callback URI. This functions like `state` in OAuth2. Without this, an attacker can trick a user into logging into Phabricator with an account the attacker controls. Test Plan: - Logged in with JIRA. - Logged in with Twitter. - Logged in with Facebook (an OAuth2 provider). - Linked a Twitter account. - Linked a Facebook account. - Jiggered codes in URIs and verified that I got the exceptions I expected. Reviewers: btrahan, arice Reviewed By: arice CC: arice, chad, aran Differential Revision: https://secure.phabricator.com/D8318 --- .../PhabricatorApplicationAuth.php | 3 +- .../PhabricatorAuthLoginController.php | 6 ++++ .../auth/provider/PhabricatorAuthProvider.php | 34 +++++++++++++++++++ .../provider/PhabricatorAuthProviderOAuth.php | 30 ++-------------- .../PhabricatorAuthProviderOAuth1.php | 11 ++++++ 5 files changed, 56 insertions(+), 28 deletions(-) diff --git a/src/applications/auth/application/PhabricatorApplicationAuth.php b/src/applications/auth/application/PhabricatorApplicationAuth.php index 79a781fa33..f8023efd5c 100644 --- a/src/applications/auth/application/PhabricatorApplicationAuth.php +++ b/src/applications/auth/application/PhabricatorApplicationAuth.php @@ -76,7 +76,8 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication { '(?Penable|disable)/(?P\d+)/' => 'PhabricatorAuthDisableController', ), - 'login/(?P[^/]+)/' => 'PhabricatorAuthLoginController', + 'login/(?P[^/]+)/(?:(?P[^/]+)/)?' => + 'PhabricatorAuthLoginController', 'register/(?:(?P[^/]+)/)?' => 'PhabricatorAuthRegisterController', 'start/' => 'PhabricatorAuthStartController', 'validate/' => 'PhabricatorAuthValidateController', diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php index ffab2fe5bb..fd780cb1d0 100644 --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php @@ -4,6 +4,7 @@ final class PhabricatorAuthLoginController extends PhabricatorAuthController { private $providerKey; + private $extraURIData; private $provider; public function shouldRequireLogin() { @@ -12,6 +13,11 @@ final class PhabricatorAuthLoginController public function willProcessRequest(array $data) { $this->providerKey = $data['pkey']; + $this->extraURIData = idx($data, 'extra'); + } + + public function getExtraURIData() { + return $this->extraURIData; } public function processRequest() { diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index 56bc61c99b..ceb06fcfa7 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -441,4 +441,38 @@ abstract class PhabricatorAuthProvider { return null; } + protected function getAuthCSRFCode(AphrontRequest $request) { + $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID); + if (!strlen($phcid)) { + throw new Exception( + pht( + 'Your browser did not submit a "%s" cookie with client state '. + 'information in the request. Check that cookies are enabled. '. + 'If this problem persists, you may need to clear your cookies.', + PhabricatorCookies::COOKIE_CLIENTID)); + } + + return PhabricatorHash::digest($phcid); + } + + protected function verifyAuthCSRFCode(AphrontRequest $request, $actual) { + $expect = $this->getAuthCSRFCode($request); + + if (!strlen($actual)) { + throw new Exception( + pht( + 'The authentication provider did not return a client state '. + 'parameter in its response, but one was expected. If this '. + 'problem persists, you may need to clear your cookies.')); + } + + if ($actual !== $expect) { + throw new Exception( + pht( + 'The authentication provider did not return the correct client '. + 'state parameter in its response. If this problem persists, you may '. + 'need to clear your cookies.')); + } + } + } diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php index cf74ff8f46..138ad56ced 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuth.php @@ -35,9 +35,7 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { protected function renderLoginForm(AphrontRequest $request, $mode) { $adapter = $this->getAdapter(); - $adapter->setState( - PhabricatorHash::digest( - $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID))); + $adapter->setState($this->getAuthCSRFCode($request)); $scope = $request->getStr('scope'); if ($scope) { @@ -71,6 +69,8 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { return array($account, $response); } + $this->verifyAuthCSRFCode($request, $request->getStr('state')); + $code = $request->getStr('code'); if (!strlen($code)) { $response = $controller->buildProviderErrorResponse( @@ -82,30 +82,6 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider { return array($account, $response); } - if ($adapter->supportsStateParameter()) { - $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID); - if (!strlen($phcid)) { - $response = $controller->buildProviderErrorResponse( - $this, - pht( - 'Your browser did not submit a "%s" cookie with OAuth state '. - 'information in the request. Check that cookies are enabled. '. - 'If this problem persists, you may need to clear your cookies.', - PhabricatorCookies::COOKIE_CLIENTID)); - } - - $state = $request->getStr('state'); - $expect = PhabricatorHash::digest($phcid); - if ($state !== $expect) { - $response = $controller->buildProviderErrorResponse( - $this, - pht( - 'The OAuth provider did not return the correct "state" parameter '. - 'in its response. If this problem persists, you may need to clear '. - 'your cookies.')); - } - } - $adapter->setCode($code); // NOTE: As a side effect, this will cause the OAuth adapter to request diff --git a/src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php b/src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php index 3173c13aeb..1fd3df6ce6 100644 --- a/src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php +++ b/src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php @@ -55,6 +55,15 @@ abstract class PhabricatorAuthProviderOAuth1 extends PhabricatorAuthProvider { $response = null; if ($request->isHTTPPost()) { + // Add a CSRF code to the callback URI, which we'll verify when + // performing the login. + + $client_code = $this->getAuthCSRFCode($request); + + $callback_uri = $adapter->getCallbackURI(); + $callback_uri = $callback_uri.$client_code.'/'; + $adapter->setCallbackURI($callback_uri); + $uri = $adapter->getClientRedirectURI(); $response = id(new AphrontRedirectResponse())->setURI($uri); return array($account, $response); @@ -70,6 +79,8 @@ abstract class PhabricatorAuthProviderOAuth1 extends PhabricatorAuthProvider { // NOTE: You can get here via GET, this should probably be a bit more // user friendly. + $this->verifyAuthCSRFCode($request, $controller->getExtraURIData()); + $token = $request->getStr('oauth_token'); $verifier = $request->getStr('oauth_verifier');