1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 08:52:39 +01:00

Support "state" parameter in OAuth

Summary:
Ref T1445. Ref T1536. Although we have separate CSRF protection and have never been vulnerable to OAuth hijacking, properly implementing the "state" parameter provides a little more certainty.

Before OAuth, we set a random value on the client, and pass its hash as the "state" parameter. Upon return, validate that (a) the user has a nonempty "phcid" cookie and (b) the OAuth endpoint passed back the correct state (the hash of that cookie).

Test Plan: Logged in with all OAuth providers, which all apparently support `state`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, arice

Maniphest Tasks: T1445, T1536

Differential Revision: https://secure.phabricator.com/D6179
This commit is contained in:
epriestley 2013-06-16 10:18:56 -07:00
parent fdbd377625
commit 8c3ef4b73c
4 changed files with 33 additions and 1 deletions

View file

@ -275,6 +275,7 @@ final class AphrontRequest {
final public function clearCookie($name) {
$this->setCookie($name, '', time() - (60 * 60 * 24 * 30));
unset($_COOKIE[$name]);
}
final public function setCookie($name, $value, $expire = null) {
@ -318,6 +319,8 @@ final class AphrontRequest {
$is_secure,
$http_only = true);
$_COOKIE[$name] = $value;
return $this;
}

View file

@ -39,7 +39,12 @@ abstract class PhabricatorAuthController extends PhabricatorController {
$request->setCookie('phusr', $user->getUsername());
$request->setCookie('phsid', $session_key);
// Clear the registration key.
$request->clearCookie('phreg');
// Clear the client ID / OAuth state key.
$request->clearCookie('phcid');
}
protected function buildLoginValidateResponse(PhabricatorUser $user) {

View file

@ -62,6 +62,7 @@ final class PhabricatorAuthStartController
if (!$request->isFormPost()) {
$request->setCookie('next_uri', $next_uri);
$request->setCookie('phcid', Filesystem::readRandomCharacters(16));
}
$out = array();

View file

@ -67,7 +67,7 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider {
$panel->appendChild($form);
$adapter = $this->getAdapter();
$adapter->setState(PhabricatorHash::digest($request->getCookie('phcid')));
$uri = new PhutilURI($adapter->getAuthenticateURI());
$params = $uri->getQueryParams();
@ -113,6 +113,29 @@ abstract class PhabricatorAuthProviderOAuth extends PhabricatorAuthProvider {
return array($account, $response);
}
if ($adapter->supportsStateParameter()) {
$phcid = $request->getCookie('phcid');
if (!strlen($phcid)) {
$response = $controller->buildProviderErrorResponse(
$this,
pht(
'Your browser did not submit a "phcid" cookie with OAuth state '.
'information in the request. Check that cookies are enabled. '.
'If this problem persists, you may need to clear your cookies.'));
}
$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