From df361470c1d915f8423d2136feb2ae76e8d5d071 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 18 Aug 2014 14:11:06 -0700 Subject: [PATCH] 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 --- src/__phutil_library_map__.php | 2 + .../response/AphrontRedirectResponse.php | 82 ++++++++++++++++++- .../AphrontRedirectResponseTestCase.php | 63 ++++++++++++++ .../PhabricatorOAuth1AuthProvider.php | 4 +- .../PhabricatorFileDataController.php | 6 ++ .../PhortunePaypalPaymentProvider.php | 4 +- .../provider/PhortuneWePayPaymentProvider.php | 8 +- src/infrastructure/env/PhabricatorEnv.php | 15 ++++ .../env/__tests__/PhabricatorEnvTestCase.php | 1 + .../testing/PhabricatorTestCase.php | 4 + 10 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 src/aphront/response/__tests__/AphrontRedirectResponseTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5231bcdea0..19d157b55f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -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', diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php index 3ab88cace9..59864a21e8 100644 --- a/src/aphront/response/AphrontRedirectResponse.php +++ b/src/aphront/response/AphrontRedirectResponse.php @@ -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; + } + } diff --git a/src/aphront/response/__tests__/AphrontRedirectResponseTestCase.php b/src/aphront/response/__tests__/AphrontRedirectResponseTestCase.php new file mode 100644 index 0000000000..637f8aa960 --- /dev/null +++ b/src/aphront/response/__tests__/AphrontRedirectResponseTestCase.php @@ -0,0 +1,63 @@ + 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); + } + } + } + } + +} diff --git a/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php b/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php index f2caa197f9..745232a783 100644 --- a/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php +++ b/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php @@ -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); } diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php index 75b62b5dc1..8f9647e6fe 100644 --- a/src/applications/files/controller/PhabricatorFileDataController.php +++ b/src/applications/files/controller/PhabricatorFileDataController.php @@ -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()); diff --git a/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php b/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php index b2b9efd690..d90d3ae326 100644 --- a/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php +++ b/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php @@ -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; diff --git a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php index ea3ad1b548..7e44eae040 100644 --- a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php +++ b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php @@ -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; diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index 3286a7258d..8fd6efe5ec 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -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/". diff --git a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php index 5a10b833a0..a34b37ae72 100644 --- a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php +++ b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php @@ -13,6 +13,7 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase { 'javascript:lol' => false, '' => false, null => false, + '/\\evil.com' => false, ); foreach ($map as $uri => $expect) { diff --git a/src/infrastructure/testing/PhabricatorTestCase.php b/src/infrastructure/testing/PhabricatorTestCase.php index e517ffaf8c..ea03645fcf 100644 --- a/src/infrastructure/testing/PhabricatorTestCase.php +++ b/src/infrastructure/testing/PhabricatorTestCase.php @@ -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() {