Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warning about agreement on Client certificate bytes. #76

Merged
merged 4 commits into from Mar 4, 2022

Conversation

jhoyla
Copy link
Contributor

@jhoyla jhoyla commented Aug 24, 2021

In the TLS 1.3 handshake the client and server do not agree on the client's certificate bytes until after the first server application message.
If this is significant to an application, then the server should send an application message before authenticating the client with an EA.

Copy link
Collaborator

@grittygrease grittygrease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double-space after the period. Otherwise fine.

@jhoyla
Copy link
Contributor Author

jhoyla commented Oct 26, 2021

Double-space after the period. Otherwise fine.

Done.

Fix nit.

Co-authored-by: Armando Faz <armfazh@users.noreply.github.com>
@richsalz
Copy link

This is useful information to convey to implementers and users. +1

negotiated out-of-band it is possible to negotiate EAs without agreeing on the
entire transcript. Servers SHOULD send application data before sending a
CertificateRequest to the client. If there is no application data to send the
server MAY send a NewSessionTicket.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewSessionTicket is not an application message, so the previous text does not actually say that receiving NST would suffice to produce full-transcript agreement. (It would, of course, since NST is protected by application traffic keys, but we should probably tweak some text to be more precise.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest adding "(because that is also protected by application traffic keys)."

@seanturner seanturner requested a review from ekr November 9, 2021 16:09
@seanturner
Copy link
Contributor

@ekr the message that started this all is in this email.

It's the second issue.

@jhoyla
Copy link
Contributor Author

jhoyla commented Nov 9, 2021

@ekr To follow up from the meeting, this is only relevant in the case where the server has requested a client certificate, which is why you can't predict the client Finished. For the Server to send an NST it must therefore have received the entire handshake. Changing this to require the Server to wait for the end of the handshake in not client certificate situations seems a touch heavy handed.

@martinthomson
Copy link
Contributor

@jhoyla the text (as shown in the session) says that the server sends something though. But the server can do that without seeing the client certificate. I think that this needs to be something different, perhaps "the server has received and validated the client's Finished".

@@ -511,6 +511,13 @@ The signatures generated with this API cover the context string
"Exported Authenticator" and therefore cannot be transplanted into other
protocols.

In TLS 1.3 the client and server are not guaranteed to agree on the client’s
final flight until the first application message. Because EAs can be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted, this first is false. 0-RTT or 0.5-RTT both violate this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MT is right. You need to delete the first sentence. just the second two are sufficient. Otherwise, this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do get a guarantee of agreement eventually, though nailing down a concise way to describe exactly when that happens may take some thought. IIRC it's successful use of application_traffic_secret_0.
I think the phrasing would be a little awkward if we just jump straight in as "Because EAs can be negotiated out-of-band it is possible to negotiate EAs without agreeing on the entire transcript". If we don't want to mention the lack of transcript agreement at all, we might flip it around, "It is possible to negotiate EAs without agreeing on the entire transcript, as might occur if [...]". We might also make the first sentence blander without removing it entirely, to mention that the non-agreement on the client's final flight is possible/a risk, without making a specific claim of when the risk goes away.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that that's true. application_traffic_secret_0 does not include anything from the client's last flight.

Copy link
Contributor Author

@jhoyla jhoyla Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do people think of this text instead:

In TLS 1.3 the client does not explicitly learn whether its Finished message
was accepted. Because the application traffic keys are not dependent on the
client's final flight, the client cannot learn whether the server ever received
it.  To avoid disagreement between the client and server on the authentication
status of EAs, servers MUST verify the client Finished before sending an EA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a good approach. I might spend a few words for "client cannot learn just from TLS messages" or similar, since some application protocols on top of TLS might enable the client to do so. But I don't think it's strictly necessary to have such a clarification.

@ekr
Copy link
Contributor

ekr commented Nov 9, 2021

@ekr To follow up from the meeting, this is only relevant in the case where the server has requested a client certificate, which is why you can't predict the client Finished. For the Server to send an NST it must therefore have received the entire handshake. Changing this to require the Server to wait for the end of the handshake in not client certificate situations seems a touch heavy handed.

I'm still trying to work out what threat we are considering here. Is it the server pretending to read the Finished and not or is it the server making an error and not reading the Finished. In the former case, it's not sufficient to send the NST because the client doesn't know what RMS the server has computed. In the latter case, why not just tell the server not to send a request until it's read Finished if there is a client cert?

@jhoyla
Copy link
Contributor Author

jhoyla commented Nov 18, 2021

At the end of a mutually authenticated handshake the client doesn't know that the server has agreed on the ClientCertificate's bytes. The threat is that the client's last message might have been corrupted in flight (intentionally or otherwise).
I was worried that if we put the text "MUST verify the Finished before sending an EA." implementations might ignore it, so the goal was to make the check something externally visible. I think you're right that the current text doesn't achieve the security goal. I can put "MUST verify the Finished" if that works better.

@ekr
Copy link
Contributor

ekr commented Nov 18, 2021

I think that's the only thing that works.

@seanturner
Copy link
Contributor

@jhoyla Just checking to see when this might be updated?

@jhoyla
Copy link
Contributor Author

jhoyla commented Nov 29, 2021

Changes pushed.

@grittygrease
Copy link
Collaborator

@ekr Does the final change address your issue?

@ekr
Copy link
Contributor

ekr commented Feb 8, 2022

[Clarifying]

In TLS 1.3 the client and server are not guaranteed to agree on the client’s final flight until the first application message."

The traffic keys do not depend on the client's final flight at all. So, I don't see what the "first application message" does here. The client receiving the server's messages doesn't tell the client anything, and receipt of the client's first application data isn't really relevant here. It's the Finished that matters.

@kaduk
Copy link
Contributor

kaduk commented Feb 9, 2022

Maybe something about how we rely on the server to not send application data if the client Finished fails to validate (and thus for application data to not be exchanged in case of tampering), rather than talking about "agree on the client's final flight"? That lines up nicely with the new MUST-level requirement to not send EA if the client Finished doesn't validate.
So something like "In TLS 1.3 the agreement on the client's final flight is enforced solely by the server. The client Finished hash incorporates the rest of the transcript, which the server uses as an integrity check over the entire transcript, including the client's final flight." and then tweak the last sentence to include a clause about "in order to ensure that the client's final flight has been validated, servers MUST ..."

@chris-wood
Copy link
Contributor

@jhoyla, @grittygrease: can you please coordinate and come to a resolution that addresses the remaining comments in this thread? In particular, I think we're seeking clarity on the conditions for agreement and how that relates to the client's finished message (and not necessarily application data, though that may imply agreement, if I'm understanding the intended text here).

Copy link
Contributor

@chris-wood chris-wood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes to resolve remaining comments.

@ekr
Copy link
Contributor

ekr commented Feb 19, 2022

Maybe something about how we rely on the server to not send application data if the client Finished fails to validate (and thus for application data to not be exchanged in case of tampering),

What about 0.5RTT?

@chris-wood
Copy link
Contributor

@grittygrease @jhoyla can we please get an update here? Would additional editors here help the process?

@kaduk
Copy link
Contributor

kaduk commented Mar 1, 2022

Maybe something about how we rely on the server to not send application data if the client Finished fails to validate (and thus for application data to not be exchanged in case of tampering),

What about 0.5RTT?

Yes, the server can send 0.5RTT before the handshake completes. That's why my focus was on "we rely on the server", as the client doesn't always have a great way of telling for sure. The application protocol's semantics might be able to clue that a given response was 1-RTT and not 0.5-RTT, but it might not, too. I probably should have said "close the connection" rather than "not send application data", or maybe "not continue to send application data".

@ekr
Copy link
Contributor

ekr commented Mar 1, 2022 via email

@jhoyla
Copy link
Contributor Author

jhoyla commented Mar 2, 2022

What do people think about putting "before sending an EA or processing a received EA." instead of "before sending an EA"?
I realised it's possible for the server to receive an EA before the client Finished as follows:
Server sends its Finished message and then immediately an AR.
Client receives the Finished, sends its own Finished, and then an EA in response to the AR.
The client Finished and EA are reordered by the network (the EA might not even be going over the TLS transport).

The server might then accept the EA and do some action, but then reject the Finished message.
I think this is already incorrect by the definition of EAs, but I thought it wouldn't hurt to make it super explicit.
@ekr

Co-authored-by: ekr <ekr@rtfm.com>
@grittygrease grittygrease merged commit 462851d into tlswg:master Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants