Subject: [tsvwg] Comments on UDP Options -13

From: Tom Jones <[email-masked]>
Date: Thu, 2 Sep 2021 14:22:37 +0100

Hi Joe,

I have read draft -13 in advance of the iterim tomorrow. This is after a long
gap of not reading updated drafts or closely following the mailing list
discussions. I have read and offer comments on the document up to section 6
here.

My comments and suggestions follow.

I don't want to re litigate old arguments if it can be helped, if
something here has been discussed please share a link to the relevant
mailing list thread and I will read and ask again if needed.

--

The first mention of ENCR is here:

	>> Only the OCS, AUTH, and ENCR options depend on the contents of
	   the option area.

It isn't described until section 5.8. I suggest you clarify that it is a UNSAFE
sub option, maybe something like:

	>> Only the OCS, AUTH and the ENCR (UNSAFE suboption) depend on the contents of
	   the options area.

I think the document should be cleaned up so that references to ENCR make it
clear that it is hidden away inside an UNSAFE option.

--

	This issue is discussed further in Section 17.
	...
	(with the exception of NOPs, which should never need to come in sequences of more than seven in a row)

The text from section 17 doesn't really add a lot, could a sentence be added
here too, I think it will help readers a lot.

This padding limit makes UDP Options DPLPMTUD a violation of UDP Options. This
seems a weird way to start out.

https://datatracker.ietf.org/doc/html/draft-fairhurst-tsvwg-udp-options-dplpmtud#section-3.2.1

-- 

	>> The OCS MUST be included when the UDP checksum is nonzero and UDP
	   options are present.

What happens when the checksum is non zero and the OCS is missing? The document
should be explicit here, maybe:

	>> The OCS MUST be included when the UDP checksum is nonzero and UDP
	   options are present. If the OCS option is not present and the UDP
	   checksum is nonzero then the OCS calculation implicitly fails and
	   all options MUST be ignored and the surplus area silently discarded.

--

	This Internet checksum is computed over the entire surplus area
	prefixed by the surplus length pseudoheader (Figure 8) and then
	adjusting the result before storing it into the OCS checksum field.

I don't understand what is meant by 'adjusting the result', I think this should
be 'aligning the result', or on reading again, 'coordinated'? I think more
clarity is required here.

--

	The OCS covers the UDP option area as formatted for transmission and
	immediately upon reception.

I don't understand what the sentence is for. I understand that the inserting
the calculated OCS should be the final modification to the packet before
transmission, I don't see what 'immediately upon reception' is trying to say.

--

	>> If the OCS fails, all options MUST be ignored and the surplus
	area silently discarded.

and 

	As a reminder, use of the UDP checksum is optional when the UDP
	checksum is zero. When not used, the OCS is assumed to be "correct"
	for the purpose of accepting UDP packets at a receiver (see Section
	7).

give the possible results of the OCS as: 'fails' and 'correct'.  I think the OCS
is 'correct' or 'incorrect' and the OCS calculation 'passes' or 'fails'

Should the OCS calculation pass in the second case? Maybe this should be:

	the OCS is assumed to be "correct" and the OCS calculation passes

--

	UDP fragmentation relies on a fragment expiration timer, which can
	be preset or could use a value computed using the UDP Timestamp
	option.

I am not happy suggesting the use of the UDP Timestamp to control fragment
lifetime. I think this invites security based implementation issues.

--

	   >> UDP fragments MUST NOT overlap.

I think you need to be explicit.

	>> UDP fragments MUST NOT overlap. On receipt of an overlapping
	   fragment, the fragment and all stored fragments matching the
	   Identification MUST be dropped.

--
	
	>> The default UDP reassembly SHOULD be no more than 2 minutes.

This should be 'The default UDP timer'

What happens after 2 minutes? As above, all fragments stored for the expired
fragment chain should be discarded.

--

	Implementers are advised to limit the space available for UDP
	reassembly.
...
	>> UDP reassembly space SHOULD be limited to reduce the impact of
	DOS attacks on resource use.

The first of these feels redundant. 

I think you should note for implementers. The use of many small fragments,
specifically ordered has been used in fragmentation and tcp segmentation
attacks in the past to create DOS by increasing processing cost of fragments.

Care should be taken when designing reassembly algorithms and selecting data
structures. see:
	https://www.swascan.com/segmentsmack/

--

I think section 5.5 would benefit from a diagram.

--

Section 5.6 doesn't adhere to the convention described in section 2 (the use of
'>>' for sections with RFC2119 key words).

--

I will admit that I wasn't following any discussion around UNSAFE and its
introduction. I cannot say that I like this design, it feels excessively
complicated in an already complicated specification.

I struggle to see why this is being included in this form when there isn't yet
any case driving its use.

--

Could you suggest some application uses of the TSval and TSecr options? 

It is clear to me how my timestamp (and its reflection) are useful to me for
calculating RTT, but that doesn't require the TSval be forward to the
application.

It is clear not how my peers timestamp is useful at all. I would prefer that
the peers timestamp be treated as a monotonically increasing token.

I guess that the values could be used to detect reordering, is this the
intention of passing the values to the application?

--

	One such use is described as part of the UDP variant of packetization layer
	path MTU discovery

I think this should say 'UDP Options variant', to me, a UDP variant would be an
application layer protocol as described in section 6.1 of RFC8899.

