RFR 8206929: Check session context for TLS session resumption

[Adam Petcher]

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. 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.

>
> 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).

>
> 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).

>
>
> 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.

>
> 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?). 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. 

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).

>
> 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.