Subject: Comments on draft-ietf-quic-transport-00

From: Martin Duke <[email-masked]>
Date: Wed, 30 Nov 2016 21:41:10 -0800

This draft has made a lot of progress in a short time. Most of my comments
center around extensibility, a few DoS vulnerabilites, and a few minor
details that make life hard on implementers.

Substantive Issues:
Section 4.1. Common Header. I know the flags are already changing pretty
rapidly here, but it's important to reserve one of the bits to indicate
that there is another byte of flags immediately following. This allows new
versions that need more public flags to correctly parse the version
negotiation fields.

It is also appropriate to define exactly what can and cannot change in the
public flags field between versions. New versions MUST NOT change the
location or existence of the VERSION and MORE_FLAGS bits, as that will
break version negotiation. All QUIC hosts, of any version, MUST process at
least these two flags to remain compatible with other versions. The same
would also be true of CONNECTION_ID were it not for the recommendation in
Section 4.2 below.

In the same spirit, it might make sense to alter the flow chart so that the
VERSION flag is processed first. The PUBLIC_RESET strikes me as a packet
that introduces some potential vulnerabilities, and therefore something
that future versions might want to substantially revise.

Section 4.2. Regular Packets. The version field should immediately follow
the public flags, in case future versions mess with the length of the
connection ID and the receiver must correctly parse the header to find the
version. Regrettably, that means the connection ID will not always be 1
byte into the packet, but we already have to read the bits to find the conn
ID.

Section 4.2.1. Should receivers enforce that packet numbers are long
enough? I don't feel strongly about this either way, but I'm somewhat
concerned that sender and receiver may not quite agree what is "in flight"
if a STOP_WAITING frame is in flight, lost, or otherwise delayed. For
instance, if the sender sends a STOP_WAITING that moves the window by a
large number of packets, which is lost, and then sends a packet with a
truncated packet number, there could be issues.

Perhaps we need to tighten up the language here.

Section 4.2.2 Frame types. I think we need an "experimental" frame type,
followed by a few bytes of additional identifiers, like option 254 in TCP.
Use of these frames would be negotiated by COPT, though to be extra-careful
we could also add a length frame (e.g., we might need a new frame during
the handshake, before COPT negotiation).

Section 4.4. Public Reset. There's been a lot of discussion of this on the
list, and I've already said a lot there. Suffice it to say there's work to
do to properly authenticate this packet, certainly against off-path
attacks, and ideally against eavesdroppers as well. There was also talk of
an optional waiting period before tearing down after a Reset, in case the
reset is spoofed.

Separately, what is the Connection ID we MUST use if the client never
generated one?

Section 5.1. Version Negotiation. This mechanism is completely vulnerable
to DoS by eavesdropper at connection startup. An attacker can sniff the
client hello and spoof a server version negotiation message that only
advertises nonexistent versions, preventing connection.

As with Public Resets, a short waiting period before abandoning the
connection would solve the problem.

Section 5.2.1. ICSL is missing here, even though it is listed as mandatory
in 5.4.

Section 5.2.2. You mention "stateless rejects" here, but the draft could
really use a full section on it. In particular, it's not clear how QUIC
would determine that the crypto handshake was incomplete after one message.
I presume this is the case for the stateless reject?

Section 5.3. Do we need an ADD_ADDR message to advertise additional
addresses, in the event that address is unreachable from the peer's
currently known addresses?

Section 5.4. Why is ICSL required if there is a default?

What is a "silent close option?" Is this negotiated or just a
sysctl/sockopt? If negotiated, we need more details.

Abrupt shutdown. "Commonly, a public reset is expected to be sent when a
       packet on an established connection is received by an endpoint
       that is unable decrypt the packet."

I *think* you mean an endpoint should send a public reset should be sent
when a packet arrives that does not initiate a handshake and is not
associated with an established connection or handshake in progress. If an
endpoint with an established connection were to reset on any decryption
failure, that would allow an off-path attacker to tear down the connection
with a garbage packet if it guesses the conn-id and a reasonable packet
number.

