Code Review Request, JDK-8207009 SSLEngine#closeInbound mentions SSLException when no close_notify is received
Xue-Lei Fan
xuelei.fan at oracle.com
Tue Aug 14 13:25:31 UTC 2018
Hi Brad,
Good points! Here is the updated webrev:
http://cr.openjdk.java.net/~xuelei/8207009/webrev.06/
Please let me know if you have more comments by 11:30AM today.
Thanks,
Xuelei
On 8/13/2018 4:43 PM, Bradford Wetmore wrote:
> Hi Xuelei,
>
> > 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.
>
> SSLEngine.java
> --------------
> 159: Both sides (i.e. the peer) may not be a SSLEngine:
>
> both the client and server applications should close the {@code
> SSLEngine} by calling {@link SSLEngine#closeInbound} and {@link
> SSLEngine#closeOutbound} and should send/receive any...
> ->
> the application should close the {@code SSLEngine} by calling {@link
> SSLEngine#closeInbound} and {@link SSLEngine#closeOutbound} and should
> send/receive any...
>
>
> SSLSocket.java
> --------------
> With all of the ways to close, trying to say anything here is going to
> get really ugly really fast. SSLSocket.close(), InputStream.close() &
> OutputStream.close, and now Socket.shutdownInput() &
> Socket.shutdownOutput(). Even shutdownInput()/shutdownOutput() still
> throw UnsupportedOperationExceptions in JDK 10 all the way back to 1.4.
>
> Can we just leave it at SSLSocket.close() and skip the
> Input/OutputStream.close() and shutdownInput/Output? Or something like:
>
> @apiNote
> The TLS standard has evolved over the years, especially when it comes to
> closing Sockets and streams. The most straightforward way to close a
> connection is for an application to call SSLSocket.close(). Some
> versions of the standard allow for independently closing the input or
> output sides of a connection, but may throw Exceptions if certain
> conditions aren't met. If such a connection state is desired, the
> output stream should generally be closed before the input stream.
>
> Once an {@code SSLSocket} is closed, it is not reusable: a new
> {@code SSLSocket} must be created.
>
> Brad
>
>
>
>
> On 8/13/2018 11:50 AM, Xue-Lei Fan wrote:
>> 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