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

CURLOPT_FTP_SKIP_PASV_IP unusable #1455

Closed
Zenju opened this Issue Apr 28, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@Zenju

Zenju commented Apr 28, 2017

Using CURLOPT_FTP_SKIP_PASV_IP requires knowledge if the IP as returned in a server's response to PASV is routable or not. Often this information is not available before actually trying to make a connection: If the IP is not routable (e.g. 192.168.1.1) then libcurl will hang. Therefore simply falling back to adding CURLOPT_FTP_SKIP_PASV_IP only in the failure case is not a good solution. Conversely always using CURLOPT_FTP_SKIP_PASV_IP is also premature.
Other FTP clients solve this by doing a simple check if the IP in PASV's response is routable or not and only if it is not, fall back to the IP of the control connection.
Is there a reason why libcurl does not support this or has just nobody bothered to code it yet? For IPv4 it's almost trivial. Heck, I'd do it myself even, because I need it! :)
Option could be named: CURLOPT_FTP_SKIP_PASV_IP_IF_NOT_ROUTABLE

@bagder bagder added the FTP label Apr 30, 2017

@bagder

This comment has been minimized.

Member

bagder commented Apr 30, 2017

Using CURLOPT_FTP_SKIP_PASV_IP requires knowledge if the IP as returned in a server's response to PASV is routable or not

No, that's what what it requires. It basically assumes that the returned IP is useless information and that the client can connect back again to the same IP it is already connected. That's a rather bold assumption, but many FTP servers work this way still and this is how the option is documented to work so I see no fundamental problem with it.

If the IP is not routable (e.g. 192.168.1.1) then libcurl will hang

  1. If you set CURLOPT_FTP_SKIP_PASV_IP, the returned IP would be skipped and not cause a hang.
  2. Trying an IP like that will in many cases not cause a hang but a rather quick failed connection

But sure, it can be a bit annoying. Broken servers are annoying.

Other FTP clients solve this by doing a simple check if the IP in PASV's response is routable or not and only if it is not, fall back to the IP of the control connection.

How does a client determine that? And secondly, private IP addresses like that are often used in LANs so they are routable and fine, just plain wrong to connect back to if the FTP server is remote and responds with such an address.

Is there a reason why libcurl does not support this or has just nobody bothered to code it yet?

The latter. But as I mention above, being "routable" doesn't mean it works. It would probably work even better if there was a fail-safe mode that would go back and try the original IP if the one used from the response doesn't work...

@Zenju

This comment has been minimized.

Zenju commented Apr 30, 2017

No, that's what what it requires

Maybe I wasn't clear: I know what CURLOPT_FTP_SKIP_PASV_IP does, but the problem with the option is that it does not solve the requirement to only skip the IP if it is not routable, but use it otherwise. This is what other FTP clients do in this case.

How does a client determine that?

The solution other FTP clients apply is to pragmatically check for local IPs:

bool isRoutableIpV4(int ip[4])
{
	if (ip[0] == 127 || //127.0.0.0/8 (localhost)
		ip[0] == 10  || //10.0.0.0/8 (private)
		(ip[0] == 192 && ip[1] == 168) || //192.168.0.0/16 (private)
		(ip[0] == 169 && ip[1] == 254) ||  //169.254.0.0/16 (link-local)
		(ip[0] == 172 && ip[1] / 16 == 1)) //172.16.0.0/12 (private)
		return false;
	return true;
}

The logic to implement would be:

"if (!isRoutableIpV4(data channel IP) && isRoutableIpV4(control channel IP))
	use control channel IP for data channel, too"

This should be safe in general and is how the biggest FTP client does it. It will fix broken servers but still be compatible witht he few that have different IPs for their control and data channels. Since the implementation is trivial, I can do it if it helps.

@bagder

This comment has been minimized.

Member

bagder commented May 3, 2017

Aha, you meant RFC1918 private addresses! Yes it would be interesting to add such a check and then have libcurl automatically ignore that returned address - if the main IP itself isn't also one of them!

Is that really a widespread problem?

@Zenju

This comment has been minimized.

Zenju commented May 5, 2017

Yes it would be interesting to add such a check

Sounds good! I've created a pull request accordingly.

Is that really a widespread problem?

Hard to come up with numbers, but shortly after I added FTP support I received a couple of bug reports that could be traced to PASV returning local IP addresses. With this rate I estimate that I would lose a lot of time dealing with support request in the future, time that I do not have. The other open source FTP clients I had a look at, all had this IP-check discussed above, so I estimate this is a recurring problem.

@bagder bagder added the help wanted label Jul 29, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 29, 2017

#1470 was closed since it was abandoned. This issue could still be worth fixing and thus I've marked it as PR-welcome.

@bagder bagder closed this in a9f6698 Nov 14, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.