Page MenuHomePhabricator
Files F15421990

D12136.id29178.diff
No OneTemporary
Actions

D12136.id29178.diff
View Options

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<string>',
+ $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<string> 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<string> 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;
}

File Metadata

Mime Type
text/plain
Expires
Sun, Mar 23, 4:15 AM (6 d, 2 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7382431
Default Alt Text
D12136.id29178.diff (21 KB)

Event Timeline

Phabricator · Built by Phacility