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.
