RFR 8206929: Check session context for TLS session resumption

[Xuelei Fan]

On 7/13/2018 12:13 PM, Adam Petcher wrote:
> On 7/13/2018 1:35 PM, Xuelei Fan wrote:
> 
>> I think we need to check more aspects, for both the session resumption 
>> producer and consumer:
>> 1. the local is able to resume the session.
>> 2. the peer is able to resume the session.
>> 3. the change of the security parameters does not impact the resumption.
> 
> 1 and 3 I agree with, and I believe are pretty well covered (after this 
> change), but I'm still not convinced that the implementation should do 
> any checking on behalf of the peer.
It is not necessary to be on behalf of the peer.  The local can check if 
the peer request something correctly.  We normally call it input validation.

> This includes checking the peer 
> supported signature algorithms. Maybe this is a good idea, but I think 
> it is not in scope of this ticket, and it should be discussed 
> separately. If you want to pursue this, feel free to open another ticket 
> for it.
> 
I would treat it as a part of the bug.  I'm fine if you just want to fix 
half of it.  I need a confirmation.  Please let me know if you want 
another half be addressed in a new bug.

>>
>> So, for the protocol version checking in server side:
>> 1. the client requested to use the negotiated version.
>> 2. the server still supports the negotiated version.
>>
>> I think #2 is get checked in line 406-414 block. 
> Yes.
> 
>> Did we check #1 in somewhere else?
> 
> A lot of the checks for the client side uses existing code, and is 
> shared between TLS 1.3 and earlier versions. The way it works is that 
> the shared code will decide which session to resume in ClientHello. At 
> this point, a lot of things are checked, including the protocol version 
> of the session. Only if this code decides that a session should be 
> resumed, then the code for TLS 1.3+ in PreSharedKeyExtension will 
> perform the additional checks that are specific to TLS 1.3 and then 
> produce the extension.
> 
> Specifically, the protocol version is checked in ClientHello.java near 
> line 458 (in existing code).
> 
I meant to ask whether server side validate the client input.  See what 
we did for TLS v1.2 in line 973 of ClientHello.java.

>>
>> For the cipher suite checking in server side:
>> 1. the client requested to use the negotiate cipher suite.
>> 2. the server still supports the negotiated cipher suite.
>>
>> I think #2 is get checked in line 445-453 block. 
> 
> Yes.
> 
>> Did we check #1 in somewhere else?
> 
> ClientHello.java near line 445 (in existing code).
> 
I meant to ask whether server side validate the client input.  See what 
we did for TLS v1.2 in line 1003.

>>
>>
>> For the signature algorithms checking in server side:
>> 1. the client requested to use the algorithms used in full handshake
>> 2. the server still supports the algorithms used in full handshake
>>
>> The algorithms used in full handshake is one element or a subset of 
>> the local and peer supported algorithms.   We may not cache the used 
>> algorithms in the session.  But it is acceptable to me to use 
>> defensive policy that we don't allow session resumption if the 
>> supported algorithms get changed (as you did in the webrev to use 
>> Collection.containsAll() line 435).
> 
> We do cache the certificate chain. If we wanted to, we could examine the 
> chain and make sure all of the signature algorithms in the chain are 
> still supported. I didn't think this was worth the effort, though.
> 
I'm fine to use the defensive policy.  I don't purchase for a perfect check.

>>
>> As reminded me that we may also want to check for both the 
>> signature_algorithms and signature_algorithms algorithms.
> 
> Did you mean signature_algorithms and signature_algorithms_cert? I think 
> that is handled correctly now, because the set of (local) supported 
> signature algorithms for certificates and handshake messages will always 
> be the same (right?).
Right, the local algorithms checking should be sufficient.

> We only have one field for this in 
> HandshakeContext: localSupportedSignAlgs. The way it is used in the 
> code, this field is used for both handshake messages and certificates.
>
>>
>> I think #2 is get checked in line 431-442 block. 
> 
It's for local algorithms.  We should valid the peer input as well.

> Yes.
> 
>> Did we check #1 in somewhere else?
> 
> Because this is specific to TLS 1.3+, I put it in the 
> CHPreSharedKeyProducer. That is in PreSharedKeyExtension.java, near line 
> 610 (in the new code in the Webrev).
> 
Yes, it is a good check the generation.  We also need to valid the peer 
input as well, in the consumer.

In the producer, it is call local supported signature algorithms; in the 
consumer, the corresponding one is peer support signature algorithms. 
Peer support signature algorithms should be validated for session 
resumption, I think.

Thanks,
Xuelei

>>
>> On 7/13/2018 9:10 AM, Adam Petcher wrote:
>>> On 7/13/2018 11:34 AM, Xuelei Fan wrote:
>>>
>>>> PreSharedKeyExtension.java
>>>> --------------------------
>>>> The local supported signature algorithms are checked in the 
>>>> canRejoin() method.  Should the peer supported signature algorithms 
>>>> be checked as well?
>>>
>>> I don't think so. When the peer creates its PreSharedKeyExtension, it 
>>> should only offer sessions (i.e. PSK identities) that it is willing 
>>> to resume. This includes checking for its supported signature 
>>> algorithms, or any checks that are required by its policy.
>> I see your point.  But the peer is not trustworthy so that we normally 
>> validate the input to make sure the peer is doing the right thing.
>>
>> Thanks,
>> Xuelei
>>
>>> If the server gets a PSK identity from the client, then server should 
>>> use that PSK to resume a session as long as it is acceptable 
>>> according to the server's policy. Trying to figure out the peer's 
>>> policy and enforce it is error prone and adds unnecessary complexity.
>>>
>>> Though maybe I'm missing some other motivation to add this check.
>