companion: add stronger validation for urls sent via URL plugin

[ifedapoolarewaju]

this PR allows for stronger validation on URLs sent via the URL plugin (for file downloads).

It blocks URLs like:

• redis://domain.com/file-name (because of bad protocol)
• https://168.192.4.100/file-name (because hostname is not a Fully Qualified Domain Name)
• http://localhost:9000/file-name (because hostname has no TLD i.e .com suffix)

The last two above are only blocked when companion is not running in debug mode. So that testers running the app locally can test with local servers that may have such URLs.

UPDATE:
I have also added a DNS lookup to validate that download URLs are not pointing to a local IP. In which case, the request will be aborted.

A follow up PR will add an option to companion where users can set a blacklist of URLs similar to the whitelist available for upload URLs

[Acconut]

I am not sure what the purpose of blocking IPs and requiring TLDs is but that can easily be circumvented. For example, I can simply make my domain (evilshit.de) point to 127.0.0.1 and then use http://evilshit.de:8000/foo instead of http://localhost:8000/foo. Depending on your threat model, this may or may not be a problem.

[netcode]

@Acconut
For the current prevention implementation you definitely can do DNS pinning as you described.

But I believe it can be fixed using DNS resolve first before calling the url using dns.resolve , something like that

dns.resolve(url, (err,dnsRecords) => {
   if(err) return; //error in dns resolve
   for (const record of dnsRecords) {
      //compare the record with IPS blacklisted 
      //ex: 127.0.0.1, 0.0.0.0 , some internal services .. etc
      //if success mark this url as infected
   }
});

What do you think ?

[ifedapoolarewaju]

@Acconut @netcode I have added a DNS lookup to validate that download URLs are not pointing to a local IP. In which case, the request will be aborted. Tested with LOCAL_IP_ADDRESS.xip.io. But please let me know what you think.

[goto-bus-stop]

We don't need the redirect prevention and tld filtering anymore if we already do filtering at the DNS level right? There are legitimate use cases for redirects and loading files from IPs

[netcode]

@goto-bus-stop The redirection is essential , because I can host script that redirect to the IP

evilwebsite.com/index.php

<?php
header('Location: http://127.0.0.1');

[goto-bus-stop]

@netcode The current implementation in this PR does the check after each DNS lookup, including when following redirects (instead of doing a separate lookup first). So if we prevent private IPs at that stage it should automatically address malicious redirects, while mundane ones still work, right?

[Acconut]

I am not going to comment much on the Uppy code itself, but the IP filtering using the HTTP agent looks amazing! Would be nice to have a test for this, though!

[ifedapoolarewaju]

@netcode @goto-bus-stop @Acconut I tested with redirects enabled and it's true what @goto-bus-stop says, the dns lookup (validation) is done on every URL including the URL redirected to. So we can indeed enable redirects

There are legitimate use cases for loading files from IPs

However, if we enable IPs (i.e if we remove the FQDN validation), then it doesn't do a dns lookup, hence the validation is skipped. Except we try to do a validation on the higher level to see if the URL is an IP, and then we'd validate that it's not a private IP. But I'm not sure if there are other risks involved by allowing this. I'm also not sure how far we are willing to go to allow this.

[Acconut]

we try to do a validation on the higher level to see if the URL is an IP, and then we'd validate that it's not a private IP

👍 for that

[goto-bus-stop]

Yeah in that case we can also do that check inside createConnection directly right? since it will be called again for the redirect but with an IP as the hostname?

[ifedapoolarewaju]

I have updated the PR to allow IP addresses and redirects, but validate that the IP address is not a private one.

[kvz]

I don't believe this to be correct, i think there are actual WAN addresses that start with 10 and the 10. range should be more discriminatory

[kvz]

i just checked but couldn't find any such claims online so it seems i was wrong

[goto-bus-stop]

looks like we're missing an @types/ dependency for something?

[goto-bus-stop]

Oh. please remove the packages/@uppy/companion/package-lock.json and do an npm install in the repo root, then it should add ip-address to the root package-lock.json instead. it's kinda annoying that this is what we have to do but oh well 🙃

[goto-bus-stop]

save for CI and the package-lock, this LGTM!

(can we also add a test?)

[netcode]

Looks good to me