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....@oracle.com> > Cc: security-dev@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-boun...@openjdk.java.net] On > Behalf > > Of Xuelei Fan > > Sent: Freitag, 18. März 2016 03:52 > > To: security-dev@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@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 > > > > > > > > >