Bug 61948 - BufferUnderflowException and IllegalArgumentException in TLSClientHelloExtractor
Summary: BufferUnderflowException and IllegalArgumentException in TLSClientHelloExtractor
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Util (show other bugs)
Version: 9.0.2
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-31 14:44 UTC by Evgenij Ryazanov
Modified: 2018-01-03 11:19 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Evgenij Ryazanov 2017-12-31 14:44:11 UTC
I found two similar exceptions in system journal.

1:22:10 org.apache.tomcat.util.net.NioEndpoint$SocketProcessor doRun
SEVERE: 
java.nio.BufferUnderflowException
at java.base/java.nio.Buffer.nextGetIndex(Buffer.java:634)
at java.base/java.nio.HeapByteBuffer.getChar(HeapByteBuffer.java:299)
at org.apache.tomcat.util.net.TLSClientHelloExtractor.<init>(TLSClientHelloExtractor.java:110)
at org.apache.tomcat.util.net.SecureNioChannel.processSNI(SecureNioChannel.java:282)
at org.apache.tomcat.util.net.SecureNioChannel.handshake(SecureNioChannel.java:175)
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1353)
at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
at java.base/java.lang.Thread.run(Thread.java:844)

1:22:11 org.apache.tomcat.util.net.NioEndpoint$SocketProcessor doRun
SEVERE: 
java.lang.IllegalArgumentException: newPosition > limit: (34392 > 248)
at java.base/java.nio.Buffer.createPositionException(Buffer.java:313)
at java.base/java.nio.Buffer.position(Buffer.java:288)
at java.base/java.nio.ByteBuffer.position(ByteBuffer.java:1079)
at java.base/java.nio.ByteBuffer.position(ByteBuffer.java:260)
at org.apache.tomcat.util.net.TLSClientHelloExtractor.skipBytes(TLSClientHelloExtractor.java:250)
at org.apache.tomcat.util.net.TLSClientHelloExtractor.<init>(TLSClientHelloExtractor.java:141)
at org.apache.tomcat.util.net.SecureNioChannel.processSNI(SecureNioChannel.java:282)
at org.apache.tomcat.util.net.SecureNioChannel.handshake(SecureNioChannel.java:175)
at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1353)
at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
at java.base/java.lang.Thread.run(Thread.java:844)

It seems that TLSClientHelloExtractor doesn't have enough checks for sanity of received client hello message.
Comment 1 Evgenij Ryazanov 2018-01-01 03:59:33 UTC
The following simple code causes BufferUnderflowException in TLSClientHelloExtractor.isClientHello() and IllegalArgumentException in TLSClientHelloExtractor.skipBytes().

import java.io.OutputStream;
import java.net.Socket;
import javax.net.SocketFactory;

byte[][] data = {
{ /* TLS handshake */ 22, /* TLS 1.0 */ 3, 1, /* Length 0 */ 0, 0 },
{ /* TLS handshake */ 22, /* TLS 1.0 */ 3, 1, /* Length 4 */ 0, 4, /* Type 1 */ 1, /* Size 0 */ 0, 0, 0 },
};

for (byte[] a : data)
  try (Socket s = SocketFactory.getDefault().createSocket("hostname", 443);
       OutputStream out = s.getOutputStream()) {
    out.write(a);
  }

There are many ways to get exceptions with larger ill-formed packets.
Comment 2 Remy Maucherat 2018-01-02 13:08:58 UTC
Do you have any exceptions for legitimate TLS records ? If not, instead of validating all reads, it is reasonable to catch the exceptions and log as debug instead. I reviewed the code and it seems to properly validate the lengths, it requests more data, etc.
Comment 3 Evgenij Ryazanov 2018-01-02 14:28:27 UTC
I don't know the source of requests from system journal. I think that both requests were ill-formed. They may even be specially crafted. I agree that simple try-catch will be more efficient and reasonable than a lot of additional checks.
Comment 4 Mark Thomas 2018-01-02 20:50:07 UTC
If the code were to throw an IOException on a malformed ClientHello then a debug log message would be generated as required.

I'm going to look at turning simple code to reproduce into a test case and then fixing the issue.
Comment 5 Mark Thomas 2018-01-02 21:33:55 UTC
Fixed in:
- trunk for 9.0.3 onwards
- 8.5.x for 8.5.25 onwards

Earlier versions are not affected as SNI is not supported.
Comment 6 Remy Maucherat 2018-01-03 10:55:35 UTC
This is the way I was planning to do it, so good.
Evgenij, please let us know if you find any legitimate TLS records that would cause an exception (the debug level will now hide the fact the connection is being closed, so it would be best to fix them now).
Comment 7 Mark Thomas 2018-01-03 11:19:09 UTC
FYI I went through the SNI parsing code manually and as far as I could tell the two provided test cases covered the two possible exception types for malformed input. There was the possibility of an IndexOutOfBoundsException but the input that could trigger that is not under user control and the values Tomcat provides will never trigger it.

Of course, I could have missed something so any additional test cases would be welcome.