upgrade commons-net to 3.10.0 due to cve #171

pjfanning · 2023-06-11T11:29:46Z

Remaining work

add config to allow commons-net FTPSClient to be used
add FTP NOTICE file to connectors-ftp jar
the LegacyFtpsClient can be greatly simplified after commons-net 3.11.0 is released - we can subclass commons-net FTPSClient and just override one method - [NET-726] Add protected getters to FTPSClient commons-net#204 makes the state that we need accessible

pjfanning · 2023-06-11T11:56:54Z

so far, this is causing problems with org.apache.pekko.stream.connectors.ftp.FtpsWithProxyStageTest

like this one:

[info] Test org.apache.pekko.stream.connectors.ftp.FtpsWithProxyStageTest.listFiles started
--> [org.apache.pekko.stream.connectors.ftp.FtpsWithProxyStageTest: listFiles] Start of log messages of test that failed with assertion failed: expected class org.apache.pekko.stream.testkit.TestSubscriber$OnNext, found class org.apache.pekko.stream.testkit.TestSubscriber$OnError (OnError(javax.net.ssl.SSLException: Unsupported or unrecognized SSL message
	at sun.security.ssl.SSLSocketInputRecord.handleUnknownRecord(SSLSocketInputRecord.java:455)
	at sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:184)
	at sun.security.ssl.SSLTransport.decode(SSLTransport.java:109)
	at sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1397)
	at sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1305)
	at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:440)
	at org.apache.commons.net.ftp.FTPSClient._openDataConnection_(FTPSClient.java:278)
	at org.apache.commons.net.ftp.FTPClient._openDataConnection_(FTPClient.java:638)
	at org.apache.commons.net.ftp.FTPClient.initiateListParsing(FTPClient.java:1988)
	at org.apache.commons.net.ftp.FTPClient.initiateListParsing(FTPClient.java:2084)
	at org.apache.commons.net.ftp.FTPClient.listFiles(FTPClient.java:2282)
	at org.apache.pekko.stream.connectors.ftp.impl.CommonFtpOperations.listFiles(CommonFtpOperations.scala:39)
	at org.apache.pekko.stream.connectors.ftp.impl.CommonFtpOperations.listFiles$(CommonFtpOperations.scala:36)
	at org.apache.pekko.stream.connectors.ftp.impl.FtpLike$$anon$2.listFiles(FtpLike.scala:80)
	at org.apache.pekko.stream.connectors.ftp.impl.CommonFtpOperations.listFiles(CommonFtpOperations.scala:73)
	at org.apache.pekko.stream.connectors.ftp.impl.CommonFtpOperations.listFiles$(CommonFtpOperations.scala:73)
	at org.apache.pekko.stream.connectors.ftp.impl.FtpLike$$anon$2.listFiles(FtpLike.scala:80)
	at org.apache.pekko.stream.connectors.ftp.impl.FtpLike$$anon$2.listFiles(FtpLike.scala:80)
	at org.apache.pekko.stream.connectors.ftp.impl.FtpBrowserGraphStage$$anon$1.getFilesFromPath(FtpBrowserGraphStage.scala:87)
	at org.apache.pekko.stream.connectors.ftp.impl.FtpBrowserGraphStage$$anon$1.initBuffer(FtpBrowserGraphStage.scala:74)
	at org.apache.pekko.stream.connectors.ftp.impl.FtpBrowserGraphStage$$anon$1.doPreStart(FtpBrowserGraphStage.scala:67)
	at org.apache.pekko.stream.connectors.ftp.impl.FtpGraphStageLogic.preStart(FtpGraphStageLogic.scala:51)

pjfanning · 2023-07-30T12:55:15Z

test issue possibly caused by changes https://issues.apache.org/jira/browse/NET-642 - see https://issues.apache.org/jira/browse/NET-718

pjfanning · 2024-01-09T10:04:33Z

Since https://issues.apache.org/jira/browse/NET-718 is not making progress - I have brought the old working code from commons-net 3.8.0 into my PR.

