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

Improve KEY_PHASE description #43

Merged
merged 15 commits into from
Dec 9, 2016
Merged

Improve KEY_PHASE description #43

merged 15 commits into from
Dec 9, 2016

Conversation

martinthomson
Copy link
Member

@martinthomson martinthomson commented Nov 29, 2016

There's a bunch of things that were broken with the original description of KEY_PHASE. This restructures things a little, moves the basic packet protection above the section on KEY_PHASE and changes the algorithm.

There is still a nasty trial-decryption-like thing that the server has to do during the handshake, which I'd dearly love to kill, but I haven't worked out how to fix it.

Builds on #30.

I've improved terminology throughout the early sections and tried to
be more precise.  I probably need to add a picture or two to help.

The latter sections still need work.

I want to move the "system component" diagram that I had to the main
draft.  It would be good to have a topological map of all the pieces.
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -tls labels Nov 29, 2016
Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

Thanks for the huge update.

negotiation of keying material happens after the QUIC protocol has started.
This simplifies the use of TLS since QUIC is able to ensure that the TLS
handshake packets are delivered reliably and in order.
QUIC reserves stream 1 for a TLS connection. Stream contains a complete TLS
Copy link
Contributor

Choose a reason for hiding this comment

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

Stream contains is awkward. Did you mean Stream 1 contains?

on QUIC operation during this phase.
Initial packets are not authenticated or confidentiality protected.
{{pre-handshake}} covers some of the implications of this design. This imposes
some restrictions on what packets can be sent. There are also restrictions on
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems mostly redundant. Actually, most of this paragraph seems redundant to the one on line 162 and part of the paragraph at 214.

protection. However, keys derived at this stage are not exported for use in
QUIC. QUIC frames from the server are sent in the clear until the final
protection. These keys are not exported from the TLS connection for use in
QUIC. QUIC packets from the server are sent in the clear until the final
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the transition from frames to packets? I think frames is slightly more accurate, since a server could send a ServerHello, then encrypted data, and then have to resend the ServerHello in a new packet with a larger packet number.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to be more consistent about talking about packets when it comes to packet protection. If I need to talk about frames, it's only to the extent that those frame contain TLS handshake messages. What is most relevant is either the TLS handshake messages, or the packet. Frames are sort of sandwiched in the middle there.

the connection is fully enabled, the KEY_PHASE bit can toggle between 0 and 1 as
keys are updated (see {{key-update}}).
A QUIC server starts the process by providing TLS with any stream 1 octets that
might have arrived. Only in-sequence packets are delivered; packets that arrive
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can shorten this section(up to 379) a bit. The first sentence says most of the important information. QUIC streams mean in-order delivery, so just saying in-order once should be sufficient. There's no need to describe how QUIC provides in order delivery.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. Shortening this a lot.

KEY_PHASE bit can update keys and decrypt the packet that contains the changed
bit, see {{key-update}}.

The KEY_PHASE bit on the public flags is the most significant bit (0x80).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change this to the diversification nonce bit(0x04), since that's not needed with TLS 1.3 crypto.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to do that in a single PR. I was sorely tempted to do everything at once, but that PR will hit two files and I want to keep it discrete.

{{key-phase-table}} shows that a client marks 1-RTT with a different KEY_PHASE
bit depending on whether 0-RTT is attempted. Attempting 0-RTT therefore results
in fully protected data having different KEY_PHASE values. This is true even if
0-RTT data is rejected and ignored by the server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? If the 0RTT packets are never received by the server, wouldn't 0RTT with packet loss and 1RTT be identical from the server's perspective, making it look like 0 for handshake and 0 for 1RTT data, with no 1 in between.

I think the approach of using the version bit, possibly requiring the version bit to be set for handshake packets, then requiring it not be set for 0RTT data, would be a reasonable option? There are few ways to make it work, but the above doesn't seem like it works, and generally seems unnecessarily complex.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we might want to use the version bit for disambiguation.

0-RTT going missing isn't a big deal. A server that receives 1-RTT encrypted data must have 1-RTT keys AND it doesn't need to receive any more cleartext handshake messages (it must have read the ClientHello in order to have responded to it). Thus, the only catch is with unprotected ACK frames that arrive late. I go into this in more detail further down.


### Retransmission and Acknowledgment of Unprotected Packets

The handshake messages the first flight of handshake messages from both client
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a sentence?

keys (see {{key-ready-events}}).

Retransmissions of these handshake messages MUST be send in unprotected packets
(with a KEY_PHASE of 0). Any ACK frames for these messages MUST also be send in
Copy link
Contributor

Choose a reason for hiding this comment

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

I think acks can be encrypted. In particular, one could ack encrypted data and unencrypted data in the same ack. Otherwise you'd have to clear out all ack data as soon as you transitioned keys. It's possible, but a bit weird and it seems unnecessary. As discussed, it's more important we don't ack encrypted data with unencrypted acks.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem we have is that an ACK might not be readable if it is encrypted. See #34.

  1. If the server ACKs the ClientHello by encrypting it, that's probably OK, it has to do that with 1-RTT keys. But the client might not have the 1-RTT keys because it needs the entire set of server handshake messages to that.

  2. If the client ACKs the ServerHello (et al.) with 0-RTT keys, then those ACKs are lost if the server rejects 0-RTT. I guess you could say that this is OK on the basis that we lose ACKs too, but I think that it could force the server into retransmission on a timer (ugh).

  3. If the client ACKs the ServerHello (et al.) with 1-RTT keys, then the server should be able to decrypt them perfectly well. However, the only value gained from this is better feedback on timing, it doesn't help with loss recovery because there was none.


TBD:

: We could observe/require that all unprotected packets are marked with a
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, it also means simplifying a bunch of text that says "If you have 0RTT data, then X, otherwise Y"

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't remove the asymmetry, but yeah, it's probably worth doing still.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to have VERSION bit set not only for unprotected packets but for 0RTT encrypted packets as well. For 2 reasons:
1)VERSION number tells the server how to interpreter packet, so if ClientHello packet gets lost and server receives 0RTT encrypted packet it will not know how to read it because it does not know which protocol version it is. (if we do not change the packet format for different version it might be ok, but we do not want to ossify on this). In that case it would need to wait for ClientHello and it will not be able to send acks to trigger retransmits, the version change request from server will be delayed,etc.
2) we can remove asymmetry: unprotected packet have KEY_PHASE=0 and VERSION=1, 0RTT packets have KEY_PHASE=1 and VERSION=1 and 1RTT packet have KEY_PHASE=1 and VERSION=0 (which will be the same on the server side and the same as the case when 0RTT is not used)

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this logic. I will make that change.

I suspect that servers will likely have very different tolerances for 0-RTT packets, particularly in the absence of the ClientHello. The ClientHello will probably be the only place that we can put a source address validation token.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, let's go with this.

In addition to denying endpoints messages, an attacker to generate packets that
cause no state change in a recipient. See {{useless}} for a discussion of these
risks.
In addition to causing valid packets to be dropped, an attacker to generate
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a sentence

@martinthomson martinthomson merged commit b57a13c into master Dec 9, 2016
@martinthomson martinthomson deleted the 0rtt-phases branch December 9, 2016 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants