Fix outgoing port on data connections in active mode

[yurivkhan]

This patch addresses the problem described in the bug
report [ 1575413 ] Active mode data connection is made
from the wrong port.

[yurivkhan]

Call SetSockOpt(..., SO_REUSEADDR, ...) before binding socket

[Tim Kosse]

Rejected. Multiple instances of FileZilla can be active at
the same time. If using SO_REUSEADDR, both instances could
listen on the same port. Acording to the MSDN library it's
not specified which instance will then get the packets.

[yurivkhan]

Correct. The SO_REUSEADDR option should only be activated if
the socket is used for an outbound connection. In a Winsock
application, one would deal with this by inserting a
setsockopt() call between the socket() and bind() only in
the back-connecting code path.

Since the CAsyncSocket architecture insists on binding the
socket in the Create() method shortly after socket creation,
I am checking the lEvent flags that indirectly indicate the
socket usage (incoming or outgoing).

+ if (0 != nSocketPort && 0 != (FD_CONNECT & lEvent))

The incoming connection sockets (the admin interface socket,
the control socket, and the passive data connection sockets)
throughout FileZilla Server are created with FD_ACCEPT and
without FD_CONNECT flag.

Since TCP connections are identified by the combination of
the local and remote address:port, all that is required for
outbound connections is that the remote address:port be
different for each connection.

Connections from different addresses will automatically be
distinct. Connections from normal FTP clients will be
distinct because they will listen to a distinct port for
each transfer.

The worst thing that could happen is, a malicious client
could establish two control connections and ask the server
to establish two data connections to the same remote
address:port by issuing a PORT command on both control
connections. In this case, the first one will succeed, and
the second one will fail with WSAEADDRINUSE. This will
automatically be handled by
CControlSocket::CreateTransferSocket with a "425 Can't open
data connection" response.

[Tim Kosse]

This will automatically be handled by
CControlSocket::CreateTransferSocket with a "425 Can't
open data connection" response.

There still should be a fallback in my opinion to use a
random port, the FTP specifications don't mandate to use
controlport-1. After all the connection can be established,
forcing the same port just artificially restricts it.

[yurivkhan]

the FTP specifications don't mandate to use controlport-1

RFC959, chapter 3.3, verse 1:

Default Data Connection Ports: All FTP implementations
must support use of the default data connection ports,
and only the User-PI may initiate the use of non-default
ports.

I interpret this as a requirement that the server must not
use non-default ports unless the client specifically asks
for that.

The next paragraph (3.3/1) describes the means that the
client may use to request non-default ports:

Negotiating Non-Default Data Ports: The User-PI may
specify a non-default user side data port with the PORT
command. The User-PI may request the server side to
identify a non-default server side data port with the PASV
command. Since a connection is defined by the pair of
addresses, either of these actions is enough to get a
different data connection, still it is permitted to do both
commands to use new ports on both ends of the data
connection.

Not sure what client software would actually issue a PASV
command immediately followed by a PORT command, though.

Many firewall configuration guides specifically recommend
that, in order to use active FTP, it is sufficient to accept
incoming connections to arbitrary ports above 1024 from port
20 only. That is, actually, the whole practical reason for
suggesting this patch -- in the default configuration
FileZilla Server is not accessible to clients behind
firewalls configured that way.

[Tim Kosse]

Yes, you're right. Sorry for being so rejecting on this, but
mentioning SO_REUSEADDR triggers quite some alarms in my
head. Improper usage of this socket flag has caused me
nothing but problems in the past.
I think I'll apply this patch, even though the benefit is
marginal.

Actually some parts of the specs are rarely implemented and
often impractical. RFC 959 is 21 years old and bases on even
earlier specs and implementations. It makes use of
assumptions about systems no longer in use and paradigms
difficult or impractical to apply to modern systems.
Take a look at sections 3.2 and 4.1.2 for example. If I
interpret them correctly, neither PORT nor PASV are needed
for transfers and furthermore, both client and server have
to remember last used PORT arguments and/or PASV replies.
Yet I don't know a single server (not to mention complete
lack of clients) complying to this part. They all require
that a PORT or PASV command gets sent.

Other, very important aspects are just left out in RFC 959,
like directory structure or directory listing format.
Machine-readability and interoperability of completely
different systems and architectures was not an issue back
then I think.

[sf-robot]

This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).

[Tim Kosse]

Applied to CVS.