Improve granluarity and defaults of security.allow-outbound-http

Summary:
Ref T6755. This is a partial fix, but:

  - Allow netblocks to be blacklisted instead of making the feature all-or-nothing.
  - Default to disallow requests to all reserved private/local/special IP blocks. This should generally be a "safe" setting.
  - Explain the risks better.
  - Improve the errors rasied by Macro when failing.
  - Removed `security.allow-outbound-http`, as it is superseded by this setting and is somewhat misleading.
    - We still make outbound HTTP requests to OAuth.
    - We still make outbound HTTP requests for repositories.

From a technical perspective:

  - Separate URIs that are safe to link to or redirect to (basically, not "javascript://") from URIs that are safe to fetch (nothing in a private block).
  - Add the default blacklist.
  - Be more careful with response data in Macro fetching, and don't let the user see it if it isn't ultimately valid.

Additionally:

  - I want to do this check before pulling repositories, but that's enough of a mess that it should go in a separate diff.
  - The future implementation of T4190 needs to perform the fetch check.

Test Plan:
  - Fetched a valid macro.
  - Fetched a non-image, verified it didn't result in a viewable file.
  - Fetched a private-ip-space image, got an error.
  - Fetched a 404, got a useful-enough error without additional revealing response content (which is usually HTML anyway and not useful).
  - Fetched a bad protocol, got an error.
  - Linked to a local resource, a phriction page, a valid remote site, all worked.
  - Linked to private IP space, which worked fine (we want to let you link and redierect to other private services, just not fetch them).
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12136

src/aphront/response/AphrontRedirectResponse.php

@@ -127,7 +127,7 @@ class AphrontRedirectResponse extends AphrontResponse {
       }
 
       // 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 @@ class AphrontRedirectResponse extends AphrontResponse {
       }
 
       // 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 '.

src/applications/auth/controller/PhabricatorAuthFinishController.php

@@ -74,7 +74,7 @@ final class PhabricatorAuthFinishController
     $request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
     $request->clearCookie(PhabricatorCookies::COOKIE_HISEC);
 
-    if (!PhabricatorEnv::isValidLocalWebResource($next)) {
+    if (!PhabricatorEnv::isValidLocalURIForLink($next)) {
       $next = '/';
     }
 

src/applications/auth/view/PhabricatorAuthAccountView.php

@@ -71,7 +71,7 @@ final class PhabricatorAuthAccountView extends AphrontView {
       // 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(

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

@@ -217,6 +217,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
       '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;

src/applications/config/option/PhabricatorSecurityConfigOptions.php

@@ -25,6 +25,26 @@ final class PhabricatorSecurityConfigOptions
     $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 @@ final class PhabricatorSecurityConfigOptions
             "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)
+        $this->newOption(
-          ->setBoolOptions(
+          'security.outbound-blacklist',
-            array(
+          'list<string>',
-              pht('Allow'),
+          $default_address_blacklist)
-              pht('Disallow'),
-            ))
           ->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 '.
+              'Phabricator users can make requests to other services from '.
-              'potentially make requests to external servers.')),
+              '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(

src/applications/files/storage/PhabricatorFile.php

@@ -452,25 +452,14 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
 
   public static function newFromFileDownload($uri, array $params = array()) {
-    // Make sure we're allowed to make a request first
+    PhabricatorEnv::requireValidRemoteURIForFetch(
-    if (!PhabricatorEnv::getEnvConfig('security.allow-outbound-http')) {
+      $uri,
-      throw new Exception('Outbound HTTP requests are disabled!');
+      array(
-    }
+        'http',
-
+        'https',
-    $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;
-    }
 
     $timeout = 5;
-
     list($file_data) = id(new HTTPSFuture($uri))
         ->setTimeout($timeout)
         ->resolvex();

src/applications/macro/controller/PhabricatorMacroEditController.php

@@ -33,7 +33,6 @@ final class PhabricatorMacroEditController extends PhabricatorMacroController {
     $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 @@ final class PhabricatorMacroEditController extends PhabricatorMacroController {
         }
       }
 
+      $uri = $request->getStr('url');
+
+      $engine = new PhabricatorDestructionEngine();
+
       $file = null;
       if ($request->getFileExists('file')) {
         $file = PhabricatorFile::newFromPHPUpload(
@@ -67,18 +70,40 @@ final class PhabricatorMacroEditController extends PhabricatorMacroController {
             '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 @@ final class PhabricatorMacroEditController extends PhabricatorMacroController {
         $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 AphrontFormFileControl())
@@ -226,13 +249,11 @@ final class PhabricatorMacroEditController extends PhabricatorMacroController {
         ->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(

src/applications/mailinglists/controller/PhabricatorMailingListsEditController.php

@@ -54,7 +54,7 @@ final class PhabricatorMailingListsEditController
       }
 
       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.');
         }

src/applications/oauthserver/PhabricatorOAuthServer.php

@@ -198,7 +198,7 @@ final class PhabricatorOAuthServer {
    * 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;
     }
 

src/infrastructure/env/PhabricatorEnv.php

@@ -563,11 +563,11 @@ final class PhabricatorEnv {
 
 
   /**
-   * Detect if a URI satisfies either @{method:isValidLocalWebResource} or
+   * Detect if a URI satisfies either @{method:isValidLocalURIForLink} or
-   * @{method:isValidRemoteWebResource}, i.e. is a page on this server or the
+   * @{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 @@ final class PhabricatorEnv {
    * @return bool True if the URI identifies a web resource.
    * @task uri
    */
-  public static function isValidWebResource($uri) {
+  public static function isValidURIForLink($uri) {
-    return self::isValidLocalWebResource($uri) ||
+    return self::isValidLocalURIForLink($uri) ||
-           self::isValidRemoteWebResource($uri);
+           self::isValidRemoteURIForLink($uri);
   }
 
 
@@ -592,7 +592,7 @@ final class PhabricatorEnv {
    * @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 @@ final class PhabricatorEnv {
 
 
   /**
-   * 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) {
+  public static function isValidRemoteURIForLink($uri) {
-    $uri = (string)$uri;
+    try {
-
+      self::requireValidRemoteURIForLink($uri);
-    $proto = id(new PhutilURI($uri))->getProtocol();
-    if (!$proto) {
-      return false;
-    }
-
-    $allowed = self::getEnvConfig('uri.allowed-protocols');
-    if (empty($allowed[$proto])) {
-      return false;
-    }
-
       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));
+    }
+
+    $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 PhutilCIDRList::newList($blacklist)->containsAddress($address);
   }
 
   public static function isClusterRemoteAddress() {

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 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase {
     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://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),
+        PhabricatorEnv::isValidRemoteURIForLink($uri),
-        "Valid remote resource: {$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}");
     }
   }
 

src/infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php

@@ -71,7 +71,7 @@ final class PhabricatorNavigationRemarkupRule extends PhutilRemarkupRule {
       }
 
       if ($item['href'] !== null) {
-        if (PhabricatorEnv::isValidWebResource($item['href'])) {
+        if (PhabricatorEnv::isValidRemoteURIForLink($item['href'])) {
           $tag->setHref($item['href']);
           $tag->setExternal(true);
         }

src/view/page/PhabricatorStandardPageView.php

@@ -562,7 +562,7 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView {
       $name = idx($item, 'name', pht('Unnamed Footer Item'));
 
       $href = idx($item, 'href');
-      if (!PhabricatorEnv::isValidWebResource($href)) {
+      if (!PhabricatorEnv::isValidURIForLink($href)) {
         $href = null;
       }