Subject: Re: [pim] draft-ietf-pim-igmp-mld-extension WGLC

From: Stig Venaas <[email-masked]>
Date: Fri, 15 Jan 2021 15:35:00 -0800

Thanks a lot for all the WGLC responses, in particular to Ian, Lenny,
Jeffrey and Jake for great comments. Hope I didn't miss anyone.

I just posted revision 03 that I think should address your comments.
Great if you can have a look. I realize I forgot to update the
acknowledgements though, so I will do that shortly.

Regards,
Stig

On Thu, Dec 17, 2020 at 3:25 PM Stig Venaas <[email-masked]> wrote:
>
> Hi Jeffrey
>
> I appreciate your comments, please see responses inline.
>
> On Tue, Dec 15, 2020 at 6:21 PM Jeffrey (Zhaohui) Zhang
> <zzhang=[email-masked]> wrote:
> >
> > Support progressing this work.
> >
> >
> >
> > I have a couple of curiosity questions on the following:
> >
> >
> >
> >    Note that there may be additional data following this extension.  The
> >
> >    Total Extension Length field would indicate where this extension
> >
> >    ends, and the additional data starts.
> >
> >
> >
> > How would you have additional data following this extension? The TLV mechanism seems to be universal/generic to handle all possible extensions, so why would you have another extension blob after this, and how would you indicate so? By another bit in the reserved field?
> >
> > Just wondering if this additional data needs to be mentioned at all.
>
> I agree that there should be no need for additional data for future
> extensions as TLVs can provide any potential extension. If the WG and
> co-authors agree, I'm happy to remove the additional additional data.
> We can then also simplify the length check and say that the extension
> length specified must be equal to the remaining payload (subtracting
> from packet payload length).
>
> >
> >   Also, there is a possibility
> >
> >    that an implementation uses the Additional Data part of IGMP/MLD
> >
> >    messages, but not according to this extension scheme.
> >
> >
> >
> > Do you mean a non-standard existing implementation? Now that you mention that, are we supposed to define the interop procedures? Are they supposed to interop with the implementations following this draft at all?
>
> The IGMP/MLD RFCs specify how additional data may be present and how
> it should be ignored. I've seen cases where implementations
> intentionally (or maybe by accident) have sent packets with additional
> data. I think it is safest to use a reserved bit to clearly indicate
> that this extension is in place, rather than trying to parse whatever
> additional data may be present.
>
> If we receive data from an existing implementation, the reserved bit
> would not be set, and we would process it exactly as before.
> If an existing implementation receives the new IGMP/MLD message, it
> would both ignore the reserved bit, and the additional data according
> to the RFCs (although if implementations tried to use additional data
> in some way, then they might break, but I think we have to live with
> that). They were not supposed to use it.
>
> When using the extension, one must consider interoperability with
> implementations that do not support the extension mechanism, as well
> as implementations that do not support the specified types. Not
> supporting the mechanism, should be equivalent to not supporting any
> of the types.
>
> I can try to add a few more details in the draft.
> >
> >
> > Also, for the following:
> >
> >
> >
> >    The extension will be part of additional data as mentioned in
> >
> >    [RFC3810] Section 5.1.12 (resp.  [RFC3376] Section 4.1.10) for query
> >
> >    messages and [RFC3810] Section 5.2.12 (resp.  [RFC3376]
> >
> >    Section 4.2.11) for report messages.
> >
> >
> >
> > 5.2.12 should be 5.2.11.
>
> Good catch!
>
> Let me know if you have any thoughts on what I wrote above, or anything else.
>
> Thanks,
> Stig
>
> >
> >
> > Jeffrey
> >
> >
> >
> > From: pim <[email-masked]> On Behalf Of Michael McBride
> > Sent: Monday, December 7, 2020 7:24 PM
> > To: [email-masked]
> > Subject: [pim] draft-ietf-pim-igmp-mld-extension WGLC
> >
> >
> >
> > [External Email. Be cautious of content]
> >
> >
> >
> > Hello all,
> >
> >
> >
> > Today begins a two week wglc for https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-extension-02
> >
> >
> >
> > Please review (it’s a quick read) and let us know your opinion on it’s readiness to progress to the iesg.
> >
> >
> >
> > Thanks,
> >
> > mike
> >
> >
> > Juniper Business Use Only
> >
> > _______________________________________________
> > pim mailing list
> > [email-masked]
> > https://www.ietf.org/mailman/listinfo/pim

---

