Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received

Xue-Lei Fan xuelei.fan at oracle.com
Mon Aug 13 18:50:23 UTC 2018


Hi Jamil,

Thanks for review.  One more step close to the integration.

On 8/13/2018 11:45 AM, Jamil Nimeh wrote:
> Hi Xuelei,
> 
>   * SSLSocket.java
>       o 134: Nit - You can remove the first "both" in this sentence
>         since you use it later with the input/output stream closure.
> 
Let's use two to emphasize the behaviors:
1. both input and output streams should be closed in each side, and
2. both client and server should perform #1.

Thanks,
Xuelei

> Looks good to me otherwise.
> 
> --Jamil
> 
> On 8/13/2018 11:31 AM, Xue-Lei Fan wrote:
>> One more update:
>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.05/
>>
>> It is desired to make a note in SSLSocket and SSLEngine specification, 
>> so that users have a good sense that an application should close the 
>> input and output stream always.
>>
>> Updated for SSLEngine.java and SSLSocket.java only.  No changes in 
>> other files.
>>
>> Please let me know your concerns by the end of today.
>>
>> Thanks,
>> Xuelei
>>
>> On 8/10/2018 4:02 PM, Jamil Nimeh wrote:
>>> I'm good with the changes.
>>>
>>> --Jamil
>>>
>>> On 8/7/2018 5:24 PM, Xuelei Fan wrote:
>>>> Hi Jamil,
>>>>
>>>> Thanks for comments.  Here is the updated webrev:
>>>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.04/
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>> On 8/7/2018 3:12 PM, Jamil Nimeh wrote:
>>>>> Hi Xuelei, mostly small stuff:
>>>>>
>>>>>   * SSLEngineImpl.java
>>>>>       o 717: Nit, inbout --> inbound
>>>>>   * SSLEngineOutputRecord.java
>>>>>       o 162, 169: Nit, applicatoin --> application
>>>>>       o Same section: It looks like the "if" and "else if" clauses 
>>>>> take
>>>>>         the same actions with the same message.  Maybe just do "if
>>>>>         (isClosed())" ?  Or were you planning to have different 
>>>>> messages
>>>>>         here?
>>>>>   * SSLSocketOutputRecord.java
>>>>>       o 58, 97, 204: Typo,  closedd --> closed
>>>>>   * TLSEngineClosureTest.java
>>>>>       o 2: Copyright date fix
>>>>>       o 26: If this is a new test should the bug ID be different than
>>>>>         8085979?
>>>>>
>>>>> --Jamil
>>>>>
>>>>>
>>>>> On 08/07/2018 07:46 AM, Xuelei Fan wrote:
>>>>>> New webrev:
>>>>>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.03/
>>>>>>
>>>>>> Thanks for a find of Tim Brooks, that the SSLEngine 
>>>>>> inbound/outbound status is incorrect if closing during handshake. 
>>>>>> The above webrev is trying to fix the problems. See more in the 
>>>>>> OpenJDK thread:
>>>>>> http://mail.openjdk.java.net/pipermail/security-dev/2018-August/017778.html 
>>>>>>
>>>>>>
>>>>>> Please let me know your concerns before this Wednesday.
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>>
>>>>>> On 8/3/2018 1:55 PM, Xuelei Fan wrote:
>>>>>>> Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.02/
>>>>>>>
>>>>>>> In webrev.01, the socket close may be blocked by super class 
>>>>>>> close synchronization.  Updated the SSLSocketImpl.java to use 
>>>>>>> handshake only lock in the startHandshake() implementation.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Xuelei
>>>>>>>
>>>>>>> On 8/1/2018 7:27 PM, Xuelei Fan wrote:
>>>>>>>> Update: http://cr.openjdk.java.net/~xuelei/8207009/webrev.01/
>>>>>>>>
>>>>>>>> Integrated the fix for JDK-8208642, "Server initiated TLSv1.2 
>>>>>>>> renegotiation fails if Java client allows TLSv1.3". 
>>>>>>>> SSLHandshake.java is updated to use negotiated version so that 
>>>>>>>> TLS 1.2 HelloRequest is acceptable in TLS 1.3 client side.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Xuelei
>>>>>>>>
>>>>>>>> On 7/30/2018 10:24 AM, Xuelei Fan wrote:
>>>>>>>>> <loop in net-dev as well>
>>>>>>>>> Please let me know your concerns by the end of August 1st, 2018.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Xuelei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 7/30/2018 9:59 AM, Xuelei Fan wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Please review the update for the TLS 1.3 half-close and 
>>>>>>>>>> synchronization implementation:
>>>>>>>>>> http://cr.openjdk.java.net/~xuelei/8207009/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Unlike TLS 1.2 and prior versions, for TLS 1.3, the 
>>>>>>>>>> close_notify is use to close the local write side and peer 
>>>>>>>>>> read side only. After the close_notify get handles, the local 
>>>>>>>>>> read side and peer write side may still be open.
>>>>>>>>>>
>>>>>>>>>> In this update, if an application calls 
>>>>>>>>>> SSLEngine.closeInbound/Outbound() or 
>>>>>>>>>> SSLSocket.shutdownInput/Output(), half-close will be used.  
>>>>>>>>>> For compatibility, if SSLSocket.close() get called, a duplex 
>>>>>>>>>> close will be tried.  In order to support duplex close, JDK 
>>>>>>>>>> will use the user_canceled warning alert even the handshake 
>>>>>>>>>> complete.
>>>>>>>>>>
>>>>>>>>>> In practice, an application may only close outbound even it is 
>>>>>>>>>> intended to close the inbound as well, or close the connection 
>>>>>>>>>> completely.  It works for TLS 1.2 and prior versions.  But no 
>>>>>>>>>> more for TLS 1.3 because of the close_notify behavior change 
>>>>>>>>>> in the TLS 1.3 specification.  The application may be hung and 
>>>>>>>>>> dead-waiting for read/close.  It could be solved by closing 
>>>>>>>>>> the inbound explicitly. In order to mitigate the impact, a new 
>>>>>>>>>> System Property is introduced, 
>>>>>>>>>> "jdk.tls.acknowledgeCloseNotify" if source code update is not 
>>>>>>>>>> available.   If the System Property is set to "true", if 
>>>>>>>>>> receiving the close_notify, a close_notify alert will be 
>>>>>>>>>> responded.  It is a countermeasure of the TLS 1.3 half-close 
>>>>>>>>>> issues.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>
>>>
> 



More information about the security-dev mailing list