RFR (S): JDK-8149169 - SSLSocketInputRecord.decodeInputRecord buffer overflow

Fri Mar 18 09:29:19 UTC 2016

Hi Xuelei,

thanks for your feedback. I tried to address all your points and made the test case more robust.

For SSLSocketImpl I also took the chance to remove 2 unused fields, hope that's ok. And I put the buffer length check in the block of handling non null buffer input.

If you are satisfied with my adaptions, it would be great if you could push the change for me as I'm no committer yet.

Thanks & Best regards
Christoph

> -----Original Message-----
> From: security-dev [mailto:security-dev-bounces at openjdk.java.net] On Behalf
> Of Xuelei Fan
> Sent: Freitag, 18. März 2016 03:52
> To: security-dev at openjdk.java.net
> Subject: Re: RFR (S): JDK-8149169 - SSLSocketInputRecord.decodeInputRecord
> buffer overflow
> 
> Hi Christoph,
> 
> Thank you for taking care of this issue.  Some minor comments:
> 
> SSLSocketImpl.java
> ------------------
> 1012    if (buffer != null && (buffer.limit() <
> inputRecord.bytesInCompletePacket(sockInput)))
> 1013         return 0;
> 
> 1. It would be nice to keep the line less than 80 characters.
> 2. In general, braces ('{' and '}') should always be used for 'if'
> statement.
> 3. It is safer to replace buffer.limit() with buffer.remaining().
> 
> LargePacketAfterHandshakeTest.java
> ----------------------------------
> See #1, too.
> 
> 4. The client thread may try to connect to server before server ready.
> As may result in intermittent failure.
> 
> In the template, test/javax/net/ssl/templates/SSLSocketTemplate.java, a
> serverReady variable is used to keep the pace of client and server.
> Just for your reference.
> 
> Thanks,
> Xuelei
> 
> On 3/17/2016 5:28 AM, Langer, Christoph wrote:
> > Hi,
> >
> >
> >
> > I think I've found a way to fix the issue which looks quite reasonable
> > to me. Would you please comment/review it? I've also included a test to
> > reproduce the issue.
> >
> >
> >
> > Webrev:http://cr.openjdk.java.net/~clanger/webrevs/8149169.1/
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8149169
> >
> >
> >
> > Thanks and best regards
> >
> > Christoph
> >
> >
> >
> > *From:*Langer, Christoph
> > *Sent:* Dienstag, 15. März 2016 23:00
> > *To:* security-dev at openjdk.java.net
> > *Subject:* Regarding JDK-8149169 -
> > SSLSocketInputRecord.decodeInputRecord buffer overflow
> >
> >
> >
> > Hi there,
> >
> >
> >
> > today I did some debugging regarding the TLS exception I've seen and
> > reported in JDK-8149169:
> >
> >
> >
> > javax.net.ssl.SSLException: java.nio.BufferOverflowException
> >         at sun.security.ssl.Alerts.getSSLException(Alerts.java:214)
> >         at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1948)
> >         at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1900)
> >         at
> > sun.security.ssl.SSLSocketImpl.handleException(SSLSocketImpl.java:1883)
> >         at
> > sun.security.ssl.SSLSocketImpl.handleException(SSLSocketImpl.java:1809)
> >         at sun.security.ssl.AppInputStream.read(AppInputStream.java:173)
> >         at java.io.BufferedInputStream.fill(BufferedInputStream.java:246)
> >         at java.io.BufferedInputStream.read1(BufferedInputStream.java:286)
> >         at java.io.BufferedInputStream.read(BufferedInputStream.java:345)
> >         at
> > sun.net.www.http.HttpClient.parseHTTPHeader(HttpClient.java:704)
> > <http://www.http.HttpClient.parseHTTPHeader%28HttpClient.java:704%29>
> >         at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:647)
> > <http://www.http.HttpClient.parseHTTP%28HttpClient.java:647%29>
> >         at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:675)
> > <http://www.http.HttpClient.parseHTTP%28HttpClient.java:675%29>
> >         at
> >
> sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConne
> ction.java:1534)
> >
> <http://www.protocol.http.HttpURLConnection.getInputStream0%28HttpURLCo
> nnection.java:1534%29>
> >         at
> >
> sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnec
> tion.java:1439)
> >
> <http://www.protocol.http.HttpURLConnection.getInputStream%28HttpURLCon
> nection.java:1439%29>
> >         at
> > java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
> >         at
> >
> sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsU
> RLConnectionImpl.java:319)
> >
> <http://www.protocol.https.HttpsURLConnectionImpl.getResponseCode%28Http
> sURLConnectionImpl.java:319%29>
> >
> >         at
> >
> com.sap.cl.HttpsURLConnectionTest.sendGETRequest(HttpsURLConnectionTest.j
> ava:42)
> >
> >         at
> > com.sap.cl.HttpsURLConnectionTest.main(HttpsURLConnectionTest.java:63)
> > Caused by: java.nio.BufferOverflowException
> >         at java.nio.HeapByteBuffer.put(HeapByteBuffer.java:206)
> >         at
> >
> sun.security.ssl.SSLSocketInputRecord.decodeInputRecord(SSLSocketInputRecor
> d.java:226)
> >
> >         at
> > sun.security.ssl.SSLSocketInputRecord.decode(SSLSocketInputRecord.java:178)
> >         at
> > sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:1012)
> >         at
> > sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:957)
> >         at sun.security.ssl.AppInputStream.read(AppInputStream.java:159)
> >         ... 12 more
> >
> >
> >
> > I think the problem is with the logic in
> > sun.security.ssl.AppInputStream. read(byte[] b, int off, int len). The
> > read method calls the readRecord(buffer) method of the socket
> > (SSLSocketImpl) and hands it the buffer to be eventually filled by
> > SSLSocketInputRecord.decodeInputRecord(). The buffer is initialized with
> > 4K and before readRecord is called, the packet length is verified (in
> > line 144: int packetLen = socket.bytesInCompletePacket();) and the
> > buffer reallocated if the incoming package would be larger than the
> > buffer. However, in my case, the incoming package is a handshake package
> > of a small size, so the buffer won't be adjusted. Then, after the
> > handshake is done, the real data packet is read, still within
> > SSLSocketImpl.readRecord() (e.g. line 1012 of SSLSocketImpl) and this
> > one has a length of more than 4K. So the buffer will be too small in
> > decodeInputRecord and hence the exception is thrown.
> >
> >
> >
> > So, basically the issue will appear if the TLS data package following
> > immediately after a server initiated handshake will be larger than the
> > buffer of AppInputStream. I guess that should be easily recreatable in a
> > small test case.
> >
> >
> >
> > Now the question how to fix? I can see 3 options:
> >
> > a)      Just allocate the ByteBuffer in AppInputStream to
> > SSLRecord.maxLargeRecordSize (about 32K) - easiest fix and removing the
> > need to check the length for each record. But I guess this is not
> > desired as the buffer is unnecessarily large for most cases?
> >
> > b)      Extend SSLSocketInputRecord somehow to be able to not only read
> > the length of the incoming packet but also the type, e.g. if it is a
> > handshake. In that case the buffer would need to be extended to
> > SSLRecord.maxLargeRecordSize. But why not do a) then??
> >
> > c)       Check the volume bytes returned from readRecord and redo the
> > read in case the volume is larger than the buffer capacity
> >
> >
> >
> > Which way should I pursue? Do you see another option? Or am I getting
> > something completely wrong running into an illegal case?
> >
> >
> >
> > Thanks in advance for your feedback,
> >
> > Christoph
> >
> >
> >