https://www.rfc-editor.org/rfc/rfc8899.html#name-application-support-for-dpl

- Tom

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 2 Sep 2021 09:36:23 -0700

Hi, Tom,

I’ll try to clarify below. Not sure I can dig up all the related threads, but also not sure they’re needed…

—
Joe Touch, temporal epistemologist
www.strayalpha.com

> On Sep 2, 2021, at 6:22 AM, Tom Jones <[email-masked]> wrote:
> 
> Hi Joe,
> 

Agreed.

Gorry can help us understand how that padding works. It could be just that the options end (at EOL), but the remaining zeros (which aren’t NOPs because they’re after EOL) are the padding intended.

This is the thread that Gorry and Mike just commented on today. I think we have a resolution that would update this text.

Agreed; it’s alignment.

OCS check should be the first step of interpreting the option area at the receiver.

Agreed this is vague and will clarify. 

Can you clarify? UDP isn’t a “secure” protocol per se. It seems reasonable for UDP reassembly to take into account UDP measurements of the RTT, the same way that TCP uses its timestamps for its time-dependent features.

I favor copying the text from 8200 on this point:
      o  If any of the fragments being reassembled overlap with any
         other fragments being reassembled for the same packet,
         reassembly of that packet must be abandoned and all the
         fragments that have been received for that packet must be
         discarded, and no ICMP error messages should be sent.

         It should be noted that fragments may be duplicated in the
         network.  Instead of treating these exact duplicate fragments
         as overlapping fragments, an implementation may choose to
         detect this case and drop exact duplicate fragments while
         keeping the other fragments belonging to the same packet.

Agreed (same as for IP frags, e.g., as per RFC8200 as well).

I would prefer a stable reference if one were available.

Can you let me know what you’re thinking would help? I.e., do you want to see before/after frag?

I plan on catching this more consistently in the final doc passes.

UDP options are generally in the same spirit as TCP options - ignore the ones you don’t support. There are some we don’t want to have that behavior, i.e., we want them to abort option processing rather than be silently ignored, esp. because we can’t negotiate their use with hard state the way TCP does.

Options that modify the UDP user data fall into this category.

You can always look at the smallest value you get reflected back; the difference between that and your current time is RTT.

> the peers timestamp be treated as a monotonically increasing token.

The peer’s TS is needed because it’s what you return to help them compute RTT.

> intention of passing the values to the application?

Also to let them do things like compute RTT max, RTT std dev, etc.

Agreed.

> https://www.rfc-editor.org/rfc/rfc8899.html#name-application-support-for-dpl

Sure.

---

From: Tom Jones <[email-masked]>
Date: Fri, 3 Sep 2021 10:55:44 +0100

...snipped out where we agree...

> Gorry can help us understand how that padding works. It could be just that the options end (at EOL), but the remaining zeros (which aren’t NOPs because they’re after EOL) are the padding intended.

Chalk this one up to me failing to page information back in. I think the
current implementation we have of dplpmtud with udp options works this way.

> This is the thread that Gorry and Mike just commented on today. I think we have a resolution that would update this text.

okay, I'll reply there if I think I need to

> OCS check should be the first step of interpreting the option area at the receiver.

I think there is a language issue between the OCS field (the bytes in
the packet) and the OCS check (the checksum calculation)

I see this too in section 7:

	 if the UDP checksum fails then
               silently drop (per RFC1122)
           if the UDP checksum passes then
               if OCS is present and fails then
                   deliver the UDP payload but ignore all other options
                   (this is required to emulate legacy behavior)
               if OCS is present and passes then
                   deliver the UDP payload after parsing
                   and processing the rest of the options,
                   regardless of whether each is supported or succeeds
                   (again, this is required to emulate legacy behavior)

I think these extra words should be added throughout the document in the
correct places (field and check), like so:

	 if the UDP checksum fails then
               silently drop (per RFC1122)
           if the UDP checksum passes then
               if OCS field is present and the OCS check fails then
                   deliver the UDP payload but ignore all other options
                   (this is required to emulate legacy behavior)
               if OCS field is present and the OCS check passes then
                   deliver the UDP payload after parsing
                   and processing the rest of the options,
                   regardless of whether each is supported or succeeds
                   (again, this is required to emulate legacy behavior)

> Can you clarify? UDP isn’t a “secure” protocol per se. It seems reasonable for UDP reassembly to take into account UDP measurements of the RTT, the same way that TCP uses its timestamps for its tim…

"Using the timestamp option" implies to me that the fragment lifetime
timer can be set by peer controlled data. I can immediately see an
implementer using a peer provided field for the timer and creating a DOS
situation an attacker can use to exhaust fragment space.

If timestamps are to be used for the fragment timer then a warning for
implementers should be included. I think we should highlight the sharp
edges.

>          keeping the other fragments belonging to the same packet.

I like this text

> I would prefer a stable reference if one were available.

I was sure there was a paper, but all I can turn up are CVEs

https://www.kb.cert.org/vuls/id/962459

> Can you let me know what you’re thinking would help? I.e., do you want to see before/after frag?

I think before and after fragmentation, I am struggling to understand how
options fit in around the fragments from just reading the text.

> Also to let them do things like compute RTT max, RTT std dev, etc.

Okay, I understand. Our implementation only uses options for connected
UDP sockets and so state is stored in the kernel. If you allow
unconnected UDP then his state has to exist in the application.

- Tom