Aim is to add a config setting that lets users choose to use the latest commons-net FTPSClient or to use the legacy one. The legacy one will be default since we know that that works with our tests - while the change in commons-net has no tests but theoretically fixes issues for some users (https://issues.apache.org/jira/browse/NET-642)

laglangyue · 2024-01-09T12:41:09Z

lgtm

experiment to see if we relay on this dangerous option in the broken tests try a different type of proxy for FTPS (plain HTTP seems strange) Update FtpsWithProxyStageTest.java Update BaseFtpSupport.java Update BaseFtpSupport.java

nvollmar

lgtm

He-Pin

Lgtm

He-Pin · 2024-02-06T18:57:54Z

ftp/src/main/java/org/apache/pekko/stream/connectors/ftp/impl/LegacyFtpsClient.java

+     *         initialization of the connection.
+     * @throws IOException If there is any problem with the connection.
+     * @see FTPClient#_openDataConnection_(int, String)
+     * @deprecated (3.3) Use {@link FTPClient#_openDataConnection_(FTPCmd, String)} instead


Should we adopt the @SInCE version in this file ?

This is a temporary copy of the commons-net code. We can remove it when commons-net 3.11.0 is released.

The code is not public - ie the class is not public.

This is a temporary copy of the commons-net code. We can remove it when commons-net 3.11.0 is released.

How serious is this cve, can we just wait until commens-net 3.11.0 is released?

If it's that critical I would have assumed that commons-net 3.11.0 would be out by now

commons-net have assigned severity 'low' to it, NVD a 6.5 medium.

the problem was actually fixed in 3.9.0. 3.11.0 is not the fix but adds apache/commons-net#204 which apparently makes it easier to adopt this newer version in pekko-connectors

Given this, doesn't it just make sense to wait till 3.11.0 is released rather than prematurely rushing to inline the code now?

The issue is that there is a CVE in the SFTP code of commons-net, fixed in v3.9.0. Unfortunately, commons-net 3.9.0 also includes a change in the FTPS code. The contributor and some other commenters claimed that it fixed FTPS via proxy for them - without providing evidence or a test case. The equivalent test case in Akka/Pekko (FTPS via proxy) fails with the change. So this code copies in the old FTPS code and lets Pekko users choose if they want the old or new FTPS code. I submitted a fix for commons-net 3.11.0 that allows the old code to be shorter because it can extend the commons-net FTPSClient. It can't extend the commons-net FTPSClient in commons-net 3.10.0 because too much state is private. I can provide a patch to Pekko Connectors when commons-net 3.11.0 is released (which is not yet under discussion afaik).

Okay let me look into this in more detail then, I was just skimming the PR beforehand.

Roiocam

LGTM, I have read about the context of this PR, I have no problem with retaining the legacy client and set as default. But I have a question that will not block this PR, that is, the new FTPClient will not work under the proxy settings. Is this limited to testing or general? Should we mention it in the document or the release notes?

pjfanning · 2024-02-07T09:10:43Z

LGTM, I have read about the context of this PR, I have no problem with retaining the legacy client and set as default. But I have a question that will not block this PR, that is, the new FTPClient will not work under the proxy settings. Is this limited to testing or general? Should we mention it in the document or the release notes?

We will include release notes describing the new setting that controls whether to use the legacy or the new FTPS Client.

pjfanning · 2024-02-09T13:35:42Z

I logged #465 as a follow up

pjfanning marked this pull request as draft June 11, 2023 11:47

pjfanning force-pushed the commons-net-cve branch from 8fb8d43 to f036083 Compare July 30, 2023 14:09

pjfanning changed the title ~~upgrade commons-net to 3.9.0 due to cve~~ upgrade commons-net to 3.10.0 due to cve Jan 9, 2024

pjfanning added this to the 1.1.0 milestone Jan 9, 2024

pjfanning marked this pull request as ready for review January 27, 2024 20:27

pjfanning added 6 commits January 28, 2024 23:36

upgrade commons-net to 3.9.0 due to cve

5dfe534

experiment to see if we relay on this dangerous option in the broken tests try a different type of proxy for FTPS (plain HTTP seems strange) Update FtpsWithProxyStageTest.java Update BaseFtpSupport.java Update BaseFtpSupport.java

need to fork commons-net FTPSClient to bring back old, working behaviour

48db147

Update FtpsOperations.scala

ae1a332

code deprecations

640ffd7

compile issue

8b39022

Create ftpsclient.backwards.excludes

4aa4b04

pjfanning force-pushed the commons-net-cve branch from 00d2456 to 4aa4b04 Compare January 28, 2024 22:38

pjfanning added 3 commits January 28, 2024 23:41

Update Dependencies.scala

445ad88

Update FtpSourceFactory.scala

505e84f

work in progress

f33a372

pjfanning requested review from mdedetrich, He-Pin, samueleresca, Roiocam and nvollmar February 6, 2024 10:26

nvollmar approved these changes Feb 6, 2024

View reviewed changes

He-Pin approved these changes Feb 6, 2024

View reviewed changes

Roiocam approved these changes Feb 7, 2024

View reviewed changes

pjfanning merged commit fc36c8f into apache:main Feb 9, 2024
49 of 50 checks passed

pjfanning deleted the commons-net-cve branch February 9, 2024 13:29

pjfanning added the release note should be mentioned in release notes label Feb 9, 2024

pjfanning mentioned this pull request Feb 9, 2024

uptake commons-net 3.11.0 (when released) and rewrite the LegacyFtpsClient #465

Closed

pjfanning mentioned this pull request Feb 14, 2024

upgrade commons-net to 3.10.0 #169

Closed

pjfanning mentioned this pull request Jun 2, 2024

change FTP license/notice #685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade commons-net to 3.10.0 due to cve #171

upgrade commons-net to 3.10.0 due to cve #171

pjfanning commented Jun 11, 2023 •

edited

Loading

pjfanning commented Jun 11, 2023 •

edited

Loading

pjfanning commented Jul 30, 2023 •

edited

Loading

pjfanning commented Jan 9, 2024

laglangyue commented Jan 9, 2024

nvollmar left a comment

He-Pin left a comment

He-Pin Feb 6, 2024

pjfanning Feb 6, 2024

pjfanning Feb 6, 2024

mdedetrich Feb 7, 2024

raboof Feb 7, 2024

mdedetrich Feb 7, 2024

pjfanning Feb 7, 2024 •

edited

Loading

mdedetrich Feb 7, 2024

Roiocam left a comment

pjfanning commented Feb 7, 2024

pjfanning commented Feb 9, 2024

upgrade commons-net to 3.10.0 due to cve #171

upgrade commons-net to 3.10.0 due to cve #171

Conversation

pjfanning commented Jun 11, 2023 • edited Loading

pjfanning commented Jun 11, 2023 • edited Loading

pjfanning commented Jul 30, 2023 • edited Loading

pjfanning commented Jan 9, 2024

laglangyue commented Jan 9, 2024

nvollmar left a comment

Choose a reason for hiding this comment

He-Pin left a comment

Choose a reason for hiding this comment

He-Pin Feb 6, 2024

Choose a reason for hiding this comment

pjfanning Feb 6, 2024

Choose a reason for hiding this comment

pjfanning Feb 6, 2024

Choose a reason for hiding this comment

mdedetrich Feb 7, 2024

Choose a reason for hiding this comment

raboof Feb 7, 2024

Choose a reason for hiding this comment

mdedetrich Feb 7, 2024

Choose a reason for hiding this comment

pjfanning Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

mdedetrich Feb 7, 2024

Choose a reason for hiding this comment

Roiocam left a comment

Choose a reason for hiding this comment

pjfanning commented Feb 7, 2024

pjfanning commented Feb 9, 2024

pjfanning commented Jun 11, 2023 •

edited

Loading

pjfanning commented Jun 11, 2023 •

edited

Loading

pjfanning commented Jul 30, 2023 •

edited

Loading

pjfanning Feb 7, 2024 •

edited

Loading