diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php --- a/src/aphront/response/AphrontRedirectResponse.php +++ b/src/aphront/response/AphrontRedirectResponse.php @@ -127,7 +127,7 @@ } // Check that it's a valid remote resource. - if (!PhabricatorEnv::isValidRemoteWebResource($uri)) { + if (!PhabricatorEnv::isValidURIForLink($uri)) { throw new Exception( pht( 'Refusing to redirect to external URI "%s". This URI '. @@ -148,7 +148,7 @@ } // If this is a local resource, it must be a valid local resource. - if (!PhabricatorEnv::isValidLocalWebResource($uri)) { + if (!PhabricatorEnv::isValidLocalURIForLink($uri)) { throw new Exception( pht( 'Refusing to redirect to local resource "%s". This URI is not '. diff --git a/src/applications/auth/controller/PhabricatorAuthFinishController.php b/src/applications/auth/controller/PhabricatorAuthFinishController.php --- a/src/applications/auth/controller/PhabricatorAuthFinishController.php +++ b/src/applications/auth/controller/PhabricatorAuthFinishController.php @@ -74,7 +74,7 @@ $request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI); $request->clearCookie(PhabricatorCookies::COOKIE_HISEC); - if (!PhabricatorEnv::isValidLocalWebResource($next)) { + if (!PhabricatorEnv::isValidLocalURIForLink($next)) { $next = '/'; } diff --git a/src/applications/auth/view/PhabricatorAuthAccountView.php b/src/applications/auth/view/PhabricatorAuthAccountView.php --- a/src/applications/auth/view/PhabricatorAuthAccountView.php +++ b/src/applications/auth/view/PhabricatorAuthAccountView.php @@ -71,7 +71,7 @@ // Make sure we don't link a "javascript:" URI if a user somehow // managed to get one here. - if (PhabricatorEnv::isValidRemoteWebResource($account_uri)) { + if (PhabricatorEnv::isValidRemoteURIForLink($account_uri)) { $account_uri = phutil_tag( 'a', array( diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -217,6 +217,9 @@ 'storage.upload-size-limit' => pht( 'Phabricator now supports arbitrarily large files. Consult the '. 'documentation for configuration details.'), + 'security.allow-outbound-http' => pht( + 'This option has been replaced with the more granular option '. + '`security.outbound-blacklist`.'), ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorSecurityConfigOptions.php b/src/applications/config/option/PhabricatorSecurityConfigOptions.php --- a/src/applications/config/option/PhabricatorSecurityConfigOptions.php +++ b/src/applications/config/option/PhabricatorSecurityConfigOptions.php @@ -25,6 +25,26 @@ $doc_href = PhabricatorEnv::getDoclink('Configuring a File Domain'); $doc_name = pht('Configuration Guide: Configuring a File Domain'); + // This is all of the IANA special/reserved blocks in IPv4 space. + $default_address_blacklist = array( + '0.0.0.0/8', + '10.0.0.0/8', + '100.64.0.0/10', + '127.0.0.0/8', + '169.254.0.0/16', + '172.16.0.0/12', + '192.0.0.0/24', + '192.0.2.0/24', + '192.88.99.0/24', + '192.168.0.0/16', + '198.18.0.0/15', + '198.51.100.0/24', + '203.0.113.0/24', + '224.0.0.0/4', + '240.0.0.0/4', + '255.255.255.255/32', + ); + return array( $this->newOption('security.alternate-file-domain', 'string', null) ->setLocked(true) @@ -210,19 +230,32 @@ "inline. This has mild security implications (you'll leak ". "referrers to YouTube) and is pretty silly (but sort of ". "awesome).")), - $this->newOption('security.allow-outbound-http', 'bool', true) - ->setBoolOptions( - array( - pht('Allow'), - pht('Disallow'), - )) + $this->newOption( + 'security.outbound-blacklist', + 'list', + $default_address_blacklist) ->setLocked(true) ->setSummary( - pht('Allow outbound HTTP requests.')) + pht( + 'Blacklist subnets to prevent user-initiated outbound '. + 'requests.')) ->setDescription( pht( - 'If you enable this, you are allowing Phabricator to '. - 'potentially make requests to external servers.')), + 'Phabricator users can make requests to other services from '. + 'the Phabricator host in some circumstances (for example, by '. + 'creating a repository with a remote URL or having Phabricator '. + 'fetch an image from a remote server).'. + "\n\n". + 'This may represent a security vulnerability if services on '. + 'the same subnet will accept commands or reveal private '. + 'information over unauthenticated HTTP GET, based on the source '. + 'IP address. In particular, all hosts in EC2 have access to '. + 'such a service.'. + "\n\n". + 'This option defines a list of netblocks which Phabricator '. + 'will decline to connect to. Generally, you should list all '. + 'private IP space here.')) + ->addExample(array('0.0.0.0/0'), pht('No Outbound Requests')), $this->newOption('security.strict-transport-security', 'bool', false) ->setLocked(true) ->setBoolOptions( diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -452,25 +452,14 @@ public static function newFromFileDownload($uri, array $params = array()) { - // Make sure we're allowed to make a request first - if (!PhabricatorEnv::getEnvConfig('security.allow-outbound-http')) { - throw new Exception('Outbound HTTP requests are disabled!'); - } - - $uri = new PhutilURI($uri); - - $protocol = $uri->getProtocol(); - switch ($protocol) { - case 'http': - case 'https': - break; - default: - // Make sure we are not accessing any file:// URIs or similar. - return null; - } + PhabricatorEnv::requireValidRemoteURIForFetch( + $uri, + array( + 'http', + 'https', + )); $timeout = 5; - list($file_data) = id(new HTTPSFuture($uri)) ->setTimeout($timeout) ->resolvex(); diff --git a/src/applications/macro/controller/PhabricatorMacroEditController.php b/src/applications/macro/controller/PhabricatorMacroEditController.php --- a/src/applications/macro/controller/PhabricatorMacroEditController.php +++ b/src/applications/macro/controller/PhabricatorMacroEditController.php @@ -33,7 +33,6 @@ $e_name = true; $e_file = null; $file = null; - $can_fetch = PhabricatorEnv::getEnvConfig('security.allow-outbound-http'); if ($request->isFormPost()) { $original = clone $macro; @@ -57,6 +56,10 @@ } } + $uri = $request->getStr('url'); + + $engine = new PhabricatorDestructionEngine(); + $file = null; if ($request->getFileExists('file')) { $file = PhabricatorFile::newFromPHPUpload( @@ -67,18 +70,40 @@ 'isExplicitUpload' => true, 'canCDN' => true, )); - } else if ($request->getStr('url') && $can_fetch) { + } else if ($uri) { try { $file = PhabricatorFile::newFromFileDownload( - $request->getStr('url'), + $uri, array( 'name' => $request->getStr('name'), - 'authorPHID' => $user->getPHID(), + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, 'isExplicitUpload' => true, 'canCDN' => true, )); + + if (!$file->isViewableInBrowser()) { + $mime_type = $file->getMimeType(); + $engine->destroyObject($file); + $file = null; + throw new Exception( + pht( + 'The URI "%s" does not correspond to a valid image file, got '. + 'a file with MIME type "%s". You must specify the URI of a '. + 'valid image file.', + $uri, + $mime_type)); + } else { + $file + ->setAuthorPHID($user->getPHID()) + ->save(); + } + } catch (HTTPFutureHTTPResponseStatus $status) { + $errors[] = pht( + 'The URI "%s" could not be loaded, got %s error.', + $uri, + $status->getStatusCode()); } catch (Exception $ex) { - $errors[] = pht('Could not fetch URL: %s', $ex->getMessage()); + $errors[] = $ex->getMessage(); } } else if ($request->getStr('phid')) { $file = id(new PhabricatorFileQuery()) @@ -175,14 +200,12 @@ $other_label = pht('File'); } - if ($can_fetch) { - $form->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('URL')) - ->setName('url') - ->setValue($request->getStr('url')) - ->setError($request->getFileExists('file') ? false : $e_file)); - } + $form->appendChild( + id(new AphrontFormTextControl()) + ->setLabel(pht('URL')) + ->setName('url') + ->setValue($request->getStr('url')) + ->setError($request->getFileExists('file') ? false : $e_file)); $form->appendChild( id(new AphrontFormFileControl()) @@ -226,13 +249,11 @@ ->setEncType('multipart/form-data') ->setUser($request->getUser()); - if ($can_fetch) { - $upload_form->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('URL')) - ->setName('url') - ->setValue($request->getStr('url'))); - } + $upload_form->appendChild( + id(new AphrontFormTextControl()) + ->setLabel(pht('URL')) + ->setName('url') + ->setValue($request->getStr('url'))); $upload_form ->appendChild( diff --git a/src/applications/mailinglists/controller/PhabricatorMailingListsEditController.php b/src/applications/mailinglists/controller/PhabricatorMailingListsEditController.php --- a/src/applications/mailinglists/controller/PhabricatorMailingListsEditController.php +++ b/src/applications/mailinglists/controller/PhabricatorMailingListsEditController.php @@ -54,7 +54,7 @@ } if ($list->getURI()) { - if (!PhabricatorEnv::isValidWebResource($list->getURI())) { + if (!PhabricatorEnv::isValidRemoteURIForLink($list->getURI())) { $e_uri = pht('Invalid'); $errors[] = pht('Mailing list URI must point to a valid web page.'); } diff --git a/src/applications/oauthserver/PhabricatorOAuthServer.php b/src/applications/oauthserver/PhabricatorOAuthServer.php --- a/src/applications/oauthserver/PhabricatorOAuthServer.php +++ b/src/applications/oauthserver/PhabricatorOAuthServer.php @@ -198,7 +198,7 @@ * for details on what makes a given redirect URI "valid". */ public function validateRedirectURI(PhutilURI $uri) { - if (!PhabricatorEnv::isValidRemoteWebResource($uri)) { + if (!PhabricatorEnv::isValidRemoteURIForLink($uri)) { return false; } diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -563,11 +563,11 @@ /** - * Detect if a URI satisfies either @{method:isValidLocalWebResource} or - * @{method:isValidRemoteWebResource}, i.e. is a page on this server or the + * Detect if a URI satisfies either @{method:isValidLocalURIForLink} or + * @{method:isValidRemoteURIForLink}, i.e. is a page on this server or the * URI of some other resource which has a valid protocol. This rejects * garbage URIs and URIs with protocols which do not appear in the - * ##uri.allowed-protocols## configuration, notably 'javascript:' URIs. + * `uri.allowed-protocols` configuration, notably 'javascript:' URIs. * * NOTE: This method is generally intended to reject URIs which it may be * unsafe to put in an "href" link attribute. @@ -576,9 +576,9 @@ * @return bool True if the URI identifies a web resource. * @task uri */ - public static function isValidWebResource($uri) { - return self::isValidLocalWebResource($uri) || - self::isValidRemoteWebResource($uri); + public static function isValidURIForLink($uri) { + return self::isValidLocalURIForLink($uri) || + self::isValidRemoteURIForLink($uri); } @@ -592,7 +592,7 @@ * @return bool True if the URI identifies a local page. * @task uri */ - public static function isValidLocalWebResource($uri) { + public static function isValidLocalURIForLink($uri) { $uri = (string)$uri; if (!strlen($uri)) { @@ -628,27 +628,167 @@ /** - * Detect if a URI identifies some valid remote resource. + * Detect if a URI identifies some valid linkable remote resource. * * @param string URI to test. * @return bool True if a URI idenfies a remote resource with an allowed * protocol. * @task uri */ - public static function isValidRemoteWebResource($uri) { - $uri = (string)$uri; - - $proto = id(new PhutilURI($uri))->getProtocol(); - if (!$proto) { + public static function isValidRemoteURIForLink($uri) { + try { + self::requireValidRemoteURIForLink($uri); + return true; + } catch (Exception $ex) { return false; } + } + + + /** + * Detect if a URI identifies a valid linkable remote resource, throwing a + * detailed message if it does not. + * + * A valid linkable remote resource can be safely linked or redirected to. + * This is primarily a protocol whitelist check. + * + * @param string URI to test. + * @return void + * @task uri + */ + public static function requireValidRemoteURIForLink($uri) { + $uri = new PhutilURI($uri); + + $proto = $uri->getProtocol(); + if (!strlen($proto)) { + throw new Exception( + pht( + 'URI "%s" is not a valid linkable resource. A valid linkable '. + 'resource URI must specify a protocol.', + $uri)); + } - $allowed = self::getEnvConfig('uri.allowed-protocols'); - if (empty($allowed[$proto])) { + $protocols = self::getEnvConfig('uri.allowed-protocols'); + if (!isset($protocols[$proto])) { + throw new Exception( + pht( + 'URI "%s" is not a valid linkable resource. A valid linkable '. + 'resource URI must use one of these protocols: %s.', + $uri, + implode(', ', array_keys($protocols)))); + } + + $domain = $uri->getDomain(); + if (!strlen($domain)) { + throw new Exception( + pht( + 'URI "%s" is not a valid linkable resource. A valid linkable '. + 'resource URI must specify a domain.', + $uri)); + } + } + + + /** + * Detect if a URI identifies a valid fetchable remote resource. + * + * @param string URI to test. + * @param list Allowed protocols. + * @return bool True if the URI is a valid fetchable remote resource. + * @task uri + */ + public static function isValidRemoteURIForFetch($uri, array $protocols) { + try { + self::requireValidRemoteURIForFetch($uri, $protocols); + return true; + } catch (Exception $ex) { return false; } + } + + + /** + * Detect if a URI identifies a valid fetchable remote resource, throwing + * a detailed message if it does not. + * + * A valid fetchable remote resource can be safely fetched using a request + * originating on this server. This is a primarily an address check against + * the outbound address blacklist. + * + * @param string URI to test. + * @param list Allowed protocols. + * @return void + * @task uri + */ + public static function requireValidRemoteURIForFetch( + $uri, + array $protocols) { + + $uri = new PhutilURI($uri); + + $proto = $uri->getProtocol(); + if (!strlen($proto)) { + throw new Exception( + pht( + 'URI "%s" is not a valid fetchable resource. A valid fetchable '. + 'resource URI must specify a protocol.', + $uri)); + } + + $protocols = array_fuse($protocols); + if (!isset($protocols[$proto])) { + throw new Exception( + pht( + 'URI "%s" is not a valid fetchable resource. A valid fetchable '. + 'resource URI must use one of these protocols: %s.', + $uri, + implode(', ', array_keys($protocols)))); + } + + $domain = $uri->getDomain(); + if (!strlen($domain)) { + throw new Exception( + pht( + 'URI "%s" is not a valid fetchable resource. A valid fetchable '. + 'resource URI must specify a domain.', + $uri)); + } + + $addresses = gethostbynamel($domain); + if (!$addresses) { + throw new Exception( + pht( + 'URI "%s" is not a valid fetchable resource. The domain "%s" could '. + 'not be resolved.', + $uri, + $domain)); + } + + foreach ($addresses as $address) { + if (self::isBlacklistedOutboundAddress($address)) { + throw new Exception( + pht( + 'URI "%s" is not a valid fetchable resource. The domain "%s" '. + 'resolves to the address "%s", which is blacklisted for '. + 'outbound requests.', + $uri, + $domain, + $address)); + } + } + } + + + /** + * Determine if an IP address is in the outbound address blacklist. + * + * @param string IP address. + * @return bool True if the address is blacklisted. + */ + public static function isBlacklistedOutboundAddress($address) { + $blacklist = self::getEnvConfig('security.outbound-blacklist'); - return true; + return PhutilCIDRList::newList($blacklist)->containsAddress($address); } public static function isClusterRemoteAddress() { diff --git a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php --- a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php +++ b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php @@ -2,7 +2,7 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase { - public function testLocalWebResource() { + public function testLocalURIForLink() { $map = array( '/' => true, '/D123' => true, @@ -19,23 +19,66 @@ foreach ($map as $uri => $expect) { $this->assertEqual( $expect, - PhabricatorEnv::isValidLocalWebResource($uri), + PhabricatorEnv::isValidLocalURIForLink($uri), "Valid local resource: {$uri}"); } } - public function testRemoteWebResource() { + public function testRemoteURIForLink() { $map = array( - 'http://example.com/' => true, - 'derp://example.com/' => false, - 'javascript:alert(1)' => false, + 'http://example.com/' => true, + 'derp://example.com/' => false, + 'javascript:alert(1)' => false, + 'http://127.0.0.1/' => true, + 'http://169.254.169.254/latest/meta-data/hostname' => true, ); foreach ($map as $uri => $expect) { $this->assertEqual( $expect, - PhabricatorEnv::isValidRemoteWebResource($uri), - "Valid remote resource: {$uri}"); + PhabricatorEnv::isValidRemoteURIForLink($uri), + "Valid linkable remote URI: {$uri}"); + } + } + + public function testRemoteURIForFetch() { + $map = array( + 'http://example.com/' => true, + + // No domain or protocol. + '' => false, + + // No domain. + 'http://' => false, + + // No protocol. + 'evil.com' => false, + + // No protocol. + '//evil.com' => false, + + // Bad protocol. + 'javascript://evil.com/' => false, + 'file:///etc/shadow' => false, + + // Unresolvable hostname. + 'http://u1VcxwUp368SIFzl7rkWWg23KM5JPB2kTHHngxjXCQc.zzz/' => false, + + // Domains explicitly in blacklisted IP space. + 'http://127.0.0.1/' => false, + 'http://169.254.169.254/latest/meta-data/hostname' => false, + + // Domain resolves into blacklisted IP space. + 'http://localhost/' => false, + ); + + $protocols = array('http', 'https'); + + foreach ($map as $uri => $expect) { + $this->assertEqual( + $expect, + PhabricatorEnv::isValidRemoteURIForFetch($uri, $protocols), + "Valid fetchable remote URI: {$uri}"); } } diff --git a/src/infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php b/src/infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php --- a/src/infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php +++ b/src/infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php @@ -71,7 +71,7 @@ } if ($item['href'] !== null) { - if (PhabricatorEnv::isValidWebResource($item['href'])) { + if (PhabricatorEnv::isValidRemoteURIForLink($item['href'])) { $tag->setHref($item['href']); $tag->setExternal(true); } diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -562,7 +562,7 @@ $name = idx($item, 'name', pht('Unnamed Footer Item')); $href = idx($item, 'href'); - if (!PhabricatorEnv::isValidWebResource($href)) { + if (!PhabricatorEnv::isValidURIForLink($href)) { $href = null; }