From: "Holland, Jake" <[email-masked]>
Date: Sun, 17 Jan 2021 01:49:21 +0000

Hi Stig,

Thanks for the update, on the whole I think this improved the text
with respect to my previous comments and made several things better,
thanks.

However, while looking through the diffs, to me there seem some new
points of confusion raised by the new edits, so here's a few new
comments:

1.
This new paragraph in the intro seems like it might be over-reaching:

   When this extension mechanism is used, it will make use of the entire
   Additional Data section defined in IGMPv3/MLDv2 for TLVs.  The TLV
   scheme is flexible enough to provide for any future extensions.

I think this spec doesn't say anything about possible alternative uses
of other reserved bits indicating alternate semantics for the Auxiliary
Data sections (and probably shouldn't make claims about being flexible
enough for all future extensions).

However, it does specify that when the E bit is set, no other data
besides the Extension TLVs described here can appear in the Auxiliary
Data part of the packet.  This is narrower than what's written, I think.

1.a.
Sorry, I should have noticed this in my first review.  But while revisiting
this, I noticed that the MLDv2 and IGMPv3 specs call the extra data field
"Auxiliary Data", but this spec calls it "Additional Data".

This seems like a point of possible confusion, is there a reason not to call
it "Auxiliary Data" in this doc as well?

2.
In section 3 the 2nd to last paragraph says there MUST be no data
in the message after the last TLV:

   There MUST be no data in the message after the last TLV.  The TLVs
   are processed until the end of the message is reached.  When
   processing the TLVs an implementation MUST keep track of how many
   octets are remaining in the message and stop TLV processing when
   there is no room for any further TLVs.  That is, TLV processing stops
   if there are less than 4 octets remaining in the message after a TLV
   is processed since there is not enough room for an additional minimal
   TLV.  ...

This wording I think is confusing on a few points:
- "in the message" is sort of unclear (e.g. it could refer to the IP
  payload length, at least for the last group record).  Although the doc
  does refer briefly to the relevant sections of the IGMP and MLD specs
  in the 2nd to last paragraph of the intro, it's not clear here, near
  the normative requirements about parsing, what "in the message" means.
  I think these requirements mean to say that the TLVs must fit entirely
  within the Auxiliary Data field, with a minimum of leftover space.
- the requirement "no data following the last TLV in the Auxiliary Data
  field" might not be satisfiable, because the TLV length is given in
  octets but the Auxiliary Data length is given in 32-bit words, so there
  would have to be some leftover data when the sum of TLV lengths is not
  a multiple of 4.  So I think this requirement needs to be articulated
  better.
- Nothing is stated about leftover data if any (since there might be up
  to 3 octets unless there should be an alignment requirement?).  Maybe
  it's worth stating it should be set to 0 and ignored, if there are
  any extra octets at the end?

3.
The second of these paragraphs in the "Extension Length" explanation in
section 3 seems to be trying to clarify the parsing, but to me it raises
more questions than it answers:
      Extension Length: 2 octets.  This specifies the length in octets
      of the following Extension Value field.  Note that this value may
      be zero, in which case there is no Extension Value field present.

      The next type field, if any, will come immediately after this
      length field.

I think this is trying to say that in the event of a 0-length Extension
Value, the Extension Type of the next TLV (if any) follows the Extension
Length immediately.

But it made me realize nothing seems to state explicitly that the next
extension type field (if any) immediately follows the extension value
field (as opposed to padding for alignment or something, as Figure 1,
right above this, seems to imply it might).

It's a minor point, but it might make sense to tweak this to explain the
whole picture with something like:
      Extension Length: 2 octets.  This specifies the length in octets
      of the following Extension Value field.

      The next Extension Type field, if any, will come immediately after
      the following Extension Value field (or immediately after the
      Extension Length field, for cases with a 0-length Extension Value).

(And perhaps the 2nd paragraph or something like it should move into the
explanation of the Extension Value, or into a paragraph explaining about
the parsing overview, rather than being in the Extension Length explanation).

4.
I find this design a bit troubling:
   Also if a TLV has a length exceeding the remainder of the
   message, that TLV is ignored, and further TLV processing stops.

As an implementor, if I see this scenario I would tend to assume that
I'm talking to an implementation that is using a different non-standard
meaning for the E bit and the Auxiliary Data (or perhaps an attack
packet), and I would think the better choice is to discard the entire
Auxiliary Data and treat it as though the E bit was not set, ignoring
all the TLVs.  (I assume this should be OK since the Auxiliary Data can
be ignored anyway by the devices that don't know about it.)

Anyway, I hope that's helpful, and thanks for the updates addressing my
prior comments.

Best regards,
Jake

On 1/15/21, 3:35 PM, "Stig Venaas" <[email-masked]> wrote:

Regards,
Stig

On Thu, Dec 17, 2020 at 3:25 PM Stig Venaas <[email-masked]> wrote:
> > Today begins a two week wglc for https://urldefense.com/v3/__https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-extension-02__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFvcqNJMw$ 
> >
> >
> >
> > https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/pim__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFK1H-G2g$ 

_______________________________________________
pim mailing list
[email-masked]
https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/pim__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFK1H-G2g$

---

From: Stig Venaas <[email-masked]>
Date: Tue, 19 Jan 2021 10:20:09 -0800

Hi Jake

Thanks for reviewing this, please see my comments inline.

> Data part of the packet.  This is narrower than what's written, I think.

I do believe TLVs really is flexible enough for any future extension
and I mentioned this since people might question why we don't need
Additional Data when the extension is used. But I can certainly remove
this. The main point is that there is no data besides the Extension
TLVs. I'll try to improve this text. Of course we are also able to
extend IGMP/MLD in other ways when not setting the E-bit if we want.

> it "Auxiliary Data" in this doc as well?

The specs define both Additional Data and Auxiliary Data. The former
is at the end of the IGMP/MLD messages, which we are using. The latter
is potentially auxiliary data at the end of a group record. We are not
using that.
>   within the Auxiliary Data field, with a minimum of leftover space.

Yes (except additional not auxiliary). Perhaps it is best if I refer
to bytes remaining in the IP payload.

>   a multiple of 4.  So I think this requirement needs to be articulated

The additional data does not have to be a multiple of 4.

>       Extension Length field, for cases with a 0-length Extension Value).

This sounds good. Thanks, I'll use this. It is pretty common to draw
the figure like this even when no alignment is expected, but I think
I'll point out that there is no alignment or padding.

> the parsing overview, rather than being in the Extension Length explanation).

Agree, that is better.

> be ignored anyway by the devices that don't know about it.)

When the E-bit is set, one should be following this document, but of
course there may be bugs or attacks. I'm fine with being strict as you
suggest, the only downside is that one will have to validate this
before applying any of the TLVs. Since the length does not have to be
a multiple of 4, I think it might be best then to require that the
last TLV ends at the end of the IP payload (no trailing bytes).

Thanks,
Stig

---

From: "Holland, Jake" <[email-masked]>
Date: Tue, 19 Jan 2021 19:43:46 +0000

Hi Stig,

> extend IGMP/MLD in other ways when not setting the E-bit if we want.

I agree it's likely to be flexible enough, but I have minor reservations
about stating it as a known fact (as in -03), because it's hard to be
certain that all future needs will be fully compatible with the semantics
defined for these TLVs. (And I agree if it happens then it can be covered
by using something else other than the E-bit).

I'm not sure it needs anything pointing out the flexibility to future
needs, but if you think it will help forestall questions to include it,
even just "is flexible enough" -> "is expected to be flexible enough"
would address my reservation.

> is potentially auxiliary data at the end of a group record. We are not

Ah, thanks, my mistake.  I think the reason I missed it in my first review
was because I understood it correctly that time and then forgot.  Sorry
about that.

>>    There MUST be no data in the message after the last TLV.  The TLVs
...
> to bytes remaining in the IP payload.

Yes, to me something like one of these would be an improvement over "in
the message":
- in the IP payload
- in the Additional Data field
- in the Additional Data field, which extends to the end of the IP payload

(please disregard the rest of my comment #2, it was based on my mistaken
reading thinking you were using Auxiliary data.)

...
> I'll point out that there is no alignment or padding.

Cool, thanks.  I agree the figure seems fine to me.

> last TLV ends at the end of the IP payload (no trailing bytes).

Yes, I think that's right.

I hope that's not too bad a downside, I think it sounds ok to me.  And on
the upside it's maybe slightly more likely to get length-checked properly.

It's a minor point though, if you prefer to leave it alone I won't object.
I maybe sometimes worry more than is necessary about the sad state of
buffer-overflow avoidance strategies in networking.

Thanks and regards,
Jake

---

From: Stig Venaas <[email-masked]>
Date: Thu, 21 Jan 2021 11:54:22 -0800

Hi Jake

I just posted revision 04 that I think should address your comments.

Agree buffer-overflow is a big concern in general, I'm happy to call
it out in the document.

Thanks a lot for your time,
Stig

---

From: "Holland, Jake" <[email-masked]>
Date: Thu, 21 Jan 2021 20:02:25 +0000

Thanks Stig, much appreciated.  -04 Looks good to me.

Best regards,
Jake

On 1/21/21, 11:54 AM, "Stig Venaas" <[email-masked]> wrote:

Hi Jake

---

From: Michael McBride <[email-masked]>
Date: Sat, 23 Jan 2021 00:08:24 +0000

And with that, it appears -04 has addressed all comments and we have consensus that this draft is ready to progress to the iesg. Please speak up if you disagree. I'll plan to send this along the forwarding path next week. Have a good weekend.

Thanks,
mike

-----Original Message-----
From: pim <[email-masked]> On Behalf Of Holland, Jake
Sent: Thursday, January 21, 2021 12:02 PM
To: Stig Venaas <[email-masked]>
Cc: [email-masked]; Leonard Giuliano <[email-masked]>; [email-masked]; Jeffrey (Zhaohui) Zhang <zzhang=[email-masked]>
Subject: Re: [pim] draft-ietf-pim-igmp-mld-extension WGLC

Best regards,
Jake

Hi Jake

Agree buffer-overflow is a big concern in general, I'm happy to call it out in the document.

On Tue, Jan 19, 2021 at 11:43 AM Holland, Jake <[email-masked]> wrote:
> > Additional Data when the extension is used. But I can certainly 
> > remove this. The main point is that there is no data besides the 
> > Extension TLVs. I'll try to improve this text. Of course we are also 
> > able to extend IGMP/MLD in other ways when not setting the E-bit if we want.
>
> I agree it's likely to be flexible enough, but I have minor 
> reservations about stating it as a known fact (as in -03), because 
> it's hard to be certain that all future needs will be fully compatible 
> with the semantics defined for these TLVs. (And I agree if it happens 
> then it can be covered by using something else other than the E-bit).
>
> needs, but if you think it will help forestall questions to include 
> it, even just "is flexible enough" -> "is expected to be flexible enough"
> >> Sorry, I should have noticed this in my first review.  But while 
> >> revisiting this, I noticed that the MLDv2 and IGMPv3 specs call the 
> >> extra data field "Auxiliary Data", but this spec calls it "Additional Data".
> >>
> >> This seems like a point of possible confusion, is there a reason 
> >> not to call it "Auxiliary Data" in this doc as well?
> >
> > is at the end of the IGMP/MLD messages, which we are using. The 
> > latter is potentially auxiliary data at the end of a group record. 
> > We are not using that.
>
> Ah, thanks, my mistake.  I think the reason I missed it in my first 
> review was because I understood it correctly that time and then 
> forgot.  Sorry about that.
>
> >> 2.
> Yes, to me something like one of these would be an improvement over 
> "in the message":
> - in the IP payload
> (please disregard the rest of my comment #2, it was based on my 
> mistaken reading thinking you were using Auxiliary data.)
>
> >> 3.
> ...
> >> As an implementor, if I see this scenario I would tend to assume 
> >> that I'm talking to an implementation that is using a different 
> >> non-standard meaning for the E bit and the Auxiliary Data (or 
> >> perhaps an attack packet), and I would think the better choice is 
> >> to discard the entire Auxiliary Data and treat it as though the E 
> >> bit was not set, ignoring all the TLVs.  (I assume this should be 
> >> OK since the Auxiliary Data can be ignored anyway by the devices 
> >> that don't know about it.)
> >
> > course there may be bugs or attacks. I'm fine with being strict as 
> > you suggest, the only downside is that one will have to validate 
> > this before applying any of the TLVs. Since the length does not have 
> > to be a multiple of 4, I think it might be best then to require that 
> > the last TLV ends at the end of the IP payload (no trailing bytes).
>
> I hope that's not too bad a downside, I think it sounds ok to me.  And 
> on the upside it's maybe slightly more likely to get length-checked properly.
>

_______________________________________________
pim mailing list
[email-masked]
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fmailman%2Flistinfo%2Fpim&amp;data=04%7C01%7Cmichael.mcbride%40futurewei.com%7Ce0771d5eb7b040e1d7f208d8be478d2c%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637468562337051310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=yM%2F3POQByuvGS8hvQn1Sc3RKIHxftQmirf2kmU%2FF3bg%3D&amp;reserved=0