Re: Http11Nio2Protocol allows brand-new sockets to live indefinitely? (Tomcat 9.0.8 and others)

[Alex Marchevskiy <al...@backblaze.com>]

Hi Rémy,

Thank you for your quick follow up to the issue posted by Adam. I have been reviewing the patch from r1832519 and it appears that if a connection is established and no bytes are sent, the socket remains open indefinitely waiting for the handshakeReadCompletionHandler to callback. Hence it would be possible for a malicious user to establish enough connections to match the OS file descriptor limit and prevent Tomcat from servicing any new connections simply by keeping the connections open and not sending any data. 

In order to work around this case, I was able to include the endpoint connection timeout in the processSNI method and pass it to the read under the case where no bytes are available in the netInBuffer. Please see the diff below:


diff --git a/java/org/apache/tomcat/util/net/SecureNio2Channel.java b/java/org/apache/tomcat/util/net/SecureNio2Channel.java
index b0202b706..5cf044bff 100644
--- a/java/org/apache/tomcat/util/net/SecureNio2Channel.java
+++ b/java/org/apache/tomcat/util/net/SecureNio2Channel.java
@@ -221,8 +221,9 @@ public class SecureNio2Channel extends Nio2Channel  {
            return 0; //we have done our initial handshake
        }

+        long timeout = endpoint.getConnectionTimeout();
        if (!sniComplete) {
-            int sniResult = processSNI();
+            int sniResult = processSNI(timeout);
            if (sniResult == 0) {
                sniComplete = true;
            } else {
@@ -231,7 +232,6 @@ public class SecureNio2Channel extends Nio2Channel  {
        }

        SSLEngineResult handshake = null;
-        long timeout = endpoint.getConnectionTimeout();

        while (!handshakeComplete) {
            switch (handshakeStatus) {
@@ -366,12 +366,13 @@ public class SecureNio2Channel extends Nio2Channel  {
     * present and, if it is, what host name has been requested. Based on the
     * provided host name, configure the SSLEngine for this connection.
     */
-    private int processSNI() throws IOException {
+    private int processSNI(long timeout) throws IOException {
        // If there is no data to process, trigger a read immediately. This is
        // an optimisation for the typical case so we don't create an
        // SNIExtractor only to discover there is no data to process
        if (netInBuffer.position() == 0) {
-            sc.read(netInBuffer, socket, handshakeReadCompletionHandler);
+            sc.read(netInBuffer, Nio2Endpoint.toNio2Timeout(timeout),
+                    TimeUnit.MILLISECONDS, socket, handshakeReadCompletionHandler);
            return 1;
        }


I was hoping to ask for your feedback regarding whether this is a valid way to handle the aforementioned condition. I would be happy to work on getting this merged upstream with proper test coverage if you believe this is reasonable. 

Thanks in advance,
Alex
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org

Re: Http11Nio2Protocol allows brand-new sockets to live indefinitely? (Tomcat 9.0.8 and others)

[Alex Marchevskiy <al...@backblaze.com>]

Thanks Rémy! 

> On Jun 2, 2018, at 12:49 PM, Rémy Maucherat <re...@apache.org> wrote:
> 
> On Sat, Jun 2, 2018 at 9:25 PM Alex Marchevskiy <al...@backblaze.com> wrote:
> 
>> Hi Rémy,
>> 
>> Thank you for your quick follow up to the issue posted by Adam. I have
>> been reviewing the patch from r1832519 and it appears that if a connection
>> is established and no bytes are sent, the socket remains open indefinitely
>> waiting for the handshakeReadCompletionHandler to callback. Hence it would
>> be possible for a malicious user to establish enough connections to match
>> the OS file descriptor limit and prevent Tomcat from servicing any new
>> connections simply by keeping the connections open and not sending any
>> data.
>> 
> 
> Ok, there were three read operations that did not have a timeout and that
> is now fixed as well. However, the timeout is often "longish", so it won't
> make such a big difference anyway and NIO2 is not supposed to operate with
> any real connection limit.
> 
> Rémy


---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org

Re: Http11Nio2Protocol allows brand-new sockets to live indefinitely? (Tomcat 9.0.8 and others)

[Rémy Maucherat <re...@apache.org>]

On Sat, Jun 2, 2018 at 9:25 PM Alex Marchevskiy <al...@backblaze.com> wrote:

> Hi Rémy,
>
> Thank you for your quick follow up to the issue posted by Adam. I have
> been reviewing the patch from r1832519 and it appears that if a connection
> is established and no bytes are sent, the socket remains open indefinitely
> waiting for the handshakeReadCompletionHandler to callback. Hence it would
> be possible for a malicious user to establish enough connections to match
> the OS file descriptor limit and prevent Tomcat from servicing any new
> connections simply by keeping the connections open and not sending any
> data.
>

Ok, there were three read operations that did not have a timeout and that
is now fixed as well. However, the timeout is often "longish", so it won't
make such a big difference anyway and NIO2 is not supposed to operate with
any real connection limit.

Rémy