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

[Xuelei Fan]

Hi Christoph,

Thank you for taking care of this issue.  Some minor comments:

SSLSocketImpl.java
--
1012if (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)
> 
> at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:647)
> 
> at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:675)
> 
> at
> sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1534)
> 
> at
> sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1439)
> 
> at
> java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
> at
> sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsURLConnectionImpl.java:319)
> 
> 
> 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
> SSLSocketI

Re: Code Review Request, 8152221 Use try-with-resource in test templates

[Wang Weijun]

Great, looks fine.

Thanks
Max

> On Mar 19, 2016, at 10:16 PM, Xuelei Fan  wrote:
> 
> Hi,
> 
> Please review this test update for JDK-8152221:
> 
> http://cr.openjdk.java.net/~xuelei/8152221/webrev.00/
> 
> The templates for SSL/TLS implementation testing are updated to use
> try-with-resource statements.
> 
> Thanks,
> Xuelei

Code Review Request, 8152221 Use try-with-resource in test templates

[Xuelei Fan]

Hi,

Please review this test update for JDK-8152221:

   http://cr.openjdk.java.net/~xuelei/8152221/webrev.00/

The templates for SSL/TLS implementation testing are updated to use
try-with-resource statements.

Thanks,
Xuelei

Re: RFR 8140422: Add mechanism to allow non default root CAs to be not subject to algorithm restrictions

[Erik Joelsson]

Much better, and thank you for fixing the existing mkdir/echo lines too. 
Just one nit, for this continuation:


$(TOOL_CACERTSHASHER) -i $(GENDATA_CACERTSHASHER_IN) \
 -o $(GENDATA_CACERTSHASHER)

please use tab+4spaces for the second line. No need to resend webrev for 
that. See [1] for our build system code conventions.


[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

/Erik

On 2016-03-18 18:09, Anthony Scarpino wrote:

I believe I got everyone's comments. I've updated the webrev.

http://cr.openjdk.java.net/~ascarpino/8140422/webrev.02/

Thanks

Tony


On 02/29/2016 08:55 AM, Anthony Scarpino wrote:

Currently CertPath algorithm restrictions allow or deny all
certificates.  This change adds the ability to reject certificate chains
that contain a restricted algorithm and the chain terminates at a root
CA; therefore, allowing a self-signed or chain that does not terminate
at a root CA.

https://bugs.openjdk.java.net/browse/JDK-8140422

Thanks

Tony

Re: JDK 9 RFR of JDK-8151763; Use more informative format for problem list

[Joseph D. Darcy]

Hi Jon,

Noted; I'll make that improvement in the next round.

Thanks for pointing this out,

-Joe

On 3/16/2016 4:50 PM, Jonathan Gibbons wrote:



On 03/11/2016 07:28 PM, joe darcy wrote:

Hello,

As Jon Gibbons has noted off-list, the problem list entries can 
directly include the bug number associated with the test in question, 
enabling better reporting. This format should be used rather than the 
current convention of putting the bug number in a comment.


Please review the webrev to adopt the revised format for the problem 
list:


http://cr.openjdk.java.net/~darcy/8151763.0/

I've verified jtreg produces the same test list with the old and new 
versions of the problem list.


Thanks,

-Joe



Joe,

You can use a comma-separated list when multiple bugs are involved.   
The only restriction is,  no embedded whitespace within the list


 342 # Also 8080165
 343 java/util/Arrays/ParallelPrefix.java   8085982 generic-all

can be

 343 java/util/Arrays/ParallelPrefix.java   8085982,8080165 generic-all


-- Jon

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

[Langer, Christoph]

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
> --
> 1012if (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)
> > 
> > at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:647)
> > 
> > at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:675)
> > 
> > at
> >
> sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConne
> ction.java:1534)
> >
>  nnection.java:1534%29>
> > at
> >
> sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnec
> tion.java:1439)
> >
>  nection.java:1439%29>
> > at
> > java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
> > at
> >
> sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsU
> RLConnectionImpl.java:319)
> >
>  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(SSLS

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

[Langer, Christoph]

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)
at 
sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:647)
at 
sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:675)
at 
sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1534)
at 
sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1439)
at 
java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
at 
sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsURLConnectionImpl.java:319)
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 

Re: JDK 9 RFR of JDK-8151763; Use more informative format for problem list

[Jonathan Gibbons]

On 03/11/2016 07:28 PM, joe darcy wrote:

Hello,

As Jon Gibbons has noted off-list, the problem list entries can 
directly include the bug number associated with the test in question, 
enabling better reporting. This format should be used rather than the 
current convention of putting the bug number in a comment.


Please review the webrev to adopt the revised format for the problem 
list:


http://cr.openjdk.java.net/~darcy/8151763.0/

I've verified jtreg produces the same test list with the old and new 
versions of the problem list.


Thanks,

-Joe



Joe,

You can use a comma-separated list when multiple bugs are involved.   
The only restriction is,  no embedded whitespace within the list


 342 # Also 8080165
 343 java/util/Arrays/ParallelPrefix.java   8085982 generic-all

can be

 343 java/util/Arrays/ParallelPrefix.java   8085982,8080165 generic-all


-- Jon

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

[Xuelei Fan]

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' 
>> 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
>>> --
>>> 1012if (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)
 
 at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:647)
 
 at sun.net.www.http.HttpClient.parseHTTP(HttpClient.java:675)
 
 at

>>>
>> sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConne
>>> ction.java:1534)

>>>
>> >> nnection.java:1534%29>
 at

>>>
>> sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnec
>>> tion.java:1439)

>>>
>> >> nection.java:1439%29>
 at

>> java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
 at

>>>
>> sun.net.www.protocol.https.HttpsURLConnection