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....@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 >>>> >>>> >>>> >
