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

Fri Mar 18 13:23:55 UTC 2016

Looks fine to me.

The test passed, and I pushed the changeset.

   http://hg.openjdk.java.net/jdk9/dev/jdk/rev/d6a0479363ed

Thanks,
Xuelei

On 3/18/2016 5:33 PM, Langer, Christoph wrote:
> Sorry, forgot the new webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8149169.2/
> 
> 
>> -----Original Message-----
>> From: Langer, Christoph
>> Sent: Freitag, 18. März 2016 10:29
>> To: 'Xuelei Fan' <xuelei.fan at oracle.com>
>> Cc: security-dev at openjdk.java.net
>> Subject: RE: RFR (S): JDK-8149169 - SSLSocketInputRecord.decodeInputRecord
>> buffer overflow
>>
>> 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
>>>>
>>>>
>>>>
> 