Possible problem pertaining passive ports

[Allen Firstenberg <pr...@addventure.com>]

We've been looking into sporadic problems one of the users of the FTP server
is having, and have run into a scenario with passive ports that has us
asking more questions than answers.

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).

In researching this, however, we're VERY concerned by the algorithm used.
 Any algorithm that offers predictably guessable unsecured open ports seems
like a major security issue - does Apache FTP mitigate this by ensuring that
the same client address is used to connect to both the control and data
ports?

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?

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.

We are tempted to rewrite the PassivePorts class to address these problems.
 Before we do - is anyone else looking into these issues or have other
concerns along this front?

Allen

Re: [proftpd] Re: Re: Re: Possible problem pertaining passive ports

[Allen Firstenberg <pr...@addventure.com>]

I have opened this at
https://issues.apache.org/jira/browse/FTPSERVER-420and attached two
diff files reflecting the required changes to implement
"random port".  I've also included the full PassivePorts.java file, in case
that was easier to work with.

Allen

On Thu, Jun 16, 2011 at 5:49 AM, Niklas Gustavsson <ni...@protocol7.com>wrote:

> On Wed, Jun 15, 2011 at 11:01 PM, Allen Firstenberg
> <pr...@addventure.com> wrote:
> >> Please do. Though, keeping a list of free ports might not be optimal
> >> if the list is long (i.e. a big span of allowed ports).
> >
> > It depends what you mean by "optimal".  In some rough tests I did, time
> to
> > create a PassivePorts object is roughly the same between the two schemes,
> > although time to allocate all 65535 possible ports is 2-3 times slower
> under
> > the old scheme.  Memory is the opposite - my proposed scheme uses 3-4
> times
> > the amount of memory the old scheme uses, mostly because of the need to
> > allocate the Integer objects.  An implementation that used ints instead
> of
> > Integers could probably get the memory size to be roughly the same.
>
> Let's not worry about this prematurely. When you're done, let's have a
> look at the code instead.
>
> /niklas
>

Re: [proftpd] Re: Re: Possible problem pertaining passive ports

[Niklas Gustavsson <ni...@protocol7.com>]

On Wed, Jun 15, 2011 at 11:01 PM, Allen Firstenberg
<pr...@addventure.com> wrote:
>> Please do. Though, keeping a list of free ports might not be optimal
>> if the list is long (i.e. a big span of allowed ports).
>
> It depends what you mean by "optimal".  In some rough tests I did, time to
> create a PassivePorts object is roughly the same between the two schemes,
> although time to allocate all 65535 possible ports is 2-3 times slower under
> the old scheme.  Memory is the opposite - my proposed scheme uses 3-4 times
> the amount of memory the old scheme uses, mostly because of the need to
> allocate the Integer objects.  An implementation that used ints instead of
> Integers could probably get the memory size to be roughly the same.

Let's not worry about this prematurely. When you're done, let's have a
look at the code instead.

/niklas

Re: [proftpd] Re: Re: Possible problem pertaining passive ports

[Allen Firstenberg <pr...@addventure.com>]

On Wed, Jun 15, 2011 at 8:02 AM, Niklas Gustavsson <ni...@protocol7.com>wrote:

> On Wed, Jun 15, 2011 at 12:41 AM, Allen Firstenberg
> <pr...@addventure.com> wrote:
> > What seems simplest and most efficient to me would be to maintain two
> > ordered lists of Integers, "freePorts" and "usedPorts"
>

In my rough test I changed this from an ordered list to just a list.  There
is no reason to keep the ordering after it is created.

> If desired, I can do an implementation of this.
>
> Please do. Though, keeping a list of free ports might not be optimal
> if the list is long (i.e. a big span of allowed ports).


It depends what you mean by "optimal".  In some rough tests I did, time to
create a PassivePorts object is roughly the same between the two schemes,
although time to allocate all 65535 possible ports is 2-3 times slower under
the old scheme.  Memory is the opposite - my proposed scheme uses 3-4 times
the amount of memory the old scheme uses, mostly because of the need to
allocate the Integer objects.  An implementation that used ints instead of
Integers could probably get the memory size to be roughly the same.

I know that for my typical uses, this is not a significant amount of memory,
and that this would be a reasonable tradeoff for me given the other features
this scheme would bring.  But I would certainly entertain the notion to make
this a more modular component - providing the two implementations (picking
one as a default) for the admin to choose between.

Thoughts?

Allen

Re: [proftpd] Re: Possible problem pertaining passive ports

[Niklas Gustavsson <ni...@protocol7.com>]

On Wed, Jun 15, 2011 at 12:41 AM, Allen Firstenberg
<pr...@addventure.com> wrote:
> 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.

I can see where our algorithm would lead to this.

> 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.

Please do. Though, keeping a list of free ports might not be optimal
if the list is long (i.e. a big span of allowed ports).

/niklas

Re: [proftpd] Re: Possible problem pertaining passive ports

[Allen Firstenberg <pr...@addventure.com>]

On Tue, Jun 14, 2011 at 10:22 AM, Niklas Gustavsson <ni...@protocol7.com>wrote:

> On Mon, Jun 13, 2011 at 8:31 PM, Allen Firstenberg
> <pr...@addventure.com> 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

Re: Possible problem pertaining passive ports

[Niklas Gustavsson <ni...@protocol7.com>]

On Mon, Jun 13, 2011 at 8:31 PM, Allen Firstenberg
<pr...@addventure.com> 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?

> In researching this, however, we're VERY concerned by the algorithm used.
>  Any algorithm that offers predictably guessable unsecured open ports seems
> like a major security issue - does Apache FTP mitigate this by ensuring that
> the same client address is used to connect to both the control and data
> ports?

Yes, FtpServer has a check for this, but disabled by default. Set the
ip-check attribute on the <passive> element to true. (I noted this is
missing for our documentation, which I will fix).

> 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.

> 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.

/niklas