Section 6. Frame Formats. Another broad comment: one common task will be
take a piece of a packet corresponding to a frame and passing it into some
handling function. Similarly, we would want some basic sanity checks on
frame length (i.e. <= remaining packet length) before starting processing.
It would be helpful if it were easy to parse the first few bytes of the
frame to find its length. Length computation requires reading the flags,
and sometimes checking additional fields that are variable lengths from the
start of the frame. This is very cumbersome to implement. If you're trying
in plain old C, as I am, the macros are horrendous.

Instead, I recommend that all fields necessary for frame length computation
are first in the frame.

For example, Section 6.1, stream frames: I recommend Data length, if
present, comes right after the type field.

In Section 6.2, ack frames, "Number Blocks" and "Num Timestamps" should
move to the front of the frame.

section 6.2 Ack frames: I am unclear about the procedure for including ACK
blocks in a particular frame. Delivery of an ACK block a single time is
sufficient, but ACK delivery is unreliable. So is a receiver supposed to
repeat an ack block again and again, or do it once? I *think* the answer is
that if a packet with ACK frame has itself been acknowledged, we need not
end that ack block anymore. But it'd be good to explicitly state that.

Section 7. PMTU discovery: RFC-Compliant ICMP PMTU messages include the
oversize packet's IP header and the first 8 bytes of the transport. This
allows  TCP to demultiplex the ICMP to the relevant connection/path, and
check that the echoed sequence number corresponds to an in-flight segment,
and that the segment that would have violated the advertised MTU. As TCP
windows increase, it's true that this may be less effective.

For QUIC, these messages will contain only the UDP header. I believe this
is sufficient to demux to the right connection (QUIC allows the same
4-tuple for multiple connection IDs, but it would be fine to apply it to
all such connections), but I see no clear mechanism to prevent a blind
attacker that guesses the 4-tuple from forcing a QUIC connection to revert
to the min path MTU. Some possibilities:
    (a) Require routers to send more bytes. This is probably not
deployable, and in any case because packet numbers always start at zero
this doesn't add much security.
    (b) Store UDP or IP data. We could require QUIC to store the UDP
checksum or IP ID in its send queue, thus allowing it to associate the
returned headers with a packet in its queue. This is ugly from a layering
perspective and may not be plausible with hardware offload of checksums,
various middleboxes, etc.
    (c) Isolated DF packets. QUIC could pick a long time interval (10
seconds?) in which is sets DF on a single MTU-sized packet (which may have
to be padded). QUIC would only accept ICMP messages within an RTT or so of
having sent one of these MTU probe packets, or until it's acked. Thus the
probabability an off-path attacker could time it correctly is (RTT / 10
sec). If there's an extended burst of ICMP to overcome this, that's
probably an attack indication to handle either in IP, the firewall, or at
worst in QUIC.  I have some language to codify this, and if we agree this
is the right approach I can share it.

Section 9. Why are BLOCKED frames a MUST? They are sort of like TCP persist
probes, but WINDOW_UPDATE messages are delivered reliably in QUIC, so
BLOCKED has no input on operation. If the purpose is debugging only this
ought to be no more than a SHOULD, and probably a MAY.

9.1. Similarly, I don't see how there can be deadlock if WINDOW_UPDATE MUST
be retransmitted.

Nits:
Abstract: "from from" -> "from"

Section 4. Packet Types and Formats. Per the consensus in Seoul, all values
are now big-endian.

Section 6.2 Ack Frame. "It is recommended for the sender to send the most
recent largest packet it has received in an ack..." -->

"It is recommended for the sender to send the most recent 'largest acked'
field it has received in an ack..."

Section 6.10 GOAWAY frame. "will likely be aborted..." -> "will likely be
closed..."

Section 7. Withholding packets to wait for more frames: I suggest some sort
of bound on how long the sender can wait.

Section 7: Retransmitted Frames. What is the force of "the sender SHOULD
only send frames..." when the specifics below it cover all cases with MUST?

---

From: "Aron ." <[email-masked]>
Date: Thu, 1 Dec 2016 01:00:44 -0500

> in plain old C, as I am, the macros are horrendous.

Have the frame handling function return the number of bytes it consumed or
-1 on error.

  Aron.