Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DNS pinning after hosts are validated #2

Merged
merged 9 commits into from Oct 30, 2020
Merged

Conversation

webbtj
Copy link

@webbtj webbtj commented Oct 23, 2020

This is in response to issue 681 in vanilla-patches.

Update in the SafeCurl execute method using the CURLOPT_RESOLVE curl option. Prior to this fix an attacker could rebind DNS records between when the request URL is checked against the blacklist of IPs and when the curl request is executed.

Copy link

@initvector initvector left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a solid addition to the library. I'd like to get some tests around the functionality before we call it complete.

src/SafeCurl.php Outdated
if ($url && isset($url['host']) && isset($url['ips'])) {
$resolves = [];
foreach ($url['ips'] as $url_ip) {
foreach ([80, 443, 8080] as $url_port) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to put all the standard ports in here. We should be able to determine the port from the URL. We're only pinning the DNS resolution for this request, so this is looping for the sake of pinning at least two more than we need.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible for the URL to use a non-standard port. We default to these standard three, but the library could be used in such a way that additional ports could be whitelisted. As it stands, we're only pinning the three standards, so no pinning would occur for a URL with a non-standard port.

src/SafeCurl.php Outdated
}
curl_setopt($this->curlHandle, CURLOPT_RESOLVE, $resolves);
} else {
throw new Exception("Cannot resolve host.");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we already have an exception condition for this in UrlValidator.

throw new InvalidURLException("Unable to resolve host.");

If validateUrl doesn't throw any exceptions, I think it should give us everything we need. If any of the pieces are missing or invalid, exceptions are thrown as part of validating the URL.

@initvector initvector changed the title fix dns rebinding exploit by storing resolved domains and setting them in curl Add DNS pinning after hosts are validated Oct 28, 2020
Copy link

@initvector initvector left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! A couple minor tweaks and I think this'll be ready to merge.

src/CurlHandler.php Show resolved Hide resolved
src/SafeCurl.php Outdated
if ($url && isset($url['host']) && isset($url['ips'])) {
$this->setHostIPs($url);
} else {
throw new InvalidURLException("Unable to resolve host.");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is unnecessary, because if the call to validateUrl is returning anything, it'll be the full array including host and ips (it should probably be returning an object of a particular class, but that's a future improvement we can make). This should never be thrown, but if you think it's a good idea to keep it, I'm okay with leaving it in for now.

tests/SafeCurlTest.php Outdated Show resolved Hide resolved
src/SafeCurl.php Outdated
case 'http':
$port = 80;
case 'https':
$port = 443;
default:
$port = 80;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are no breaks in this switch, HTTP will be set to 80, then 443, then 80 again. That works out by coincidence. But I think HTTPS is going to be 443, then fall through to 80, which won't apply the proper DNS pinning. Once we fix that, it'll be a good candidate for a test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow... like it's my first day programming 🤦‍♂️

src/SafeCurl.php Outdated
*
* @param array $url
*/
public function setHostIPs(array $url): void {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is easier to test as a public member, but it seems like something we don't want to expose (it'll be a pain to enforce $url is actually the return value of validateUrl if used outside this class). Can it be defined as a private member, while still being testable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, just used reflection in the test to open up the method when testing.

Copy link

@tburry tburry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Ryan said.

webbtj and others added 2 commits October 30, 2020 11:18
Co-authored-by: Ryan <initvector@users.noreply.github.com>
@webbtj webbtj requested a review from tburry October 30, 2020 15:47
@tburry tburry merged commit 4552574 into master Oct 30, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants