Re: [tcpm] Comments on draft-ietf-tcpm-tcp-edo-13

["Iwashima, Kuniyuki" <[e-mail]>]

From: "[e-mail]" <[e-mail]>
Date: Wed, 16 Aug 2023 18:01:19 -0700
> Hi, Kuniyui (if I read your email address correctly)
> 
> > On Aug 16, 2023, at 3:47 PM, Iwashima, Kuniyuki <[e-mail]> wrote:
> > 
> > Hello Joe,
> > 
> > I'm Kuniyuki from AWS and recently working on this topic with Yoshi.
> > I have prototyped Extended Data Offset in Linux based on net-next.git and now
> > extending packetdrill to test EDO.
> > 
> > I have two comments about EDO fallback mechanism and option processing.
> > 
> > Let's say a middlebox merges the final ACK of 3WHS and the following segment
> > (3rd & 4th packets in the script below).
> 
> At that point, you have a misbehaving middlebox. It’s bad enough that it modifies TCP packets in-flight (that’s not supported by TCP), but that’s especially true if an unrecognized option is seen. At that point, the middle box should be “hands off’ that flow” at a minimum.
> 
> > ---8<---
> > 0   socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> > +0  setsockopt(3, SOL_TCP, TCP_EXT_DATA_OFFSET, [1], 4) = 0
> > +0  bind(3, ..., ...) = 0
> > +0  listen(3, 1) = 0
> > 
> > +0  < S 0:0(0) win 1000 <mss 1000,edoOK>
> > +0  > S. 0:0(0) ack 1 <edoOK, mss 1460>
> > +0  < . 1:1(0) ack 1 win 1000 <edo>
> > +0  < . 1:2(1) ack 1 win 1000 <edo>
> > ---8<---
> > 
> > If the client uses the longer variant or the middle box changes the header length, 
> > the server notices that the packet has an invalid EDO options.
> > So, the server falls back to non-EDO mode as mentioned in this part.
> 
> Here’s the flaw with even analyzing this:
> 
> 	TCP options are offered in a SYN by a client
> 		using EDO Supported, where the client goes to SYN_SENT
> 	TCP options are offered by by the server in the SYN+ACK
> 		using EDO Supported, where the server goes to SYN_RECEIVED
> 	TCP options are confirmed by the client to the server in the final ACK
> 		using EDO Extension where the client goes to ESTABLISHED
> 		and the client can now start sending data 
> 	TCP options are confirmed at the server upon receipt of the final ACK
> 		where the server goes to ESTABLISHED
> 
> Once in ESTABLISHED, packets without EDO Extension are considered invalid.
> 
> So here’s the case you describe above:
> 	TCP options are offered in a SYN by a client
> 		using EDO Supported, where the client goes to SYN_SENT
> 	TCP options are offered by by the server in the SYN+ACK
> 		using EDO Supported, where the server goes to SYN_RECEIVED
> 	TCP options are confirmed by the client to the server in the final ACK
> 		using EDO Extension where the client goes to ESTABLISHED
> 		and the client can now start sending data 
> >> (which it does - the 4th packet)
> >> final ACK and data packets get merged and the result has an invalid EDO Extension 
> 	TCP options are  NOTconfirmed at the server upon receipt of the final ACK
> 		so the server stays in SYN_RECEIVED
> 
> At this point, the connection will fail. The final ACK goes back to the server but will lack a valid EDO Extension.
> 
> So the client is in ESTABLISHED but the server stays in SYN_RECEVED. It will try to resend the previous ACK, so the server should resend the final ACK of the TWHS, which will NOT confirm receipt of the data the client sent.
> 
> At this point, no progress is made on any packets that the middlebox alters. So either one side times out or they both sit there and get no data through, at which point one side times out. At least that’s what I think so far…
> 
> Note that the server never “falls back” to non-EDO mode here. Malformed packets are silently ignored.
> 
> > ---8<---
> >>> The EDO Extension option MAY be used only if confirmed when the
> >   connection transitions to the ESTABLISHED state, e.g., a client is
> >   enabled after receiving the EDO Supported option in the SYN/ACK and
> >   the server is enabled after seeing the EDO Extension option in the
> >   final ACK of the three-way handshake. If either of those segments
> >   lacks the appropriate EDO option, the connection MUST NOT use any
> >   EDO options on any other segments.
> > ---8<---
> > 
> > If the server accepts the packet and 3WHS completes,
> 
> It doesn’t (see above).
> 
> > EDO is disabled at 
> > the server but enabled at the client.
> 
> That doesn’t happen; the server backs off only if the ACK gets through without EDO Extension, not if the option is there and malformed.

