From 4f8147dbb8c0b993b6024215bdf9695b0694efc9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 24 Mar 2015 18:49:01 -0700
Subject: [PATCH] Improve protection against SSRF attacks

Summary:
Ref T6755. This improves our resistance to SSRF attacks:

  - Follow redirects manually and verify each component of the redirect chain.
  - Handle authentication provider profile picture fetches more strictly.

Test Plan:
  - Tried to download macros from various URIs which issued redirects, etc.
  - Downloaded an actual macro.
  - Went through external account workflow.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12151
---
 .../auth/provider/PhabricatorAuthProvider.php | 14 ++-
 .../files/storage/PhabricatorFile.php         | 97 ++++++++++++++++---
 2 files changed, 93 insertions(+), 18 deletions(-)

diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php
index ef3c024bb0..b1b064c363 100644
--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -247,13 +247,19 @@ abstract class PhabricatorAuthProvider {
             $image_uri,
             array(
               'name' => $name,
-              'canCDN' => true,
+              'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
             ));
+          if ($image_file->isViewableImage()) {
+            $image_file
+              ->setViewPolicy(PhabricatorPolicies::getMostOpenPolicy())
+              ->setCanCDN(true)
+              ->save();
+            $account->setProfileImagePHID($image_file->getPHID());
+          } else {
+            $image_file->delete();
+          }
         unset($unguarded);
 
-        if ($image_file) {
-          $account->setProfileImagePHID($image_file->getPHID());
-        }
       } catch (Exception $ex) {
         // Log this but proceed, it's not especially important that we
         // be able to pull profile images.
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index 65f606e76a..bd25fe4b57 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -451,24 +451,93 @@ final class PhabricatorFile extends PhabricatorFileDAO
   }
 
 
+  /**
+   * Download a remote resource over HTTP and save the response body as a file.
+   *
+   * This method respects `security.outbound-blacklist`, and protects against
+   * HTTP redirection (by manually following "Location" headers and verifying
+   * each destination). It does not protect against DNS rebinding. See
+   * discussion in T6755.
+   */
   public static function newFromFileDownload($uri, array $params = array()) {
-    PhabricatorEnv::requireValidRemoteURIForFetch(
-      $uri,
-      array(
-        'http',
-        'https',
-      ));
-
     $timeout = 5;
-    list($file_data) = id(new HTTPSFuture($uri))
-        ->setTimeout($timeout)
-        ->resolvex();
 
-    $params = $params + array(
-      'name' => basename($uri),
-    );
+    $redirects = array();
+    $current = $uri;
+    while (true) {
+      try {
+        if (count($redirects) > 10) {
+          throw new Exception(
+            pht('Too many redirects trying to fetch remote URI.'));
+        }
 
-    return self::newFromFileData($file_data, $params);
+        PhabricatorEnv::requireValidRemoteURIForFetch(
+          $current,
+          array(
+            'http',
+            'https',
+          ));
+
+        list($status, $body, $headers) = id(new HTTPSFuture($current))
+          ->setFollowLocation(false)
+          ->setTimeout($timeout)
+          ->resolve();
+
+        if ($status->isRedirect()) {
+          // This is an HTTP 3XX status, so look for a "Location" header.
+          $location = null;
+          foreach ($headers as $header) {
+            list($name, $value) = $header;
+            if (phutil_utf8_strtolower($name) == 'location') {
+              $location = $value;
+              break;
+            }
+          }
+
+          // HTTP 3XX status with no "Location" header, just treat this like
+          // a normal HTTP error.
+          if ($location === null) {
+            throw $status;
+          }
+
+          if (isset($redirects[$location])) {
+            throw new Exception(
+              pht(
+                'Encountered loop while following redirects.'));
+          }
+
+          $redirects[$location] = $location;
+          $current = $location;
+          // We'll fall off the bottom and go try this URI now.
+        } else if ($status->isError()) {
+          // This is something other than an HTTP 2XX or HTTP 3XX status, so
+          // just bail out.
+          throw $status;
+        } else {
+          // This is HTTP 2XX, so use the the response body to save the
+          // file data.
+          $params = $params + array(
+            'name' => basename($uri),
+          );
+
+          return self::newFromFileData($body, $params);
+        }
+      } catch (Exception $ex) {
+        if ($redirects) {
+          throw new PhutilProxyException(
+            pht(
+              'Failed to fetch remote URI "%s" after following %s redirect(s) '.
+              '(%s): %s',
+              $uri,
+              new PhutilNumber(count($redirects)),
+              implode(' > ', array_keys($redirects)),
+              $ex->getMessage()),
+            $ex);
+        } else {
+          throw $ex;
+        }
+      }
+    }
   }
 
   public static function normalizeFileName($file_name) {