mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-22 23:02:42 +01:00
Be more strict about "Location:" redirects
Summary: Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com". - Reject local URIs with backslashes (we never generate these). - Fully-qualify all "Location:" redirects. - Require external redirects to be marked explicitly. Test Plan: - Expanded existing test coverage. - Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction). - Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected). - Went through some local redirects (login, save-an-object). - Verified file still work. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D10291
This commit is contained in:
parent
fe042def42
commit
df361470c1
10 changed files with 183 additions and 6 deletions
|
@ -76,6 +76,7 @@ phutil_register_library_map(array(
|
|||
'AphrontProgressBarView' => 'view/widget/bars/AphrontProgressBarView.php',
|
||||
'AphrontProxyResponse' => 'aphront/response/AphrontProxyResponse.php',
|
||||
'AphrontRedirectResponse' => 'aphront/response/AphrontRedirectResponse.php',
|
||||
'AphrontRedirectResponseTestCase' => 'aphront/response/__tests__/AphrontRedirectResponseTestCase.php',
|
||||
'AphrontReloadResponse' => 'aphront/response/AphrontReloadResponse.php',
|
||||
'AphrontRequest' => 'aphront/AphrontRequest.php',
|
||||
'AphrontRequestFailureView' => 'view/page/AphrontRequestFailureView.php',
|
||||
|
@ -2836,6 +2837,7 @@ phutil_register_library_map(array(
|
|||
'AphrontProgressBarView' => 'AphrontBarView',
|
||||
'AphrontProxyResponse' => 'AphrontResponse',
|
||||
'AphrontRedirectResponse' => 'AphrontResponse',
|
||||
'AphrontRedirectResponseTestCase' => 'PhabricatorTestCase',
|
||||
'AphrontReloadResponse' => 'AphrontRedirectResponse',
|
||||
'AphrontRequestFailureView' => 'AphrontView',
|
||||
'AphrontRequestTestCase' => 'PhabricatorTestCase',
|
||||
|
|
|
@ -7,6 +7,12 @@ class AphrontRedirectResponse extends AphrontResponse {
|
|||
|
||||
private $uri;
|
||||
private $stackWhenCreated;
|
||||
private $isExternal;
|
||||
|
||||
public function setIsExternal($external) {
|
||||
$this->isExternal = $external;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function __construct() {
|
||||
if ($this->shouldStopForDebugging()) {
|
||||
|
@ -21,7 +27,10 @@ class AphrontRedirectResponse extends AphrontResponse {
|
|||
}
|
||||
|
||||
public function getURI() {
|
||||
return (string)$this->uri;
|
||||
// NOTE: When we convert a RedirectResponse into an AjaxResponse, we pull
|
||||
// the URI through this method. Make sure it passes checks before we
|
||||
// hand it over to callers.
|
||||
return self::getURIForRedirect($this->uri, $this->isExternal);
|
||||
}
|
||||
|
||||
public function shouldStopForDebugging() {
|
||||
|
@ -31,7 +40,8 @@ class AphrontRedirectResponse extends AphrontResponse {
|
|||
public function getHeaders() {
|
||||
$headers = array();
|
||||
if (!$this->shouldStopForDebugging()) {
|
||||
$headers[] = array('Location', $this->uri);
|
||||
$uri = self::getURIForRedirect($this->uri, $this->isExternal);
|
||||
$headers[] = array('Location', $uri);
|
||||
}
|
||||
$headers = array_merge(parent::getHeaders(), $headers);
|
||||
return $headers;
|
||||
|
@ -85,4 +95,72 @@ class AphrontRedirectResponse extends AphrontResponse {
|
|||
return '';
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Format a URI for use in a "Location:" header.
|
||||
*
|
||||
* Verifies that a URI redirects to the expected type of resource (local or
|
||||
* remote) and formats it for use in a "Location:" header.
|
||||
*
|
||||
* The HTTP spec says "Location:" headers must use absolute URIs. Although
|
||||
* browsers work with relative URIs, we return absolute URIs to avoid
|
||||
* ambiguity. For example, Chrome interprets "Location: /\evil.com" to mean
|
||||
* "perform a protocol-relative redirect to evil.com".
|
||||
*
|
||||
* @param string URI to redirect to.
|
||||
* @param bool True if this URI identifies a remote resource.
|
||||
* @return string URI for use in a "Location:" header.
|
||||
*/
|
||||
public static function getURIForRedirect($uri, $is_external) {
|
||||
$uri_object = new PhutilURI($uri);
|
||||
if ($is_external) {
|
||||
// If this is a remote resource it must have a domain set. This
|
||||
// would also be caught below, but testing for it explicitly first allows
|
||||
// us to raise a better error message.
|
||||
if (!strlen($uri_object->getDomain())) {
|
||||
throw new Exception(
|
||||
pht(
|
||||
'Refusing to redirect to external URI "%s". This URI '.
|
||||
'is not fully qualified, and is missing a domain name. To '.
|
||||
'redirect to a local resource, remove the external flag.',
|
||||
(string)$uri));
|
||||
}
|
||||
|
||||
// Check that it's a valid remote resource.
|
||||
if (!PhabricatorEnv::isValidRemoteWebResource($uri)) {
|
||||
throw new Exception(
|
||||
pht(
|
||||
'Refusing to redirect to external URI "%s". This URI '.
|
||||
'is not a valid remote web resource.',
|
||||
(string)$uri));
|
||||
}
|
||||
} else {
|
||||
// If this is a local resource, it must not have a domain set. This allows
|
||||
// us to raise a better error message than the check below can.
|
||||
if (strlen($uri_object->getDomain())) {
|
||||
throw new Exception(
|
||||
pht(
|
||||
'Refusing to redirect to local resource "%s". The URI has a '.
|
||||
'domain, but the redirect is not marked external. Mark '.
|
||||
'redirects as external to allow redirection off the local '.
|
||||
'domain.',
|
||||
(string)$uri));
|
||||
}
|
||||
|
||||
// If this is a local resource, it must be a valid local resource.
|
||||
if (!PhabricatorEnv::isValidLocalWebResource($uri)) {
|
||||
throw new Exception(
|
||||
pht(
|
||||
'Refusing to redirect to local resource "%s". This URI is not '.
|
||||
'formatted in a recognizable way.',
|
||||
(string)$uri));
|
||||
}
|
||||
|
||||
// Fully qualify the result URI.
|
||||
$uri = PhabricatorEnv::getURI((string)$uri);
|
||||
}
|
||||
|
||||
return (string)$uri;
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -0,0 +1,63 @@
|
|||
<?php
|
||||
|
||||
final class AphrontRedirectResponseTestCase extends PhabricatorTestCase {
|
||||
|
||||
public function testLocalRedirectURIs() {
|
||||
// Format a bunch of URIs for local and remote redirection, making sure
|
||||
// we get the expected results.
|
||||
|
||||
$uris = array(
|
||||
'/a' => array(
|
||||
'http://phabricator.example.com/a',
|
||||
false,
|
||||
),
|
||||
'a' => array(
|
||||
false,
|
||||
false,
|
||||
),
|
||||
'/\\evil.com' => array(
|
||||
false,
|
||||
false,
|
||||
),
|
||||
'http://www.evil.com/' => array(
|
||||
false,
|
||||
'http://www.evil.com/',
|
||||
),
|
||||
'//evil.com' => array(
|
||||
false,
|
||||
false,
|
||||
),
|
||||
'//' => array(
|
||||
false,
|
||||
false,
|
||||
),
|
||||
'' => array(
|
||||
false,
|
||||
false,
|
||||
),
|
||||
);
|
||||
|
||||
foreach ($uris as $input => $cases) {
|
||||
foreach (array(false, true) as $idx => $is_external) {
|
||||
$expect = $cases[$idx];
|
||||
|
||||
$caught = null;
|
||||
try {
|
||||
$result = AphrontRedirectResponse::getURIForRedirect(
|
||||
$input,
|
||||
$is_external);
|
||||
} catch (Exception $ex) {
|
||||
$caught = $ex;
|
||||
}
|
||||
|
||||
if ($expect === false) {
|
||||
$this->assertTrue(($caught instanceof Exception), $input);
|
||||
} else {
|
||||
$this->assertEqual(null, $caught, $input);
|
||||
$this->assertEqual($expect, $result, $input);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
}
|
|
@ -62,7 +62,9 @@ abstract class PhabricatorOAuth1AuthProvider
|
|||
$client_code,
|
||||
$adapter->getTokenSecret());
|
||||
|
||||
$response = id(new AphrontRedirectResponse())->setURI($uri);
|
||||
$response = id(new AphrontRedirectResponse())
|
||||
->setIsExternal(true)
|
||||
->setURI($uri);
|
||||
return array($account, $response);
|
||||
}
|
||||
|
||||
|
|
|
@ -74,6 +74,7 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
|
|||
// if the user can see the file, generate a token;
|
||||
// redirect to the alt domain with the token;
|
||||
return id(new AphrontRedirectResponse())
|
||||
->setIsExternal(true)
|
||||
->setURI($file->getCDNURIWithToken());
|
||||
|
||||
} else {
|
||||
|
@ -128,7 +129,9 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
|
|||
// file cannot be served via cdn, and no token given
|
||||
// redirect to the main domain to aquire a token
|
||||
|
||||
// This is marked as an "external" URI because it is fully qualified.
|
||||
return id(new AphrontRedirectResponse())
|
||||
->setIsExternal(true)
|
||||
->setURI($acquire_token_uri);
|
||||
}
|
||||
}
|
||||
|
@ -171,7 +174,10 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
|
|||
// authenticate users on the file domain. This should blunt any
|
||||
// attacks based on iframes, script tags, applet tags, etc., at least.
|
||||
// Send the user to the "info" page if they're using some other method.
|
||||
|
||||
// This is marked as "external" because it is fully qualified.
|
||||
return id(new AphrontRedirectResponse())
|
||||
->setIsExternal(true)
|
||||
->setURI(PhabricatorEnv::getProductionURI($file->getBestURI()));
|
||||
}
|
||||
$response->setMimeType($file->getMimeType());
|
||||
|
|
|
@ -116,7 +116,9 @@ final class PhortunePaypalPaymentProvider extends PhortunePaymentProvider {
|
|||
'token' => $result['TOKEN'],
|
||||
));
|
||||
|
||||
return id(new AphrontRedirectResponse())->setURI($uri);
|
||||
return id(new AphrontRedirectResponse())
|
||||
->setIsExternal(true)
|
||||
->setURI($uri);
|
||||
case 'charge':
|
||||
var_dump($_REQUEST);
|
||||
break;
|
||||
|
|
|
@ -149,7 +149,9 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider {
|
|||
// user might not end up back here. Really this needs a bunch of junk.
|
||||
|
||||
$uri = new PhutilURI($result->checkout_uri);
|
||||
return id(new AphrontRedirectResponse())->setURI($uri);
|
||||
return id(new AphrontRedirectResponse())
|
||||
->setIsExternal(true)
|
||||
->setURI($uri);
|
||||
case 'charge':
|
||||
$checkout_id = $request->getInt('checkout_id');
|
||||
$params = array(
|
||||
|
@ -195,7 +197,9 @@ final class PhortuneWePayPaymentProvider extends PhortunePaymentProvider {
|
|||
|
||||
unset($unguarded);
|
||||
|
||||
return id(new AphrontRedirectResponse())->setURI($cart_uri);
|
||||
return id(new AphrontRedirectResponse())
|
||||
->setIsExternal(true)
|
||||
->setURI($cart_uri);
|
||||
case 'cancel':
|
||||
var_dump($_REQUEST);
|
||||
break;
|
||||
|
|
15
src/infrastructure/env/PhabricatorEnv.php
vendored
15
src/infrastructure/env/PhabricatorEnv.php
vendored
|
@ -462,6 +462,21 @@ final class PhabricatorEnv {
|
|||
return false;
|
||||
}
|
||||
|
||||
// Chrome (at a minimum) interprets backslashes in Location headers and the
|
||||
// URL bar as forward slashes. This is probably intended to reduce user
|
||||
// error caused by confusion over which key is "forward slash" vs "back
|
||||
// slash".
|
||||
//
|
||||
// However, it means a URI like "/\evil.com" is interpreted like
|
||||
// "//evil.com", which is a protocol relative remote URI.
|
||||
//
|
||||
// Since we currently never generate URIs with backslashes in them, reject
|
||||
// these unconditionally rather than trying to figure out how browsers will
|
||||
// interpret them.
|
||||
if (preg_match('/\\\\/', $uri)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Valid URIs must begin with '/', followed by the end of the string or some
|
||||
// other non-'/' character. This rejects protocol-relative URIs like
|
||||
// "//evil.com/evil_stuff/".
|
||||
|
|
|
@ -13,6 +13,7 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase {
|
|||
'javascript:lol' => false,
|
||||
'' => false,
|
||||
null => false,
|
||||
'/\\evil.com' => false,
|
||||
);
|
||||
|
||||
foreach ($map as $uri => $expect) {
|
||||
|
|
|
@ -118,6 +118,10 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
|
|||
|
||||
// TODO: Remove this when we remove "releeph.installed".
|
||||
$this->env->overrideEnvConfig('releeph.installed', true);
|
||||
|
||||
$this->env->overrideEnvConfig(
|
||||
'phabricator.base-uri',
|
||||
'http://phabricator.example.com');
|
||||
}
|
||||
|
||||
protected function didRunTests() {
|
||||
|
|
Loading…
Reference in a new issue