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 Allowed protocols. - * @return void + * @return pair 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); }