Bug 62791 - SecureNioChannel fails with "IllegalArgumentException: You can only read using the application read buffer provided by the handler."
Summary: SecureNioChannel fails with "IllegalArgumentException: You can only read usin...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 9.0.12
Hardware: PC Mac OS X 10.1
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-02 08:57 UTC by Maksym
Modified: 2018-10-15 05:03 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maksym 2018-10-02 08:57:30 UTC
I ran into issue with Tomcat 9 failing to handle data exchange via WSS: 

02-Oct-2018 17:02:01.421 SEVERE [https-jsse-nio-8443-exec-9] org.apache.coyote.AbstractProtocol$ConnectionHandler.process Error reading request, ignored
 java.lang.IllegalArgumentException: You can only read using the application read buffer provided by the handler.
	at org.apache.tomcat.util.net.SecureNioChannel.read(SecureNioChannel.java:571)
	at org.apache.tomcat.util.net.NioEndpoint$NioSocketWrapper.fillReadBuffer(NioEndpoint.java:1204)
	at org.apache.tomcat.util.net.NioEndpoint$NioSocketWrapper.read(NioEndpoint.java:1140)
	at org.apache.tomcat.websocket.server.WsFrameServer.onDataAvailable(WsFrameServer.java:72)
	at org.apache.tomcat.websocket.server.WsFrameServer.doOnDataAvailable(WsFrameServer.java:171)
	at org.apache.tomcat.websocket.server.WsFrameServer.notifyDataAvailable(WsFrameServer.java:151)
	at org.apache.tomcat.websocket.server.WsHttpUpgradeHandler.upgradeDispatch(WsHttpUpgradeHandler.java:148)
	at org.apache.coyote.http11.upgrade.UpgradeProcessorInternal.dispatch(UpgradeProcessorInternal.java:54)
	at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:53)
	at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:770)
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1415)
	at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
	at java.lang.Thread.run(Thread.java:748)


On a frontend side, I have a pretty simple javascript, which sends its state updates via web sockets (text) to the backend service (simple spring app running in Tomcat).
After Tomcat upgrade (from 8.0.53 to 9.0.12), application started failing to establish web socket connections with ssl-enabled connector ("plain" web sockets work fine). 

