From 40fb0f98df6c6790856b973c4e15738806be401a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 26 Mar 2015 11:12:22 -0700
Subject: [PATCH] Mostly defuse DNS rebinding attack for outbound requests

Summary: Ref T6755. I'll add some notes there about specifics.

Test Plan:
  - Made connections to HTTP and HTTPS URIs.
  - Added some debugging code to verify that HTTP URIs were pre-resolved.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6755

Differential Revision: https://secure.phabricator.com/D12169
---
 .../files/storage/PhabricatorFile.php         | 30 ++++++++++++++++---
 src/infrastructure/env/PhabricatorEnv.php     |  7 ++++-
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index bd25fe4b57..247943b26a 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -471,17 +471,39 @@ final class PhabricatorFile extends PhabricatorFileDAO
             pht('Too many redirects trying to fetch remote URI.'));
         }
 
-        PhabricatorEnv::requireValidRemoteURIForFetch(
+        $resolved = PhabricatorEnv::requireValidRemoteURIForFetch(
           $current,
           array(
             'http',
             'https',
           ));
 
-        list($status, $body, $headers) = id(new HTTPSFuture($current))
+        list($resolved_uri, $resolved_domain) = $resolved;
+
+        $current = new PhutilURI($current);
+        if ($current->getProtocol() == 'http') {
+          // For HTTP, we can use a pre-resolved URI to defuse DNS rebinding.
+          $fetch_uri = $resolved_uri;
+          $fetch_host = $resolved_domain;
+        } else {
+          // For HTTPS, we can't: cURL won't verify the SSL certificate if
+          // the domain has been replaced with an IP. But internal services
+          // presumably will not have valid certificates for rebindable
+          // domain names on attacker-controlled domains, so the DNS rebinding
+          // attack should generally not be possible anyway.
+          $fetch_uri = $current;
+          $fetch_host = null;
+        }
+
+        $future = id(new HTTPSFuture($fetch_uri))
           ->setFollowLocation(false)
-          ->setTimeout($timeout)
-          ->resolve();
+          ->setTimeout($timeout);
+
+        if ($fetch_host !== null) {
+          $future->addHeader('Host', $fetch_host);
+        }
+
+        list($status, $body, $headers) = $future->resolve();
 
         if ($status->isRedirect()) {
           // This is an HTTP 3XX status, so look for a "Location" header.
diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php
index cc804b5589..05ab7c677d 100644
--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -717,7 +717,7 @@ final class PhabricatorEnv {
    *
    * @param string URI to test.
    * @param list<string> Allowed protocols.
-   * @return void
+   * @return pair<string, string> Pre-resolved URI and domain.
    * @task uri
    */
   public static function requireValidRemoteURIForFetch(
@@ -776,6 +776,11 @@ final class PhabricatorEnv {
             $address));
       }
     }
+
+    $resolved_uri = clone $uri;
+    $resolved_uri->setDomain(head($addresses));
+
+    return array($resolved_uri, $domain);
   }