mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-24 06:20:56 +01:00
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
This commit is contained in:
parent
438915032a
commit
a566ae3730
5 changed files with 56 additions and 28 deletions
|
@ -76,7 +76,8 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication {
|
|||
'(?P<action>enable|disable)/(?P<id>\d+)/' =>
|
||||
'PhabricatorAuthDisableController',
|
||||
),
|
||||
'login/(?P<pkey>[^/]+)/' => 'PhabricatorAuthLoginController',
|
||||
'login/(?P<pkey>[^/]+)/(?:(?P<extra>[^/]+)/)?' =>
|
||||
'PhabricatorAuthLoginController',
|
||||
'register/(?:(?P<akey>[^/]+)/)?' => 'PhabricatorAuthRegisterController',
|
||||
'start/' => 'PhabricatorAuthStartController',
|
||||
'validate/' => 'PhabricatorAuthValidateController',
|
||||
|
|
|
@ -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() {
|
||||
|
|
|
@ -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.'));
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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');
|
||||
|
||||
|
|
Loading…
Reference in a new issue