On Tue, Jun 14, 2011 at 10:22 AM, Niklas Gustavsson <[email protected]>wrote:
> On Mon, Jun 13, 2011 at 8:31 PM, Allen Firstenberg > <[email protected]> wrote: > > First of all - we see the algorithm for PassivePorts is that it creates > an > > ordered list of ports and attempts to allocate the lowest available port. > > It does seem that there is a check to make sure a free port is actually > > available, but we're wondering if the port could be held open by a > firewall > > or client even tho the server thinks its closed. (We will be the first > to > > admit that we're grasping at straws here, but it would explain some of > what > > we've seen). > > Yes, in TCP/IP there is certainly the possibility of the client and > server disagreeing on wether a socket is open or not. What's the > actual problem you're seeing? > The client in question connects to the server, downloads a file, closes the connection, then immediately reconnects and downloads another file. (The first file contains metadata, including the name of the second file to download.) What we are seeing is that many times, tho not every time, it enters passive mode, gets the reply... and never seems to issue the RETR command. And 30 seconds or so later it quits and closes the session. If the client connects to a different server to get the metadata, it then doesn't have a problem connecting to the main server to download the file specified. We know that the data port being given in both PASV commands tends to be the same. Our working hypothesis is that something about the port is "stuck", so the second connection to that port doesn't do the right thing. We aren't ruling out that it is stuck on a firewall or on the client, and thus out of our control, but it may mean we have to figure out a reasonable workaround. > Most servers that implement linear allocation seem to always use "next > > available" instead of "lowest available" ports. A few even use "randomly > > available", which seems like it would be a lot more secure. Is there a > > reason either of these were not used? > > I don't really know since I did not write the passive ports > implementation, in any case these seem like any of these algorithms > mostly serve as way of slightly postponing the attack on the passive > port (where picking the lowest port arguable does the least to > postpone). Using something like the IP check or even using a secure > data connection and checking that the certificate matches (something > FtpServer does not currently support) would seem like a much better > solution. > > All that said, the passive ports algorithm is up for a rewrite > (https://issues.apache.org/jira/browse/FTPSERVER-360, > https://issues.apache.org/jira/browse/FTPSERVER-264) and any input or > code would be more than welcome. > What seems simplest and most efficient to me would be to maintain two ordered lists of Integers, "freePorts" and "usedPorts". The constructor would take the array of passivePorts (as it does now) and add them all to freePorts. reserveNextPort() would immediately return -1 if freePorts.size() == 0, and would otherwise remove a random element of freePorts, verify it with checkPortUnbound(), add it to usedPorts, and return it. This should also address the issues in FTPSERVER-264 in a typical case. releasePort() would verify the port is in usedPorts, remove it from there, and add it to freePorts. These two methods would synchronize on the object (see below) to avoid race conditions against the lists. There are some error conditions that need to be addressed (what if the port to release isn't in usedPorts, for example), but most of these conditions should raise warnings than cause any major problems. If desired, I can do an implementation of this. > > Finally, we noticed that PassivePorts.reserveNextPort() is not > synchronized. > > Why was this? We do note that it is called from two locations, both > > synchronized methods, although it seems like there is still a high > > possibility for a race condition here although we didn't dig closely into > > the methods in question. > > Seems like a bug to me, feel free to open an issue. > I've opened this at https://issues.apache.org/jira/browse/FTPSERVER-419 Thanks! Allen
