[tsvwg] Comments on UDP Options -13

[Tom Jones <[e-mail]>]

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