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

TLS MUST NOT deliver server 1RTT Rx keys until getting Finished #3173

Closed
martinduke opened this issue Oct 30, 2019 · 13 comments
Closed

TLS MUST NOT deliver server 1RTT Rx keys until getting Finished #3173

martinduke opened this issue Oct 30, 2019 · 13 comments
Labels
-tls editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@martinduke
Copy link
Contributor

In #3159 it's becoming clear that some QUIC server implementations are using their 1RTT receive keys before getting CFIN, which is not supposed to happen.

IMO, there are going to be plenty of hacky QUIC implementations but only a handful of TLS implementations, especially TLS implementations that have the special APIs required for QUIC. This is the narrow waist where we should enforce good behavior.

Figure 5 of quic-tls shows that we shouldn't deliver the keys that early, but there's no normative language. I will file a PR shortly for quic-tls that imposes this requirement, so that QUIC implementations can't possibly mess this up.

@martinthomson
Copy link
Member

I like that this class of problem is naturally assumed to require some sort of protocol change. As I said on #3159, maybe those implementations can fix their bug.

#3174 is a pragmatic sort of fix that would make this an editorial issue. I'd like that, but it sounds like there is a strong desire to go all nuclear on this. We should discuss that, or its spectre will haunt us.

@kazuho
Copy link
Member

kazuho commented Oct 31, 2019

I think it's worth noting this, though I tend to agree with @martinthomson that we cannot write this down using a normative language because that is a requirement of TLS.

That said, I actually wonder if we can have a section discussing Implementation Pitfalls. TLS 1.3 has an appendix dedicated to that (see RFC 8446, Appendix C), and it has been extremely helpful to me. I think it would be nice to have something like that in the QUIC drafts.

@martinduke
Copy link
Contributor Author

martinduke commented Oct 31, 2019 via email

@kazuho
Copy link
Member

kazuho commented Oct 31, 2019

I'm not sure of the rules here, but this requirement is on the QUIC-specific interfaces.

IIUC, This requirement comes from the TLS handshake protocol defined in section 4 of RFC 8446 (specifically, section 4.4.4). TLS 1.3, DTLS 1.3, and QUIC all share the same handshake protocol.

@ghedo
Copy link
Member

ghedo commented Oct 31, 2019

Wouldn't this prevent the server from sending 0.5 RTT responses to 0-RTT?

@ghedo
Copy link
Member

ghedo commented Oct 31, 2019

Or, well, sending 0.5 RTT at all, not just for 0-RTT.

@kazuho
Copy link
Member

kazuho commented Oct 31, 2019

@ghedo Regardless of if we should use the normative language in QUIC-TLS, I think that RFC 8446, Section 4.4.4 clarifies what the requirements are, i.e.:

  • do not use the 1-RTT keys until the Finished is received
  • with the exception of 0-RTT
  • and also with the exception of 0.5-RTT, while keeping in mind that the peer's identity or liveness has not been asserted.

@ghedo
Copy link
Member

ghedo commented Oct 31, 2019

@kazuho yeah, that makes sense thanks.

@martinduke
Copy link
Contributor Author

@ghedo No, just the receive keys.

@martinduke
Copy link
Contributor Author

@kazuho I understand that the key applicability is a general property of TLS1.3; however QUIC is the only one where the secret is actually consumed outside of TLS. quic-tls defines this interface and we have the right to impose constraints on it.

@kazuho
Copy link
Member

kazuho commented Oct 31, 2019

@martinduke As said, it is not only QUIC. DTLS 1.3 also uses the TLS 1.3 handshake protocol while having a different packetization layer ("records" in TLS term).

That said, I am fine with letting the editors of the QUIC-TLS draft decide.

@mnot
Copy link
Member

mnot commented Nov 12, 2019

Calling this editorial, since it looks like it can be addressed without adding requirements.

@mnot mnot added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Nov 12, 2019
agl pushed a commit to google/boringssl that referenced this issue Nov 27, 2019
We want the QUIC/TLS interface to never release a read key without the
corresponding write key for ACKs. This is mostly done by shipping both keys
simultaneously, but 0-RTT is weird because it is ACKed by 1-RTT.

Note this means we actually release 0-RTT keys to the server *after* the 1-RTT
keys. This is kinda weird but more directly maintains our invariant.

(We may want to revisit the key configuring API in light of
quicwg/base-drafts#3159 and
quicwg/base-drafts#3173, but start with this more
local tweak.)

Bug: 303
Change-Id: I317fe6ae8150533738373c219f19d3034bb040ad
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/38884
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Nick Harper <nharper@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
agl pushed a commit to google/boringssl that referenced this issue Mar 3, 2020
The original API separated when keys were exported to QUIC from when
they were "active". This means the obligations on QUIC are unclear. For
instance we added SSL_quic_read_level and SSL_quic_write_level to
capture when keys were active, yet QUICHE never used them anyway. It
would be better to defer releasing keys to when they are needed.

In particular, we should align our API with the guidelines in
quicwg/base-drafts#3173.

This means we need separate read and write callbacks, which
unfortunately means the invariants around ACKs must now be covered in
prose.

We'll likely remove SSL_quic_read_level and SSL_quic_write_level in a
later CL as QUIC has no need to know this anymore. (It should simply
feed handshake data to BoringSSL as it sees it and, if we reject it,
report a suitably error.) The notion of a "current" encryption level is
a little odd given the interaction between 0-RTT and
ServerHello..Finished ACKs.

Update-Note: This is an incompatible change to SSL_QUIC_METHOD.
BORINGSSL_API_VERSION can be used to distinguish the two revisions.

Bug: 303
Change-Id: I6c9dca1ae156d26a80c366a4ba969dcb04e65349
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/40127
Reviewed-by: Nick Harper <nharper@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
@martinthomson
Copy link
Member

Closing now that we have merged #3224.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

No branches or pull requests

5 participants