Skip to content

tunnel FTPHTTPClient PASV data connection to the FTP host, not the proxy - #416

Open
Sahana2524 wants to merge 1 commit into
apache:masterfrom
Sahana2524:ftphttp-pasv-tunnel-host
Open

Sahana2524 wants to merge 1 commit into
apache:masterfrom
Sahana2524:ftphttp-pasv-tunnel-host

Conversation

@Sahana2524

Copy link
Copy Markdown

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

Since 3.9.0 (the CVE-2021-37533 change), _parsePassiveModeReply ignores the address in the PASV reply by default and uses _socket_.getInetAddress() instead. That is correct for FTPClient, but in FTPHTTPClient the control socket is connected to the HTTP proxy, so the substituted address is the proxy itself. _openDataConnection_ then sends:

CONNECT <proxy-address>:<port> HTTP/1.1

where the port comes from the FTP server's 227 reply. IPv4 PASV is the default path here, since EPSV is only tried for IPv6 or with setUseEPSVwithIPv4(true).

Two consequences:

  • Plain IPv4 passive transfers through FTPHTTPClient go to the wrong host by default.
  • A malicious or compromised FTP server can pick any port on the proxy's own address, so the proxy opens a connection to whatever listens there (admin or management services that are normally only reachable from the proxy host). On STOR the client streams file data into that socket. This is a server-side request forgery through the proxy (CWE-918), and the FTP server should not get to choose a destination other than itself.

The fix uses the host given to connect() (the existing tunnelHost field, already used for EPSV) as the PASV target, and keeps the PASV reply address only when the legacy ipAddressFromPasvResponse flag is set, matching what FTPClient does. tunnelHost is now set in connect() rather than in tunnelHandshake(), so a data tunnel built in legacy mode can no longer overwrite the control host that later EPSV transfers rely on.

FTPHTTPClientTest runs a small fake proxy that tunnels the control connection, answers PASV with (10,1,2,3,4,1), and records the second CONNECT line. Without the fix it is CONNECT 127.0.0.1:1025 (the proxy address); with it, CONNECT ftp.example.invalid:1025. The default mvn build passes locally on JDK 21.

Since 3.9.0, _parsePassiveModeReply ignores the PASV reply address by
default and uses the control socket's peer instead. In FTPHTTPClient
that peer is the HTTP proxy, so the data connection was tunneled with
CONNECT <proxy-address>:<port>, the port being chosen by the FTP server.

Use the FTP host given to connect() unless the legacy
ipAddressFromPasvResponse flag is set, and only record tunnelHost in
connect() so a data tunnel cannot overwrite it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant