Re: [tcpm] AD Review: draft-ietf-tcpm-icmp-attacks-09

[Lars Eggert <[e-mail]>]

Hi,

On 2010-1-27, at 22:37, Fernando Gont wrote:
> Comments in-line.... (I have removed those changes that I will apply as
> suggested, and that didn't require further discussion).

I'll do the same in this reply.

>> INTRODUCTION, paragraph 13:
>>> Copyright Notice
>> 
>> You're using the IETF Trust Provisions' Section 6.b License Notice 
>> from 12 Sep 2009 rather than the newer Notice from 28 Dec 2009.  (See
>> http://trustee.ietf.org/license-info/)
> 
> There was no way (other manual) to fix this (at least when the last rev
> of the document was published). That said, the "grace period" announced
> by the IETF Trust still lasts till February 1st, so there's no problem
> with the old boilerplate text.

Yes, but it's unlikely that the final revision will be submitted before Feb 1? IETF Last Call and IESG Review are still coming.

>> Section 5.2., paragraph 13:
>>> Based on this analysis, most popular TCP implementations treat all
>> 
>> Odd wording. I doubt that the analysis in this ID has caused existing
>> TCP implementations to change years ago. Do you mean "An analysis of
>> most popular TCP implementations has shown..."?
> 
> No, I meant what I wrote: Those implementations that already included
> the hard_errors -> soft_errors processing did so for the same reason
> stated in the draft (e.g., I did talk with Kirk McKusick back in 2005
> and these guys (and others) got to the same conclusion (e.g.,
> hard-errors -> soft-errors) through the same analysis). That said, other
> implementations implemented this in response to this I-D (e.g., Cisco,
> Microsoft, and others). Please see the "ICMP attacks" advisories linked
> in: http://www.gont.com.ar/advisories/index.html (and there are others).

But this is a TCPM WG document, and it was your personal involvement that caused the changes, and not the TCPM WG document. So it wasn't "this analysis" in the sense of this document, but "an analysis done by someone who now happens to be editor of the WG document."

>> Section 7.1., paragraph 1:
>>> systems determine the Path MTU of an arbitrary internet path.
>> 
>> Nit: s/internet/Internet/ (also in many other places in the ID)
> 
> I did mean "internet". The mechanism works if you're using it in e.g. a
> corporate network that is not connected to the Internet.

So that by "internet" you mean "IP-based isolated network" was completely lost on me. Is this really an important point? Why wouldn't a technique for this that works on the Internet work for isolated deployments? And if it does, why don't you simply say "Internet"?

>> I thought most modern operating systems didn't use hard IRQs anymore
>> in their stacks? Isn't it all soft-interrupts or similar now? (I do
>> agree that packet processing overhead per payload byte goes up with
>> smaller packets, but I don't think it's because of IRQs.)
> 
> Would you be happier with "increased interrupt and context-switching rate"?

I'd just talk about increased processing overheads, but this isn't a terribly important issue.

>> Section 7.2., paragraph 1:
>>> The IETF has standardized a Path-MTU Discovery mechanism called 
>>> "Packetization Layer Path MTU Discovery" that does not depend on 
>>> ICMP error messages.  Implementation of the aforementioned 
>>> mechanism in replacement of the traditional PMTUD (specified in 
>>> [RFC1191] and [RFC1981]) would eliminate this vulnerability. 
>>> However, it might also lead to an increase of the PMTUD convergence
>>> time.
>> 
>> Add a citation to RFC4821 here. Also, why the conjunctive? PLPMTUD 
>> *does* eliminate this vulnerability.
> 
> PLPMTUD can be implemented with or without ICMP support (i.e., as a
> replacement for classical PMTUD, or for black-hole detection).
> Therefore, it does not necessarily eliminate this vulnerability.

OK, but then say this in the document. It wasn't clear to me at all that you tried to allude to that by saying "would eliminate."

>> Section 7.2., paragraph 11:
>>> On the other hand, maxsizeacked holds the size (in octets) of the 
>>> largest packet that has so far been acknowledged for this 
>>> connection. It is initialized to 68 (the minimum IPv4 MTU) when the
>>> underlying internet protocol is IPv4, and is initialized to 1280 
>>> (the minimum IPv6 MTU) when the underlying internet protocol is 
>>> IPv6.  Whenever an acknowledgement for a packet larger than 
>>> maxsizeacked octets is received, maxsizeacked is set to the size of
>>> that acknowledged packet.
>> 
>> Maybe I'm dense, but how does this work with delayed ACKs? If you see
>> the left edge of the window shift by n bytes, you don't know that 
>> maxsizeacked is n bytes, right?
> 
> Correct. The worst-case scenario is that you do the "path mtu update
> phase" rather than "initial path mtu discovery". i.e., the mechanism
> fails on the safe side.

Let's discuss this for a second to make sure I understand this correctly.

The definition for acked_packet_size is: "Variable holding the packet size (data, plus headers) the ACK that has just been received is acknowledging." But that's actually not accurate, because an incoming ACK doesn't ACK a specific transmitted segment - all you know when you receive an ACK is that the left edge of the window moved by a certain amount. Is acked_packet_size simply set to that amount?

If yes, then with delayed ACKs (or otherwise in corner cases) acked_packet_size is actually larger than maxsizesent always. On second reading, I don't think that's a problem for the technique, but the name and description of the variable confused me earlier.

>> Section 7.2., paragraph 20:
>>> MAXSEGRTO can be set, in principle, to any value greater than or 
>>> equal to 0.  Setting MAXSEGRTO to 0 would make TCP perform the 
>>> traditional PMTUD mechanism defined in [RFC1191] and [RFC1981].  A
>>> MAXSEGRTO of 1 provides enough protection for most cases.  In any
>>> case, implementations are free to choose higher values for this 
>>> constant.  MAXSEGRTO could be a function of the Next-Hop MTU 
>>> claimed in the received ICMP "Packet Too Big" message.  That is, 
>>> higher values for MAXSEGRTO could be imposed when the received ICMP
>>> "Packet Too Big" message claims a Next-Hop MTU that is smaller
>>> than some specified value.
>> 
>> Which value do the different implementations you surveyed use for 
>> MAXSEGRTO?
> 
> They use a MAXSEGRTO of 1. I wrote and commited the OpenBSD
> implementation myself. NetBSD ported the one in OpenBSD to their OS, so
> they use the same MAXSEGRTO.

I forgot to say in my original comment that that may be interesting information to put in the draft, for readers who are wondering.

>> Section 7.2., paragraph 22:
>>> Section 7.3 shows the proposed counter-measure in action. Section 
>>> 7.4 shows the proposed counter-measure in pseudo-code.
>> 
>> Since the publication target is Informational, calling this the 
>> "proposed" something is slightly misleading. Can you rephrase this to
>> be about what current implementations do?
> 
> I can, and I will. However, I think this has been overstressed for this
> I-D. It's an Informational I-D. So it should be clear from the target
> itself and the Page-1 boilerplate to be used for the RFC that it does
> not contain any "formal IETF recommendation".

Not all readers know the differences between different types of RFCs, and I'd prefer if the wording in the text was consistent with the message that the WG wants to send about these mitigation techniques.

Thanks,
Lars