[Int-dir] Re: Intdir telechat review of draft-ietf-tsvwg-udp-options-38

[Antoine FRESSANCOURT <[e-mail]>]

Hello Mike and Joe,

Thanks a lot for taking the time to write this detailed answer to the points I mentioned in the review, I will add my comments to your answer in the text of the previous email, prefixed with [AFT].

Best regards,

Antoine
--
Reviewer: Antoine Fressancourt
Review result: Ready with Nits

I am an assigned INT directorate reviewer for
draft-ietf-tsvwg-udp-options-38.txt. These comments were written primarily for
the benefit of the Internet Area Directors. Document editors and shepherd(s)
should treat these comments just like they would treat comments from any other
IETF contributors and resolve them along with any other Last Call comments that
have been received. For more details on the INT Directorate, see
https://datatracker.ietf.org/group/intdir/about/
<https://datatracker.ietf.org/group/intdir/about/>.

Based on my review, if I was on the IESG I would ballot this document as YES or
NO OBJECTION.

The following are comments that came to my mind while reading the document. I
would enjoy a clarification about those comments (with no obligation,
obviously):

In general, the document is well written and structured. I was not familiar
with the idea of UDP options before being asked to review this document, and I
have been intrigued about this design option. In the frame of this review, I
also carefully read [Zu20] as deployment aspects of this design seemed to me
key in the potential use of UDP options.

While reading the document, I was asking to myself for a rather long time why
the authors opted for placing the UDP option after the UDP data. I think the
surplus area gap between the IP length and the UDP length should be presented
earlier in the text, right after the 3rd paragraph of section 4.

The fourth paragraph of Section 4 explains this already. We would prefer to
leave the figures where they are.

[AFT] Noted

In the same way, I was kept wondering for a rather long time whether
intermediate (middlebox) nodes may add or remove UDP options on the flight.
This is prohibited with a sentence appearing in section 13. I would state this
in section 6 about the design principles, and in any case before the FRAG
option is introduced to rule out the possibility for a middlebox to further
fragment a UDP fragment.

Traditionally, the expectation has been that fields that are modified in transit
reside at the network layer, not the transport layer, so this does not (to us)
seem to be a design principle that is specific to UDP options. Section 16 does
address this issue generally. In the absence of a request from other reviewers,
we would prefer to leave the order of the presentation as it is.

[AFT] I have the same traditional opinion as you about the fact that anything at layer 4 and above should be left unmodified in transit, and should not be modified. Yet, NATs exist, and very recently there has been a quite heated discussion about the possibility for transit nodes to check values for transport layer checksums in another working group. Thus, I felt this clarification would be helpful in this document. Yet, I hear your point of view about it, and I let you determine whether it is appropriate or not to mention this in your document.

Talking about section 6, I find it odd that potential issues related to
fragmentation are presented in the last paragraph of the section before we got
more information about the fragmentation mechanism presented in section 11.4.

Would a forward reference to Section 11.4 be helpful?

[AFT] I think so.

After reading about the surplus area idea, I wondered why such an inconsistency
was kept, and looking at some OS code (e.g. FreeBSD), it appears that some OS
check the various length fields' consistencies. It seems to me a rather sane
behavior given the possibility to use the surplus area as a side channel to
convey data related to an attack (rather than an ossification factor as
presented in [ZU20]). Given this context, we may consider that the UDP option
mechanism presented in this document is an opportunity to clarify the use of
the surplus area and close this inconsistency's gap. Then, I am wondering why
there is still a possibility for the presence of a surplus area behind the EOL
option. In my humble opinion, given that the document mentions that UDP options
can't be introduced by on-path nodes, the document should close the gap left
open by length fields inconsistencies.

We note that RFC 768 does not explicitly prohibit the presence of surplus
area, and all systems whose code we have inspected (including FreeBSD) simply
ignore it if it is present (more on this below). To us, concerns about
side-channel attacks on an unauthenticated transport protocol that can sit
inside an IP header with arbitrary options seems misplaced. Everything is a
possible side-channel attack unless it’s under control (e.g., encrypted).

That being said, we think that the UDP options specification may actually
improve the situation for transports for which this is a concern:

- The spec rules out any use of the area past EOL by requiring zero-fill
for the part of the surplus area after EOL (Section 11.0).

- The spec also levies a requirement on option-aware UDP implementations to
provide an API setting that allows an upper layers to request that the UDP
layer discard packets with a non-empty surplus area (Section 15), something
that is currently not generally available.

[AFT] I get that, but in my view, the document should be more directive about discarding UDP packets with non-zero content after the UDP options (and have an option NOT to discard it, for instance).

Regarding authentication and encryption options introduced in section 10, after
possible authentication and encryption options were presented, I was wondering
what would such options add compared to QUIC or DTLS. Potential gap analysis
between those existing protocols and future authentication and encryption
schemes should be done in the future by UDP option designers addressing those
challenges.

We think that such a discussion would be out of place in this document. It
would be entirely appropriate subject matter for a future specification of UDP
authentication and/or encryption.

[AFT] I agree, I wanted to clearly state that such a gap analysis was necessary in my perspective.

In section 8, the reason for using zeros before the beginning of the OCS
checksum is not mentioned. It should be clearly stated in the document: does it
comply with an alignment constraint about UDP mentioned in another document? Is
it arbitrary?

The following text will appear in -41:

These alignment bytes, coupled with OCS as computed over the
remainder of the surplus area, ensure that the ones-
complement sum of the surplus area is zero. OCS is half-word
(2-byte) aligned to avoid the need for byte-swapping in its
implementation.

Please let us know if this addresses your concern.

[AFT] This text answers my concern. Thanks.

In section 9, I would add another subfigure in Figure 4 presenting the case in
which the UDP data ends at the first byte of the 4 bytes representation to
clearly show that the intention is to align on 2 bytes boundaries:

   +--------+--------+--------+--------+
   |UDP data|    0   |       OCS       |
   +--------+--------+--------+--------+

 Besides, in the same section, I would clarify whether the 0 padding before the
 OCS should be considered part of the surplus area in the checksum computation.

 We do not think that the proposed changes are improvements and would prefer
not include them. Further, it doesn’t matter if the 0 is included in the
surplus area checksum computation or not -- it’s a 0, which has no effect on
ones-complement arithmetic. What we do avoid is the need to write code to
deal with adding in individual bytes to a 16-bit quantity.

[AFT] Noted
Section 11.2 presents the NOP option as a padding solution to align options to
16-, 32- or 64-bits boundaries, yet, the need to align UDP options or not is
not mentioned in the design principles. It should be mentioned there clearly in
my view.

Allowance for alignment is not a design principle; it is an optimization for
implementation efficiency and has been common practice not needing explanation
since at least the 1980s -- see, e.g., RFC 791 (IP) and RFC 793 (TCP).

[AFT] Noted

In section 11.4, the document mentions that "The Identification field SHOULD be
generated in a manner similar to that of the IPv6 Fragment ID [RFC8200].". I
think it would be beneficial to give a bit more details about this generation
method to add clarity. In the same section, the document mentions that "2.
Identify the desired fragment size, which we will call "S". This value is
calculated to take the path MTU into account (if known) and to allow space for
per-fragment options.". I would give a better idea about the size of said space
for per-fragment options.

We think that pointing to RFC 8200 without prescribing specific algorithms
(as is done, e.g., in RFC 1948 and RFC 6528), is more than sufficient. That
being said, if you wish to propose specific text, we are all ears.

[AFT] I noted your answer to the first comment in this paragraph. Regarding the second part of the comment, by memory, RFC 3261 mentions that a packet containing a SIP header and body should not be larger than the MTU minus 200 bytes to give enough space for on path additions to the header. I was having similar recommendations in mind regarding the desired fragment size.

