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(HttpURLConnection.java:1534)
> <http://www.protocol.http.HttpURLConnection.getInputStream0%28HttpURLConnection.java:1534%29>
> at
> sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1439)
> <http://www.protocol.http.HttpURLConnection.getInputStream%28HttpURLConnection.java:1439%29>
> at
> java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
> at
> sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsURLConnectionImpl.java:319)
> <http://www.protocol.https.HttpsURLConnectionImpl.getResponseCode%28HttpsURLConnectionImpl.java:319%29>
>
> at
> com.sap.cl.HttpsURLConnectionTest.sendGETRequest(HttpsURLConnectionTest.java: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(SSLSocketInputRecord.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
>
>
>
