Re: [tsvwg] Comments on UDP Options -13

[Tom Jones <[e-mail]>]

...snipped out where we agree...

> > --
> > 
> > 	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
> 
> 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.

> 
> > -- 
> > 
> > 	>> 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 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

> > 
> > 	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.
> 
> 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)


> > --
> > 
> > 	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.
> 
> 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.
> 

"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.

> > --
> > 
> > 	   >> 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.
> 
> 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.
> 

I like this text

> > --
> > 
> > 	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 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



> > --
> > 
> > I think section 5.5 would benefit from a diagram.
> 
> 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.

> > --
> > 
> > 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.
> 
> You can always look at the smallest value you get reflected back; the difference between that and your current time is RTT.
> 
> > 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.
> 
> The peer’s TS is needed because it’s what you return to help them compute RTT.
> 
> > I guess that the values could be used to detect reordering, is this the
> > intention of passing the values to the application?
> 
> 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