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

[Stig Venaas <[e-mail]>]

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

On Tue, Jan 19, 2021 at 11:43 AM Holland, Jake <[e-mail]> wrote:
>
> Hi Stig,
>
> On 1/19/21, 10:21 AM, "Stig Venaas" <[e-mail]> wrote:
> > 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.
>
> 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.
>
> >> 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?
> >
> > 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.
>
> 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.
> >> 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
> ...
> >> 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
> >
> > Yes (except additional not auxiliary). Perhaps it is best if I refer
> > 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.)
>
> >> 3.
> ...
> > 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.
>
> Cool, thanks.  I agree the figure seems fine to me.
>
> >> 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.)
> >
> > 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).
>
> 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
>
>