Thank you for explanation.
This is what I wanted to confirm and how my implementation behaves.


> 
> > The server will sends an ACK for the (originally 4th) segment without EDO, 
> 
> Won’t happen (see above).
> 
> > but the segment will be discarded at the client side.
> > In this case, the client cannot receive any ACK and data from the server.
> > 
> > So, I think the following parts MUST be applied to the final ACK of 3WHS as well.
> > It would be helpful to clarify that we MUST NOT complete 3WHS with an invalid EDO.
> 
> That’s possibly useful to explain, but it’s already in the requirements AFAICT. Please let me know if you see otherwise.

I read "lacks the appropriate EDO option" differently:

  (1) lacks EDO Extension in ACK of 3WHS
  (2) has an invalid EDO Extension in ACK of 3WHS

In the situation above, the server side (SYN_RECV) "has not been
negotiated" EDO yet because of (2), and I read the following part
as "option MUST be ignored"

---8<---
   >> If EDO has not been negotiated and agreed, the EDO Extension
   option MUST be silently ignored on subsequent segments.
---8<---

and was wondering if it conflicts with this part and which should be
prioritised.

---8<---
   >> ... When the
   EDO Header Length is invalid, the TCP segment MUST be silently
   dropped.
---8<---

If the server ignores the option only (this should not happen though),
the server can process the ACK and transitions to ESTABLISHED state.

Then, the server did not agree EDO and "MUST NOT use any EDO options
on any other segments", leading to non-EDO fallback.

---8<---
The EDO Extension option MAY be used only if confirmed when the
connection transitions to the ESTABLISHED state
---8<---


> 
> > 
> > ---8<---
> >>> The EDO Header Length MUST be at least as large as the TCP Data
> >   Offset field of the segment in which they both appear. When the EDO
> >   Header Length equals the Data Offset length, the EDO Extension
> >   option is present but it does not extend the option space. When the
> >   EDO Header Length is invalid, the TCP segment MUST be silently
> >   dropped.
> > ...
> >>> When an endpoint receives a segment using the 6-byte EDO
> >   Extension option, it MUST validate the Segment_Length field with the
> >   length of the segment as indicated in the TCP pseudoheader. If the
> >   segment lengths do not match, the segment MUST be discarded and an
> >   error SHOULD be logged in a rate-limited manner.
> > ---8<---
> > 
> > Also, after that, client may send out another packet for retransmission and 
> > newly queued data.
> 
> I don’t see that happening, as per above.
> 
> > On the server side, EDO is disabled, but the packet will be processed ignoring
> > only EDO option.
> > Then, options after 60 bytes would be recv()ed accidentally by user.
> > 
> > ---8<---
> >>> If EDO has not been negotiated and agreed, the EDO Extension
> >   option MUST be silently ignored on subsequent segments.
> > ---8<---
> > 
> > I think this behaviour is not intended.
> > So, I think it's better to make it clear that what MUST be ignored should be
> > the whole segment, instead of EDO Extension option.
> 
> 
> We did say that elsewhere (the MUST NOT in the first quoted text above), but yes, this one should have said to drop the segment, not just the option.
> 
> Was that the only change?

Another change I added in my kernel is like:

  EDO Extension MUST NOT be used more than once in a segment.

In the current draft, we can use multiple EDO Extension if all of them
meet these conditions:

  (1) The EDO Extension option MUST occur within the space indicated by
      the TCP Data Offset.

  (2) The EDO Header Length MUST be at least as large as the TCP Data
      Offset field of the segment in which they both appear.

For example, we can craft a header like this.

  doff : 15 (60 bytes)
  EDO1 : 61
  EDO2 : 62
  EDO3 : 63
  ...
  <-- 60 bytes -->

This just slows down parsing and could be exploited for DoS.

Also, an EDO could have length less than other preceding EDO, and this
is confusing.

  EDO1 : 80
  EDO2 : 65
  EDO3 : 70

I do not see a good reason to support these use cases.  So, to make EDO
handling simpler and safer, I think we MUST restrict this kind of EDO 
Extension use.