mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-24 06:20:56 +01:00
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
This commit is contained in:
parent
2e72e9ff31
commit
40fb0f98df
2 changed files with 32 additions and 5 deletions
|
@ -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.
|
||||
|
|
7
src/infrastructure/env/PhabricatorEnv.php
vendored
7
src/infrastructure/env/PhabricatorEnv.php
vendored
|
@ -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);
|
||||
}
|
||||
|
||||
|
||||
|
|
Loading…
Reference in a new issue