Re: On tomcat FTBFS.
Hi, On Thu, Aug 08, 2019 at 02:15:52PM +0200, Markus Koschany wrote: > Am 08.08.19 um 00:50 schrieb Sylvain Beucler: > > So I reworked CVE-2017-5647, which involved 5 new commits related to > > non-blocking I/O (NIO2 and COMET). > > Stable build. > > > > Then I got upstream to renew their new certs that were expiring tomorrow (!) > > https://bz.apache.org/bugzilla/show_bug.cgi?id=63648 > > and had to fix-up the SSL client tests accordingly (new client DN). > > > > At last we have a working package that passes the testsuite. > > How would you smoke-test it? > > https://www.beuc.net/tmp/debian-lts/tomcat8/ > > You can safely ignore all SSL test failures. I suggest you compare the > output of the current Tomcat release with the output after you have > fixed the newly reported CVE. If you discover new test failures > unrelated to the current ones, then it deserves further investigation. > After that you can simply run DEB_BUILD_OPTIONS=nocheck to avoid the > FTBFS. There's no more FTBFS, but I now understand how the previous uploads "passed" the test suite :) > Another option is to upgrade to the latest stable release in case > the changes are too complex and a backport is becoming more and more > time consuming. Please note that I have fixed CVE-2017-5647 2,5 years > ago as a member of the Java team. I don't believe that the new commits > are directly related to CVE-2017-5647. This appears to be a bug that was > always present and was only fixed after Jessie became stable. Well, following the advice above, I tested with and without the CVE-2017-5647 patch, and observed a regression in TestSendFile, which I fixed with the new commits. Incidentally the failures Roberto experienced at https://lists.debian.org/debian-lts/2018/07/msg00056.html were likely caused by building with no network, which seems to break a few tests requiring a fully-functional local network (I just experienced the same tests failing within pbuilder). Cheers! Sylvain
Re: On tomcat FTBFS.
Am 08.08.19 um 00:50 schrieb Sylvain Beucler: > Hi, > > So I reworked CVE-2017-5647, which involved 5 new commits related to > non-blocking I/O (NIO2 and COMET). > Stable build. > > Then I got upstream to renew their new certs that were expiring tomorrow (!) > https://bz.apache.org/bugzilla/show_bug.cgi?id=63648 > and had to fix-up the SSL client tests accordingly (new client DN). > > At last we have a working package that passes the testsuite. > How would you smoke-test it? > https://www.beuc.net/tmp/debian-lts/tomcat8/ You can safely ignore all SSL test failures. I suggest you compare the output of the current Tomcat release with the output after you have fixed the newly reported CVE. If you discover new test failures unrelated to the current ones, then it deserves further investigation. After that you can simply run DEB_BUILD_OPTIONS=nocheck to avoid the FTBFS. Another option is to upgrade to the latest stable release in case the changes are too complex and a backport is becoming more and more time consuming. Please note that I have fixed CVE-2017-5647 2,5 years ago as a member of the Java team. I don't believe that the new commits are directly related to CVE-2017-5647. This appears to be a bug that was always present and was only fixed after Jessie became stable. Testing the server works similar to testing the Apache web server. Install Tomcat and the examples and check whether simple web applications work. Find a more complex web application and configure the server to use NIO/BIO depending on what was fixed or changed regarding to the CVE. You can only reproduce certain issues if the connector is configured correctly. Bonus points if you or Abhijith send your changes to the Java team, so that we can incorporate them into Git. Regards, Markus signature.asc Description: OpenPGP digital signature
Re: On tomcat FTBFS.
Hi, So I reworked CVE-2017-5647, which involved 5 new commits related to non-blocking I/O (NIO2 and COMET). Stable build. Then I got upstream to renew their new certs that were expiring tomorrow (!) https://bz.apache.org/bugzilla/show_bug.cgi?id=63648 and had to fix-up the SSL client tests accordingly (new client DN). At last we have a working package that passes the testsuite. How would you smoke-test it? https://www.beuc.net/tmp/debian-lts/tomcat8/ (Now maybe I can start working on the actual CVEs :)) Cheers! Sylvain On 07/08/2019 12:29, Sylvain Beucler wrote: > Hi, > > It appears that the CVE-2017-5647 fix lacked this pre-requisite: > https://bz.apache.org/bugzilla/show_bug.cgi?id=57799 > https://svn.apache.org/viewvc?view=revision=1712081 > > The test case is not flacky anymore, I'm going to test full builds again. > > Cheers! > Sylvain > > On 07/08/2019 00:45, Sylvain Beucler wrote: >> Hi Markus, >> >> I'm investigating tomcat8's FTBFS and I confirm Abhijith's findings in a >> Jessie VM: >> >> - test catalina/connector/TestSendFile.java fails with nio2 connector >> but is not reliable and will report success ~1 out of 10 even with lots >> of exceptions; catalina.log will report header parsing error and return 400 >> >> - it passes reliably without CVE-2017-5647.patch >> >> - the test certificate did expire on 2019-02-27 but changing the date to >> 2019-01-01 and rebuilding does not impact these results >> (incidentally the test certs seems to depend on an external CA >> ca-test.tomcat.apache.org, fixing the certs will require switching to >> the new-style local CA in tomcat8 - if fixing the certs is needed) >> >> As you fixed CVE-2017-5647 as well as generated the last jessie upload, >> I would be interested in your take on this :) >> TestSendFile only got trivial changes, so I guess I'll look for a fix in >> later changes affecting files modified by CVE-2017-5647. >> Still, I'm surprised updates were built given this situation - did >> everybody got lucky with the flacky test or did I miss something? >> >> Cheers! >> Sylvain >> >> On 27/07/2019 20:30, Abhijith PA wrote: >>> Hi, >>> >>> >>> I don't think the link you gave on commit [fe932dd39d] is the reason for >>> FTBFS. I tried building on a VM that matches the certificate date and it >>> was successful. I also tried disabling all ssl related tests and was fine. >>> >>> While doing these all I found TestSendFile test is the culprit. In >>> CVE-2017-5647 security patch a good amount of changes is applied for >>> SendFile*.java and *Nio2*.java. These are mostly about conditions on how >>> long the socket of sendfile keep active and to take away from it. But I >>> couldn't see any those change in its test file. Please take a look on >>> the attached patch. :) >>> >>> >>> --abhijith
Re: On tomcat FTBFS.
Hi, It appears that the CVE-2017-5647 fix lacked this pre-requisite: https://bz.apache.org/bugzilla/show_bug.cgi?id=57799 https://svn.apache.org/viewvc?view=revision=1712081 The test case is not flacky anymore, I'm going to test full builds again. Cheers! Sylvain On 07/08/2019 00:45, Sylvain Beucler wrote: > Hi Markus, > > I'm investigating tomcat8's FTBFS and I confirm Abhijith's findings in a > Jessie VM: > > - test catalina/connector/TestSendFile.java fails with nio2 connector > but is not reliable and will report success ~1 out of 10 even with lots > of exceptions; catalina.log will report header parsing error and return 400 > > - it passes reliably without CVE-2017-5647.patch > > - the test certificate did expire on 2019-02-27 but changing the date to > 2019-01-01 and rebuilding does not impact these results > (incidentally the test certs seems to depend on an external CA > ca-test.tomcat.apache.org, fixing the certs will require switching to > the new-style local CA in tomcat8 - if fixing the certs is needed) > > As you fixed CVE-2017-5647 as well as generated the last jessie upload, > I would be interested in your take on this :) > TestSendFile only got trivial changes, so I guess I'll look for a fix in > later changes affecting files modified by CVE-2017-5647. > Still, I'm surprised updates were built given this situation - did > everybody got lucky with the flacky test or did I miss something? > > Cheers! > Sylvain > > On 27/07/2019 20:30, Abhijith PA wrote: >> Hi, >> >> >> I don't think the link you gave on commit [fe932dd39d] is the reason for >> FTBFS. I tried building on a VM that matches the certificate date and it >> was successful. I also tried disabling all ssl related tests and was fine. >> >> While doing these all I found TestSendFile test is the culprit. In >> CVE-2017-5647 security patch a good amount of changes is applied for >> SendFile*.java and *Nio2*.java. These are mostly about conditions on how >> long the socket of sendfile keep active and to take away from it. But I >> couldn't see any those change in its test file. Please take a look on >> the attached patch. :) >> >> >> --abhijith
Re: On tomcat FTBFS.
Hi Markus, I'm investigating tomcat8's FTBFS and I confirm Abhijith's findings in a Jessie VM: - test catalina/connector/TestSendFile.java fails with nio2 connector but is not reliable and will report success ~1 out of 10 even with lots of exceptions; catalina.log will report header parsing error and return 400 - it passes reliably without CVE-2017-5647.patch - the test certificate did expire on 2019-02-27 but changing the date to 2019-01-01 and rebuilding does not impact these results (incidentally the test certs seems to depend on an external CA ca-test.tomcat.apache.org, fixing the certs will require switching to the new-style local CA in tomcat8 - if fixing the certs is needed) As you fixed CVE-2017-5647 as well as generated the last jessie upload, I would be interested in your take on this :) TestSendFile only got trivial changes, so I guess I'll look for a fix in later changes affecting files modified by CVE-2017-5647. Still, I'm surprised updates were built given this situation - did everybody got lucky with the flacky test or did I miss something? Cheers! Sylvain On 27/07/2019 20:30, Abhijith PA wrote: > Hi, > > > I don't think the link you gave on commit [fe932dd39d] is the reason for > FTBFS. I tried building on a VM that matches the certificate date and it > was successful. I also tried disabling all ssl related tests and was fine. > > While doing these all I found TestSendFile test is the culprit. In > CVE-2017-5647 security patch a good amount of changes is applied for > SendFile*.java and *Nio2*.java. These are mostly about conditions on how > long the socket of sendfile keep active and to take away from it. But I > couldn't see any those change in its test file. Please take a look on > the attached patch. :) > > > --abhijith
On tomcat FTBFS.
Hi,
I don't think the link you gave on commit [fe932dd39d] is the reason for
FTBFS. I tried building on a VM that matches the certificate date and it
was successful. I also tried disabling all ssl related tests and was fine.
While doing these all I found TestSendFile test is the culprit. In
CVE-2017-5647 security patch a good amount of changes is applied for
SendFile*.java and *Nio2*.java. These are mostly about conditions on how
long the socket of sendfile keep active and to take away from it. But I
couldn't see any those change in its test file. Please take a look on
the attached patch. :)
--abhijith
From: Markus Koschany
Date: Sat, 15 Apr 2017 17:25:03 +0200
Subject: CVE-2017-5647
Bug-Debian: https://bugs.debian.org/860068
Origin: http://svn.apache.org/r1788999
---
java/org/apache/coyote/AbstractProtocol.java | 7 +-
.../apache/coyote/http11/Http11AprProcessor.java | 38 +++
.../apache/coyote/http11/Http11Nio2Processor.java | 11 +++-
.../apache/coyote/http11/Http11NioProcessor.java | 26 ++--
java/org/apache/tomcat/util/net/AprEndpoint.java | 49 +-
java/org/apache/tomcat/util/net/Nio2Endpoint.java | 25 ---
java/org/apache/tomcat/util/net/NioEndpoint.java | 76 --
.../tomcat/util/net/SendfileKeepAliveState.java| 39 +++
java/org/apache/tomcat/util/net/SendfileState.java | 37 +++
9 files changed, 222 insertions(+), 86 deletions(-)
create mode 100644 java/org/apache/tomcat/util/net/SendfileKeepAliveState.java
create mode 100644 java/org/apache/tomcat/util/net/SendfileState.java
diff --git a/java/org/apache/coyote/AbstractProtocol.java b/java/org/apache/coyote/AbstractProtocol.java
index 9886cef..cabfbf6 100644
--- a/java/org/apache/coyote/AbstractProtocol.java
+++ b/java/org/apache/coyote/AbstractProtocol.java
@@ -710,10 +710,9 @@ public abstract class AbstractProtocol implements ProtocolHandler,
release(wrapper, processor, false, true);
} else if (state == SocketState.SENDFILE) {
// Sendfile in progress. If it fails, the socket will be
-// closed. If it works, the socket will be re-added to the
-// poller
-connections.remove(socket);
-release(wrapper, processor, false, false);
+// closed. If it works, the socket either be added to the
+// poller (or equivalent) to await more data or processed
+// if there are any pipe-lined requests remaining.
} else if (state == SocketState.UPGRADED) {
// Don't add sockets back to the poller if this was a
// non-blocking write otherwise the poller may trigger
diff --git a/java/org/apache/coyote/http11/Http11AprProcessor.java b/java/org/apache/coyote/http11/Http11AprProcessor.java
index e4ecd1a..a08da6f 100644
--- a/java/org/apache/coyote/http11/Http11AprProcessor.java
+++ b/java/org/apache/coyote/http11/Http11AprProcessor.java
@@ -37,6 +37,7 @@ import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState;
import org.apache.tomcat.util.net.AprEndpoint;
import org.apache.tomcat.util.net.SSLSupport;
+import org.apache.tomcat.util.net.SendfileKeepAliveState;
import org.apache.tomcat.util.net.SocketStatus;
import org.apache.tomcat.util.net.SocketWrapper;
@@ -197,22 +198,31 @@ public class Http11AprProcessor extends AbstractHttp11Processor {
// Do sendfile as needed: add socket to sendfile and end
if (sendfileData != null && !getErrorState().isError()) {
sendfileData.socket = socketWrapper.getSocket().longValue();
-sendfileData.keepAlive = keepAlive;
-if (!((AprEndpoint)endpoint).getSendfile().add(sendfileData)) {
-// Didn't send all of the data to sendfile.
-if (sendfileData.socket == 0) {
-// The socket is no longer set. Something went wrong.
-// Close the connection. Too late to set status code.
-if (log.isDebugEnabled()) {
-log.debug(sm.getString(
-"http11processor.sendfile.error"));
-}
-setErrorState(ErrorState.CLOSE_NOW, null);
+if (keepAlive) {
+if (getInputBuffer().available() == 0) {
+sendfileData.keepAliveState = SendfileKeepAliveState.OPEN;
} else {
-// The sendfile Poller will add the socket to the main
-// Poller once sendfile processing is complete
-sendfileInProgress = true;
+sendfileData.keepAliveState = SendfileKeepAliveState.PIPELINED;
+}
+} else {
+sendfileData.keepAliveState = SendfileKeepAliveState.NONE;
+}
