Remaining work here is:

1. Perform these checks before pulling repository URIs.
   • I want to unify PhutilURI and PhutilGitURI before pursuing this, because the code is a big mess.
2. Perform these checks on OAuth provider profile images.
3. Consider performing these checks on other OAuth provider fetchable URIs.
4. Ensure these checks happen when we implement T4190.
5. Via @Agarri_FR, protecting against HTTP redirects.
6. Via @Agarri_FR, dealing with DNS rebinding.

For (1), in Git and Subversion, I don't think there's a way to get content back from the request. However, content does come back in Mercurial. This might mean that we have to stop surfacing Mercurial errors into the web UI.

For (2), this should be straightforward.

For (3), this is probably straightforward but may break after, e.g., T5210, since the OAuth provider may be local. The barrier to adding custom OAuth providers is normally high, so maybe it's fine to omit this check. We could also add a CLI lock for providers, which might not be a bad idea (although it's already possible to do this via policy.locked).

(4) is hypothetical future work.

(5) is probably a practical attack which we can defuse for image outbound requests by interacting with cURL and vetting URIs, although I think this may only stop the data retrieval version of the attack (I'm not sure we can abort the request in-flight if we hit a bad redirect). We can't do anything about this when git, hg, or svn initiates the request.

I'm not sure (6) is a practical attack (my assumption is that the DNS result will be cached between our gethostbynamel() call and cURL's lookup call, although I'm not sure that's true, and even if it is true today, it may not be true forever). It's an attack we can't do anything about when another process (like git, hg, or svn) initiates the request, because we do not get an opportunity to separately construct the "Host" header and the resolved host.

although I think this may only stop the data retrieval version of the attack

I suppose we can make individual non-following requests, vet the response, and then follow the request manually to stop both the "retrieve data" and "issue commands" versions of the attack.

The DNS result will be cached... depending on its TTL. Setting the TTL to 0 is enough to get a fresh request for each resolution.

D12151 protects against redirection attacks for in-process requests.

I do not believe we can protect against rebinding attacks, even for requests we issue in-process. Specifically:

• If we do not pre-resolve DNS, cURL may now (or in the future) resolve DNS itself.
• If we do pre-resolve DNS, cURL will fail to verify SSL certificates (which are never valid for IP addresses).

We could pre-resolve and succeed only by disabling SSL verification. This seems worse and more dangerous to me than allowing SSRF via DNS rebinding. After D12136, we no longer show the user any of the response body when fetching images unless it's a valid image file, so to execute a retrieval attack a user must:

1. compromise an administrative account;
2. change the image MIME types to include text or XML types;
3. have a vulnerable service present; and
4. execute a DNS rebinding attack.

We can further mitigate this by locking the MIME-type configuration options.

This library may help: https://github.com/fin1te/safecurl

"SafeCurl intends to be a drop-in replacement for the curl_exec function in PHP. SafeCurl validates each part of the URL against a white or black list, to help protect against Server-Side Request Forgery attacks"

It doesn't, it hits the same dilemma and elects to disable SSL verification. See:

https://github.com/fin1te/safecurl/blob/master/src/fin1te/SafeCurl/SafeCurl.php#L145

We may also be vulnerable to the parse_url() vs cURL parsing issues that SafeCurl encountered, however.

We may also be vulnerable to the parse_url() vs cURL parsing issues that SafeCurl encountered, however.

We don't seem to be, but D12155 adds test coverage to make that explicit.

D12169 mostly mitigates DNS rebinding:

• For HTTP URIs, we just pin DNS. This mitigates the attack.
• For HTTPS URIs, we do not pin DNS. The attack remains live.

However, we do verify HTTPS certificates, so the HTTPS rebinding attack is only useful against services with valid certificate for a domain which the attacker controls. If they try to execute "normal" DNS rebinding, this happens:

• They enter https://rebind.evil.com/.
• We lookup rebind.evil.com and get a "good" IP.
• We issue a request to https://rebind.evil.com/.
• cURL re-resolves the domain.
• The attacker's DNS server returns a "bad" IP (the IP of an internal service).
• We open the connection and begin SSL negotiation.
• SSL negotiation immediately fails because the internal service does not have a valid rebind.evil.com SSL certificate.

Some examples of services where this attack could still work might be:

• Services which have complete wildcard self-signed SSL certificates and work for any domain? I'm not sure it's possible to generate these and they seem dangerous/inadvisable for any number of reasons.
• Services at a subdomain like mail.example.com, with a certificate for *.example.com, where an attacker might be able to register and control username.example.com. But it is unlikely that they could control the actual DNS server software, so they'd have to try to win a race in order to execute the rebinding attack, and they only get 60 attempts per hour. I'm not really aware of any services like this, and this architecture is almost certainly dangerous on its own.

After D12168, D12169 and D12170, I feel like we've reasonably mitigated this attack. The remaining vectors are:

• An attacker with permission to create macros (which is broadly available by default) can use DNS rebinding + HTTPS to perform a limited port scan of private IP space, at a maximum rate of 60 requests per hour. This capability seems useless in staging a practical attack.
• An attacker with permission to create repositories (which is restricted by default) can perform a limited port scan of private IP space using HTTP, HTTPS, or SSH. We do not restrict the outbound addresses for VCS requests because it is common and reasonable to host VCS services in private IP space. The attacker can issue commands to any services in private IP space which accept commands over unauthenticated HTTP GET from the private subnet. They can issue more than 60 requests per hour, but probably not significantly more than thousands of requests per hour (this is ultimately limited by the speed of the repository update daemon). This capability seems useless in staging a practical attack, except in the presence of pathologically insecure services. It is difficult for us to meaningfully defuse this vector because we have little control over how VCS binaries interact with remote URIs.
• An attacker with permission to create authentication providers (which is restricted by default) can perform a limited port scan using HTTP or HTTPS. They can retrieve data from any service which "looks like" a legitimate OAuth provider. We do not restrict the outbound addresses for OAuth requests because it is common and reasonable to host OAuth providers in private IP space (e.g., JIRA). For example, if a service exposes passwords at the URI /plugins/servlet/oauth/access-token, the JIRA authentication provider can be used to make requests to that URI and expose the response body to the attacker. This capability might conceivably be a component in a practical attack, especially if additional OAuth providers in the future allow customization of URIs. This can most effectively be defused by locking authentication providers, which is desirable anyway. Administrators can currently do this with policy.locked, although we should provide a simpler auth lock / auth unlock mechanism. I'll file a followup task for this, but the vulnerability window seems so narrow today that I don't intend to prioritize implementing it.

Oh, and the attacks mentioned above remain live:

• An attacker who can rebind the DNS record for a domain name which some private service has a valid HTTPS certificate for (this is extremely unusual) can get more information by HTTPS port-scanning private services via macros (HTTP response codes). They are still limited to 60 requests per hour. I can't come up with a reasonable way for an attacker to gain this kind of access.
• An attacker who can do the above and update image-mime-types (this requires CLI access) can retrieve data from those services. An attacker who can do this should be able to issue the requests directly from the CLI instead.

I didn't went through a thorough analysis of the proposed patch, but the global analysis and some specific portions (like DNS rebinding) seem fine. How did you deal with HTTP redirects?

Since D12151, we don't follow "Location:" headers for these requests. If the response comes back with a 3XX code, we look for a "Location:" header, explicitly verify that the URI it specifies is OK (permitted by the outbound rules), and then repeat the process with the new URI. We abort after following too many redirects or encountering a loop.

I see a minor problem (non security related) with the handling of HTTP redirects. Depending on the exact HTTP code (ex: 303 vs 307), a POST request may need to be converted to GET when redirected: https://tools.ietf.org/html/rfc7231#section-6.4.7

The redirects we handle manually are always only GET so we shouldn't run into problems with that.