Steps to reproduce:
1. Generate certificate (I used steps from here: https://confluence.atlassian.com/doc/running-confluence-over-ssl-or-https-161203.html#RunningConfluenceOverSSLorHTTPS-Option1:Createaself-signedcertificate)
2. Configure "secured" connector in server.xml:
<Connector port="8443" maxHttpHeaderSize="8192"
                   maxThreads="150" minSpareThreads="25"
                   protocol="org.apache.coyote.http11.Http11NioProtocol"
                   enableLookups="false" disableUploadTimeout="true"
                   acceptCount="100" scheme="https" secure="true"
                   clientAuth="false" sslProtocols="TLSv1,TLSv1.1,TLSv1.2"      
                   sslEnabledProtocols="TLSv1,TLSv1.1,TLSv1.2"
                   SSLEnabled="true"
                   URIEncoding="UTF-8" keystorePass="changeit"
                   keystoreFile="/Users/user/.keystore"/>

With such connector settings, all attempts to establish web socket connections over ssl fail with exception above. At the same time:
1. Plain web sockets work
<Connector port="8090" connectionTimeout="20000" 
                   maxThreads="48" minSpareThreads="10"
                   enableLookups="false" acceptCount="10" debug="0" URIEncoding="UTF-8"
                   protocol="org.apache.coyote.http11.Http11NioProtocol"/>
2. All non-websockets requests work (both http and https)
3. Everything works when I downgrade Tomcat to version 8.0.53 (I didn't try other versions)

Looking a bit deeper into the problem, it seems that handshake succeeds, but Tomcat fails on the first attempt to read the data from a web socket.

It looks like WsFrameServer does not respect SocketBufferHandler settings from NioEndpoint$NioSocketWrapper at the time of its (WsFrameServer) instantiation in WsHttpUpgradeHandler, and always creates its own read buffer (WsFrameBase.java:93). Later, NioEndpoint$NioSocketWrapper tries its best to fill in this buffer as much as it could, and attempts to read directly to the buffer from the websocket (NioEndpoint.java:1204), which causes buffer check failure in SecureNioChannel.

There are several ways of fixing it.
1. The most obvious solution is to disable direct reads from the socket. Easy, but probably comes with performance degradation in some cases:
Index: java/org/apache/tomcat/util/net/NioEndpoint.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- java/org/apache/tomcat/util/net/NioEndpoint.java	(revision b648bba7acc3529c35d7bc5c54639a3dcb08e812)
+++ java/org/apache/tomcat/util/net/NioEndpoint.java	(date 1538469054000)
@@ -1135,26 +1135,16 @@
 
             // The socket read buffer capacity is socket.appReadBufSize
             int limit = socketBufferHandler.getReadBuffer().capacity();
-            if (to.remaining() >= limit) {
-                to.limit(to.position() + limit);
-                nRead = fillReadBuffer(block, to);
-                if (log.isDebugEnabled()) {
-                    log.debug("Socket: [" + this + "], Read direct from socket: [" + nRead + "]");
-                }
-                updateLastRead();
-            } else {
-                // Fill the read buffer as best we can.
-                nRead = fillReadBuffer(block);
-                if (log.isDebugEnabled()) {
-                    log.debug("Socket: [" + this + "], Read into buffer: [" + nRead + "]");
-                }
-                updateLastRead();
+            nRead = fillReadBuffer(block);
+            if (log.isDebugEnabled()) {
+                log.debug("Socket: [" + this + "], Read into buffer: [" + nRead + "]");
+            }
+            updateLastRead();
 
-                // Fill as much of the remaining byte array as possible with the
-                // data that was just read
-                if (nRead > 0) {
-                    nRead = populateReadBuffer(to);
-                }
+            // Fill as much of the remaining byte array as possible with the
+            // data that was just read
+            if (nRead > 0) {
+                nRead = populateReadBuffer(to);
             }
             return nRead;
         }

2. Another possible solution would be to use temporary buffer in WsFrameServer:
Index: java/org/apache/tomcat/websocket/server/WsFrameServer.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- java/org/apache/tomcat/websocket/server/WsFrameServer.java	(revision b648bba7acc3529c35d7bc5c54639a3dcb08e812)
+++ java/org/apache/tomcat/websocket/server/WsFrameServer.java	(date 1538469610000)
@@ -69,7 +69,10 @@
             // Fill up the input buffer with as much data as we can
             inputBuffer.mark();
             inputBuffer.position(inputBuffer.limit()).limit(inputBuffer.capacity());
-            int read = socketWrapper.read(false, inputBuffer);
+            int capacity = inputBuffer.capacity();
+            byte[] byteArray = new byte[capacity];
+            int read = socketWrapper.read(false, byteArray, 0, capacity);
+            inputBuffer.put(byteArray);
             inputBuffer.limit(inputBuffer.position()).reset();
             if (read < 0) {
                 throw new EOFException();
Disadvantages for this approach would be creation of unnecessary byte buffer (i.e. allocating unnecessary memory) and (possibly) race conditions (if inputBuffer is written while we're reading to byteArray)

3. We may also get rid of buffer check in SecureNioChannel (security concern) or add some parameter to ignore that check (a bit "dirty" solution as for me):
Index: java/org/apache/tomcat/util/net/NioEndpoint.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- java/org/apache/tomcat/util/net/NioEndpoint.java	(revision b648bba7acc3529c35d7bc5c54639a3dcb08e812)
+++ java/org/apache/tomcat/util/net/NioEndpoint.java	(date 1538470221000)
@@ -1201,7 +1201,7 @@
                     }
                 }
             } else {
-                nRead = channel.read(to);
+                nRead = channel instanceof SecureNioChannel ? ((SecureNioChannel)channel).read(to, true) : channel.read(to);
                 if (nRead == -1) {
                     throw new EOFException();
                 }
Index: java/org/apache/tomcat/util/net/SecureNioChannel.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- java/org/apache/tomcat/util/net/SecureNioChannel.java	(revision b648bba7acc3529c35d7bc5c54639a3dcb08e812)
+++ java/org/apache/tomcat/util/net/SecureNioChannel.java	(date 1538470221000)
@@ -565,8 +565,12 @@
      */
     @Override
     public int read(ByteBuffer dst) throws IOException {
+        return read(dst, false);
+    }
+
+    public int read(ByteBuffer dst, boolean ignoreBufferCheck) throws IOException {
         // Make sure we only use the ApplicationBufferHandler's buffers
-        if (dst != getBufHandler().getReadBuffer() && (getAppReadBufHandler() == null
+        if (!ignoreBufferCheck && dst != getBufHandler().getReadBuffer() && (getAppReadBufHandler() == null
                 || dst != getAppReadBufHandler().getByteBuffer())) {
             throw new IllegalArgumentException(sm.getString("channel.nio.ssl.invalidBuffer"));
         }



There is a workaround: to use Http11Nio2Protocol instead of Http11NioProtocol

Java version: 1.8.0_171-b11

OS: OSX and Amazon Linux

Related changes:
Introduce fillReadBuffer(boolean, ByteBuffer)  git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1758000 13f79535-47bb-0310-9956-ffa450edef68
Introduce a new method SocketWrapperBase.read(boolean, ByteBuffer)  git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1758058 13f79535-47bb-0310-9956-ffa450edef68
Comment 1 Remy Maucherat 2018-10-02 14:18:52 UTC
I couldn't figure out why it was such a drama to not use the container buffer for NIO SSL (except: no resize ...), so I didn't put it in NIO2, that's why it works. The code path clearly uses the application provided buffer in SecureNioChannel in the situation you describe where there are no bytes left.

Either way, I would simply remove the check.

Comments ?
Comment 2 Mark Thomas 2018-10-02 15:42:53 UTC
I'll do some svn archaeology to see if I can find out why that check was added in the first place and if that reason is still valid.
Comment 3 Remy Maucherat 2018-10-02 17:05:05 UTC
Either way the check is pointless, removing it means it will cause an IOE later only if a resize is (somehow) needed. The scenario is handled well in the code.
Comment 4 Mark Thomas 2018-10-02 17:17:29 UTC
I am currently looking at the openjdk sources to see what circumstances, if any, a resize would be required.
Comment 5 Mark Thomas 2018-10-02 18:01:39 UTC
I agree the check is pointless for the reasons outlined in comment #3.

Having looked at the openjdk source I think there is a lot of scope to clean this up. The TLS spec specifies a maximum of 2^14. Java defaults to this or, if jsse.SSLEngine.acceptLargeFragments is set, defaults to 2^15 to allow for non-compliant TLS stacks. After that any renegotiation of the app buffer size is only ever going to be downwards - not upwards - for any given session. The same goes for the packet buffer.

I think we can remove the whole resize concept. We added it thinking the language in the Javadoc meant it could be resized upwards but my understanding of the code I have read is that it will only ever go downwards.
Comment 6 Mark Thomas 2018-10-08 14:17:43 UTC
Fixed in:
- trunk for 9.0.13 onwards
- 8.5.x for 8.5.35 onwards
- 7.0.x for 7.0.92 onwards

I'll look into what, if any, further clean-up is possible separately.
Comment 7 Mark Thomas 2018-10-08 16:04:16 UTC
Looks like I missed something when looking at the OpenJDK code.

From the Oracle JSSE docs:

<quote>
Note: The SSL/TLS protocols specify that implementations are to produce packets containing at most 16 kilobytes (KB) of plain text. However, some implementations violate the specification and generate large records up to 32 KB. If the SSLEngine.unwrap() code detects large inbound packets, then the buffer sizes returned by SSLSession will be updated dynamically. Applications should always check the BUFFER_OVERFLOW and BUFFER_UNDERFLOW statuses and enlarge the corresponding buffers if necessary. SunJSSE will always send standard compliant 16 KB records and allow incoming 32 KB records. For a workaround, see the System property jsse.SSLEngine.acceptLargeFragments in Customizing JSSE.
</quote>


If we removed the resizing then any spec non-complaint clients are going to fail until Tomcat is restarted with the above system property set. On balance, I think it is best to leave things as they are.
Comment 8 Remy Maucherat 2018-10-08 16:08:13 UTC
+1 to leave it as is.
Comment 9 Maksym 2018-10-11 23:45:37 UTC
Thank you for fixing it! Does it make sense to backport this fix to tomcat 8.0.x as well (especially taking into account that it was fixed in 7.0.x)?
Comment 10 Remy Maucherat 2018-10-12 06:49:02 UTC
The Tomcat 8.0 branch is EOL.
Comment 11 Maksym 2018-10-15 05:03:12 UTC
(In reply to Mark Thomas from comment #6)
> Fixed in:
> - trunk for 9.0.13 onwards
> - 8.5.x for 8.5.35 onwards
> - 7.0.x for 7.0.92 onwards
> 
> I'll look into what, if any, further clean-up is possible separately.

When 9.0.13 is planned for release?