Re: [Idr] [BULK] Bug in draft-ietf-idr-rfc5575bis, worth fixing?

[Robert Raszuk <[e-mail]>]

All,

To me the real question is if we really should validate the content of NLRI
or just make sure that NLRI boundaries meet BGP MP_REACH definition and
treat NLRI values completely opaque to BGP (as per original RFC5575).

If NLRI is really malformed resulting in MP_REACH attribute being malformed
I do not see that much harm in session reset.

But if we dive into each atomic type parsing of the NLRI value itself at
each BGP speaker talking SAFI 133/134 I think this is going to be a mess.

Best,
R.



On Mon, Sep 28, 2020 at 9:41 PM Christoph Loibl <[e-mail]> wrote:

> Hi,
>
> I do not really want to repeat the whole discussion here (seems that we
> are going in circles) if not needed. I think that we agreed that if the
> NLRI is malformed the only option is to reset (+send notification) the
> session (even if we consider rfc7606). And from draft-rfc5575bis it is
> clear that we are talking about a malformed NLRI:
>
>    A NLRI value not encoded as specified specified here is considered
>    malformed and error handling according to Section 10 is performed.
>
>
> -> I think that adding the small term that John suggested is sufficient.
>
> Cheers Christoph
>
> --
> Christoph Loibl
> [e-mail] | CL8-RIPE | PGP-Key-ID: 0x4B2C0055 | http://www.nextlayer.at
>
>
>
> On 28.09.2020, at 21:16, Alvaro Retana <[e-mail]> wrote:
>
> [Explicitly adding Jeff to the To list.]
>
> Hi!
>
> During my AD review there was a discussion on the list about this point,
> and whether we could avoid resetting the session.  Jeff presented some
> examples and, I think, made a very convincing case of why we really can’t:
> even if we can look at the length, we would still be guessing.
>
> I think this is one of the messages:
> https://mailarchive.ietf.org/arch/msg/idr/FHC-Vz26LZfam-o5Y-9-WSrEm2g/
>
> Jeff: if you get a chance, please chime in.
>
> Thanks!
>
> Alvaro.
>
> On September 28, 2020 at 2:16:38 PM, John Scudder (
> jgs=[e-mail]) wrote:
>
>
> I think that is the right thing, too: FS uses T,V and not T,L,V for its
> component types, the lengths are implicit. So, if my parser encounters an
> unknown type code it is impossible for me to know how to skip over that
> type code. At that point, parsing breaks down.
> [Bruno] I had in mind the higher level of hierarchy:
>
>    The NLRI field of the MP_REACH_NLRI and MP_UNREACH_NLRI is encoded as
>    one or more 2-tuples of the form <length, NLRI value>.  It consists
>    of a 1- or 2-octet length field followed by a variable-length NLRI
>    value.  The length is expressed in octets.
>
>                      +-------------------------------+
>                      |    length (0xnn or 0xfnnn)    |
>                      +-------------------------------+
>                      |    NLRI value   (variable)    |
>                      +-------------------------------+
>
>                 Figure 1: Flow Specification NLRI for IPv4
>
> At this level, assuming that the NLRI value is found semantically
> incorrect, it seems to me that one could:
> -          Skip this NLRI (thanks to the ‘length’ field) and continue
> with the next NLRI
> -          Read the ‘NLRI value’ as an opaque data, and treat this NLRI
> as withdraw. (In the context of the discussion, this NLRI would never had
> been accepted, so ‘treat-as-withdraw’  could even be replaced with
> ‘ignore’. But I’m _*not*_ calling for this).
> Hence it seems to me that treat-as-withdraw is technically possible.
>
>
> Fair enough. It’s a little unfortunate that the draft contains this
> ambiguity; in retrospect it would have been better to be explicit about the
> error-handling strategy chosen rather than simply referencing RFC 7606.
> Whether or not we want to respin the draft in order to clarify it, is a
> question for the WG. If we were to make a change, it could potentially be
> the addition of this sentence, in Section 10:
>
>    Error handling according to [RFC7606 <https://tools.ietf.org/html/rfc7606>] and [RFC4760 <https://tools.ietf.org/html/rfc4760>] applies to this
>
>    specification. *Notably, an NLRI that is found to be semantically*
>
>    *incorrect (for example due to an unknown component type) MUST be*
>
>    *handled using the “treat-as-withdraw” strategy (which in this case,*
>
>    *it** must be noted, works out to be the same as skipping over the NLRI)**.*
>
>
>