In section 16, the document mentions "... many of the deployment scenarios of
interest...", yet, to the best of my understanding, those scenarios are not
mentioned in the text of the document. Could a reference or a description of
those scenarios be given?

This is covered by Section 5 and by the last paragraph of Section 16, where
[He24] is cited.

[AFT] Noted.

I was a bit surprised by the results presented in section 18 of the document
because if one looks at some OS's code, the consistency between the length
advertised in the IP header and the length advertised in the UDP header is
checked (see FreeBSD: http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c)
In particular, I find the fact that the inconsistency is only noted by one
middlebox vendor astonishing. On the one end, it is good news for UDP options,
but it opens avenues for side channel communications, which I would consider
being a threat.

We looked up the BSD code at that link and saw this:

  511<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L511>         /*

  512<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L512>          * Make mbuf data length reflect UDP length.  If not enough data to

  513<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L513>          * reflect UDP length, drop.

  514<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L514>          */

  515<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L515>         len<http://fxr.watson.org/fxr/ident?i=len> = ntohs<http://fxr.watson.org/fxr/ident?i=ntohs>((u_short)uh->uh_ulen);

  516<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L516>         ip_len = ntohs<http://fxr.watson.org/fxr/ident?i=ntohs>(ip<http://fxr.watson.org/fxr/ident?i=ip>->ip_len) - iphlen;

  517<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L517>         if (proto<http://fxr.watson.org/fxr/ident?i=proto> == IPPROTO_UDPLITE<http://fxr.watson.org/fxr/ident?i=IPPROTO_UDPLITE> && (len<http://fxr.watson.org/fxr/ident?i=len> == 0 || len<http://fxr.watson.org/fxr/ident?i=len> == ip_len)) {

  518<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L518>                 /* Zero means checksum over the complete packet. */

  519<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L519>                 if (len<http://fxr.watson.org/fxr/ident?i=len> == 0)

  520<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L520>                         len<http://fxr.watson.org/fxr/ident?i=len> = ip_len;

  521<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L521>                 cscov_partial = 0;

  522<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L522>         }

  523<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L523>         if (ip_len != len<http://fxr.watson.org/fxr/ident?i=len>) {

  524<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L524>                 if (len<http://fxr.watson.org/fxr/ident?i=len> > ip_len || len<http://fxr.watson.org/fxr/ident?i=len> < sizeof(struct udphdr<http://fxr.watson.org/fxr/ident?i=udphdr>)) {

  525<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L525>                         UDPSTAT_INC<http://fxr.watson.org/fxr/ident?i=UDPSTAT_INC>(udps_badlen);

  526<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L526>                         goto badunlocked;

  527<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L527>                 }

  528<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L528>                 if (proto<http://fxr.watson.org/fxr/ident?i=proto> == IPPROTO_UDP<http://fxr.watson.org/fxr/ident?i=IPPROTO_UDP>)

  529<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L529>                         m_adj<http://fxr.watson.org/fxr/ident?i=m_adj>(m<http://fxr.watson.org/fxr/ident?i=m>, len<http://fxr.watson.org/fxr/ident?i=len> - ip_len);

  530<http://fxr.watson.org/fxr/source/netinet/udp_usrreq.c#L530>         }

If our understanding is correct, the highlighted code simply discards the
surplus area. The only policing for consistency is to make sure that UDP Length
does not point past the end of the packet, as indicated by the IP payload
length. This is consistent with what we have seen in other legacy endpoint
implementations -- they simply silently discard the surplus, which is what
we want and expect them to do.

Legacy middleboxes need to keep the surplus intact, but they’re not likely to
look at it, since it is usually deep in the packet. They can and do filter on
transport ports or UDP content, but (with some exceptions) they don’t typically
check for consistency between UDP length and IP payload length.

[AFT] Noted

The following are minor issues (typos, misspelling, minor text improvements)
with the document:

* On page 16, "...and trying to defining meaning..." should be replaced by
"...and trying to define meaning..."

Good catch. It will be fixed in -41.

Mike and Joe