RFR JDK-8240871: SSLEngine handshake status immediately after the handshake can be NOT_HANDSHAKING rather than FINISHED with TLSv1.3
Anthony Scarpino
anthony.scarpino at oracle.com
Fri May 29 17:17:59 UTC 2020
Ok thanks.. no further questions.
Tony
On 5/29/20 9:54 AM, Xuelei Fan wrote:
> On 5/29/2020 8:59 AM, Anthony Scarpino wrote:
>> Thanks for the changes Xuelei, it is much clearer to me now. Two
>> minor things and a question.
>>
>>
>> NewSessionTicket:323. The comment talks about an empty session ticket,
>>
>> SSLEngineImpl:167. Comment uses old variable name
>>
> Oops, I still missed the two above. Updated in my local workspace.
>
>> One question, NewSessionTicket 464-470 and SSLEngineImpl.writeRecord()
>> 167-172 both set needHandshakeFinishedStatus false. If the NST code
>> sets needHandshakeFinishedStatus before the asynchronous call to
>> writeRecord(), couldn't that not change the status?
> Good question. I double checked that the session ticket delivery and
> writeRecord() are synchronized. The set and use of
> needHandshakeFinishedStatus are thread safe.
>
>> Would it be better to only have the writeRecord() check?
>>
> The two block are used for different purpose.
>
> If the client support session ticket
> (psk_key_exchange_modes.psk_dhe_ke), the server will response with a
> session ticket in the last handshake wrap(). The wrap() method will
> return FINISHED status. For this case, as the FINISHED status could be
> caught, there is no need to have an additional wrap() to return the
> FINISHED handshake status any longer. The block in NewSessionTicket
> 464-470 is used to indicate the case.
>
> If the Firefox run in private mode, the psk_key_exchange_modes extension
> is not used, and the server will not response with session ticket. Then,
> the FINISHED status get missed if Certificate + CertificateVerify +
> Finished hanshake messages wrapped in one record. The block in
> SSLEngineImpl.writeRecord() 167-172 is used to handle this case.
>
> Thanks,
> Xuelei
>
>> thanks
>> Tony
>>
>>
>> On 5/27/20 8:44 PM, Xuelei Fan wrote:
>>> Tony and I had a private chat, and I understand his concerns better
>>> now. The name of the variable
>>> TransportContext.needEmptySessionTicket is confusing. I updated it
>>> to "needHandshakeFinishedStatus" per the suggestion, together with
>>> other change according to the comments.
>>>
>>> Here is the new webrev:
>>> http://cr.openjdk.java.net/~xuelei/8240871/webrev.01/
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 5/26/2020 2:40 PM, Xuelei Fan wrote:
>>>> On 5/26/2020 1:26 PM, Anthony Scarpino wrote:
>>>>> On 5/13/20 1:44 PM, Xuelei Fan wrote:
>>>>>> On 5/13/2020 9:41 AM, Anthony Scarpino wrote:
>>>>>>> On 4/30/20 10:19 AM, Xuelei Fan wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Could I get the following update reviewed:
>>>>>>>> http://cr.openjdk.java.net/~xuelei/8240871/webrev.00/
>>>>>>>>
>>>>>>>> For TLS 1.3 full handshake, if the last handshake flight wraps
>>>>>>>> the Finished together with other handshake message, for example
>>>>>>>> client certificate, the flight could be wrapped and encrypted in
>>>>>>>> one record and delegated tasks would be used. There is no
>>>>>>>> chance to return FINISHED handshake status with
>>>>>>>> SSLEngine.(un)wrap(). However, per the HandshakeStatus.FINISHED
>>>>>>>> specification, this handshake status is only generated by a call
>>>>>>>> to SSLEngine.wrap()/unwrap() and it is never generated by
>>>>>>>> SSLEngine.getHandshakeStatus().
>>>>>>>>
>>>>>>>> In order to workaround this case for TLS 1.3, the FINISHED
>>>>>>>> status could present with SSLEngine.wrap() while delivering of
>>>>>>>> the NewSessionTicket post-handshake message. If this
>>>>>>>> post-handshake message is not needed, a follow-on
>>>>>>>> SSLEngine.wrap() should be called to indicate the FINISHED
>>>>>>>> handshake status. Although this special SSLEngine.wrap() should
>>>>>>>> not consume or produce any application or network data.
>>>>>>>>
>>>>>>>> I also clean up some debug log, names and code style a little bit.
>>>>>>>>
>>>>>>>> The update could be confirmed with Tomcat and Firefox in private
>>>>>>>> mode, as described in the bug description. As this case happens
>>>>>>>> only when psk_key_exchange_modes does not present, which is not
>>>>>>>> a behavior supported by JDK, I did not find a workaround for a
>>>>>>>> new regression test yet. I added the labels, noreg-external and
>>>>>>>> noreg-hard.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Xuelei
>>>>>>>
>>>>>>> I do not fully understand the situation, mostly because of
>>>>>>> SSLEngine semantics. In normal operation, does is
>>>>>>> HandshakeStatus.FINISHED returned when Finished is received or
>>>>>>> after the NewSessionTicket message?
>>>>>> Not exactly. For TLS 1.2, FINISHED will be returned with unwrap()
>>>>>> of the finished handshake message. However, for TLS 1.3, FINISHED
>>>>>> will be returned any longer, because the finished handshake
>>>>>> message is wrapped with certificate message in one record.
>>>>>>
>>>>>> For TLS 1.3:
>>>>>> 1. client send certificate, certificate verify and finished
>>>>>> handshake message in one record.
>>>>>> 2. server call unwrap(), and return NEED_TASK to handle the
>>>>>> certificate and certificate verify.
>>>>>>
>>>>>> So, no more FINISHED for the unwrap() return.
>>>>>>
>>>>>> It is fine if there is a after NewSessionTicket message. The
>>>>>> wrap() for the post-handshake message will return FINISHED.
>>>>>>
>>>>>> The bug reported is a special one that the Firefox is run in
>>>>>> private mode, which does not request NewSessionTicket. So there
>>>>>> is no post-handshake generated and sent in server side. Then,
>>>>>> there is no FINISHED can be used if applications depends on it.
>>>>>>
>>>>>> To workaround the case, a dummy wrap() or unwrap() could be used
>>>>>> to get the FINISHED. The wrap() or unwrap() actually do nothing
>>>>>> but return the FINISHED status.
>>>>>>
>>>>>
>>>>> I don't want to be problematic, but I don't really agree with
>>>>> creating dummy messages to generate wrap/unwrap operations in the
>>>>> TLS code.
>>>> No dummy message created. Only need to call wrap() or unwrap(), but
>>>> not data consumed or generated by the call to wrap() or unwrap(), no
>>>> application data consumed, no network data consumed, no application
>>>> data generated, no network data generated.
>>>>
>>>>> If SSLEngine is doing something wrong with not fully reading the
>>>>> buffer, then I feel it's SSLEngine that should be fixed to handle
>>>>> the situation right.
>>>> It is not caused by SSLEngine that does not fully reading the
>>>> buffer. Let me try again about what's the problem.
>>>>
>>>> The client (Firefox) sends, Certificate and CerticateVerify and
>>>> Finished handshake messages in one record. The record is encrypted.
>>>> 1. One call to SSLEngine.unrwap() will read the record, and decrypt
>>>> the record.
>>>> 2. One call to SSLEngine.unwrap() cannot read the Certificate and
>>>> CerticateVerify handshake message only, without reading the Finished
>>>> handshake message. It means the unwrap() method will consume the
>>>> record data fully for all three handshake messages.
>>>> 3. The Certificate and CerticateVerify should be handled in
>>>> delegated tasks, so the call to SSLEngine.unwrap() return NEED_TASK.
>>>> 4. As the SSLEngine.unwrap() return NEED_TASK, it cannot return the
>>>> FINISHED status at the same time.
>>>> 5. The FINISHED status is only be able to return with
>>>> SSLEngine.unwrap() or SSLEngine.wrap(), and cannot returned from
>>>> delegated tasks. So the SSLEngine.getHandshakeStatus() after the
>>>> tasks cannot be used to indicate the FINISHED status.
>>>> 6. Then FINISHED status is missed, applications like Tomcat run into
>>>> problem, like the bug described.
>>>>
>>>> That's the problem of the bug as far as I can see. I agreed it is
>>>> not good to have an additional wrap() or unwrap() that did nothing,
>>>> but I have no better idea. It would be nice if we could have a fix
>>>> in JDK 15, considering the impact on Tomcat and Firefox. I'm open
>>>> if there is alternative solution or workaround.
>>>>
>>>>
>>>>> Maybe not put the finished message or put all these messages together.
>>>>>
>>>>> Maybe Brad may know of a way out of this problem? If creating a
>>>>> dummy message is the only way to fix this, then I'm ok with it. It
>>>>> is just not a clean fix in my mind.
>>>>>
>>>>>
>>>>>>> My understanding would have been after Finished because NST is
>>>>>>> suppose to be a post handshake message. So in this case there is
>>>>>>> no problem, correct?
>>>>>>>
>>>>>> Correct.
>>>>>>
>>>>>>> I'm trying to figure out why you need an empty NST. Is the
>>>>>>> problem when a number of messages are bundled together. For
>>>>>>> example, the Finished message with a partial NST, then Finished
>>>>>>> isn't processed and both sides are waiting? Or do both sides
>>>>>>> continue normal traffic, it's jut the HandshakeStatus.FINISHED is
>>>>>>> one operation behind?
>>>>>>>
>>>>>> It should be fine as empty NST is just a signal to indicate the
>>>>>> next call to wrap(). The next call to wrap() just use the signal
>>>>>> for the return of FINISHED status, not network data produced,
>>>>>> delivered or consumed.
>>>>>>
>>>>>>>
>>>>>>> One code comment so far:
>>>>>>> 433: The debug message purpose was to say the NST is a stateless
>>>>>>> ticket and not a preshared key. Can we keep "stateless" in the
>>>>>>> message?
>>>>>>>
>>>>>> NewSessionTicket.java? Sure, I may just want to shrink to one
>>>>>> line. It was not intended.
>>>>>
>>>>> shrinking the message is fine, it just needs to be clear which
>>>>> message ticket type got sent.
>>>>>
>>>> Added back.
>>>>
>>>> Xuelei
>>
More information about the security-dev
mailing list