Re: [Idr] I-D Action: draft-wang-idr-bgp-error-enhance-00.txt

[Enke Chen <[e-mail]>]

Hi, Folks:

I have several comments on the draft, and some of them are similar to
Robert's.  More specifically,

---
On 3.1: Prefix Length Error:

>>
   According to [RFC7606], when a NLRI/UNLRI or MP_REACH_NLRI/
   MP_REACH_UNLRI with invalid length, eg, IPv4 Prefix length is more
   than 32, we must drop this Prefix and ignore the following Prefixes.
   We may keep the prefixes we have parsed correctly before.
>>

This is not correct.  Regarding NLRI parsing, RFC 7606 does not relax the
error handling specified in RFC 4271:

   The NLRI field in the UPDATE message is checked for syntactic
   validity.  If the field is syntactically incorrect, then the Error
   Subcode MUST be set to Invalid Network Field.

In RFC 7606, we have clarified the "Syntactic Correctness of NLRI Fields"
in Sect. 5.3, and also emphasized the fundamental requirement or using
"treat-as-withdraw":

    o j. Finally, we observe that in order to use the approach of "treat-
       as-withdraw", the entire NLRI field and/or the MP_REACH_NLRI and
       MP_UNREACH_NLRI attributes need to be successfully parsed -- what
       this entails is discussed in more detail in Section 5.  If this
       is not possible, the procedures of [RFC4271] and/or [RFC4760]
       continue to apply, meaning that the "session reset" approach (or
       the "AFI/SAFI disable" approach) MUST be followed.

---
On 3.2 Appears More Than Once

    Again we want to make sure NLRIs are parsed correctly and fully.  It's
    difficult to know for sure when there are multiple MP_REACH_NLRI
    attributes or the MP_UNREACH_NLRI in one UPDATE message.

---
On 4.1 Nexthop

    As currently defined in RFC 4271, the procedure does not allow for graceful
    recovery from the error conditions, e.g,, after the duplicate
address is removed
    from an interface. There seems to be room for improvement, perhaps in a
    separate document.

---
On 4.2 MP_REACH_NLRI:

    Again, we must make sure the NLRIs are parsed correctly and fully. That
    is fundamental to the error handling specified in RFC 7606.

---
On 4.3 Prefix SID

    *If* there is an issue with the error handling in RFC 8669, then rfc8669bis
     would be more appropriate for making the correction, IMO.

---
On 4.4 Aggregator, AS4_Aggregator:

      Why does it matter?

---
On 4.5 ORIGINATOR_ID

     "Treat-as-withdraw" would be more disruptive, isn't it? That would contrary
      to the goal of the draft. I don't see a need for the change.

---
On 4.6 Cluster-list

     Did we ever define zero as an invalid CLUSER_ID? I do not recall
we did that,
     and I don't see a need for doing so now.
-----

Thanks.  -- Enke

On Sun, Oct 24, 2021 at 6:47 AM Robert Raszuk <[e-mail]> wrote:
>
> Dear Authors,
>
> Below are my comments after review of your proposal.
>
> 1) There is no such thing as "MP_REACH_UNLRI" ... Please swap all occurances to "MP_UNREACH_NLRI". Likewise please adjust all "UNLRI" or define a priori what you mean by this abbreviation.
>
> 2)  Section 3.1 -
>
> You say:
>
> "Then we may also try to continue parse the next update packet if we can correctly find it."
>
> I don't think this is a good suggestion for the Standards track document.
>
> 3)  Section 3.2 -
>
> I disagree with the suggestion. If a speaker is sending more than one MP_REACH_NLRI or MP_UNREACH_NLRI it should be cut out as soon as possible from causing more damage.
>
> 4) Section 4.1 -
>
> I am not sure why there is any need to enumerate the same specific special IP addresses and not to list others. For example if I receive IPv4 next hop as 127.0.0.1 is this cool ?
>
> I recommend editing this:  "f) The IP address is not a invalid ip address." I think you mean to say:  "f) The IP address is not a valid ip address."
>
> 5) Section 4.2 -
>
> I don't think this is a good suggestion.  Likewise in the case  of same prefix being present in both MP_REACH_NLRI and MP_UNREACH_NRLI suggestion to "firstly process the Prefixes in the MP_REACH_NLRI then process the Prefixes in the MP_REACH_UNLRI" is not good.
>
> 6) Section 4.3 -
>
> How can you treat it as withdraw something which is part of the "malformed prefix-SID attribute" ?
>
> 7) Sections 4.4 & 4.5 & 4.6 -
>
> Why are you suggesting again to check and detect special cases of *only* all zero addresses ? There are lot's of special IPv4 or even more IPv6 addresses to detect. I am not sure if we need to educate implementers about those in the BGP spec.
>
> Conclusion:
>
> If WG agrees that there are some valid suggestions in your proposal we should issue a RFC7606bis and not separate draft updating it like you are suggesting. So far to me like there is no substance to even go for -bis version of 7606.
>
> Yes, the BGP-4 protocol running on TCP is not bullet proof when it comes to handling bad implementations or malicious protocol attacks. Your figure 1 illustrates this well. But even with all suggestions listed there still remains lot's of attack vectors if someone has access to inject bad BGP packets to your network.
>
> I think we should consider using new transport which no longer runs on TCP and essentially not only treats all SAFIs as fully independent streams but also cut's interdependencies between all NLRI even with given SAFI.
>
> Good news is that some proposals in this direction are starting to pop-up in this WG, IMO already opening new doors for the new (hopefully much more robust) BGP version.
>
> Many thx,
> Robert
>
>
>
> On Sun, Oct 24, 2021 at 2:06 PM <[e-mail]> wrote:
>>
>>
>> A New Internet-Draft is available from the on-line Internet-Drafts directories.
>>
>>
>>         Title           : Revised Error Handling for BGP Messages
>>         Authors         : Haibo Wang
>>                           Ming Shen
>>                           Jie Dong
>>         Filename        : draft-wang-idr-bgp-error-enhance-00.txt
>>         Pages           : 8
>>         Date            : 2021-10-24
>>
>> Abstract:
>>    This document supplements and revises RFC7606.  According to RFC
>>    7606, when an UPDATE packet received from a neighbor contains an
>>    attribute of incorrect format, the BGP session cannot be reset
>>    directly.  Instead, the BGP session must be reset based on the
>>    specific problem.  Error packets must minimize the impact on routes
>>    and do not affect the correctness of the protocol.  Different error
>>    handling methods are used.  The error handling methods include
>>    discarding attributes, withdrawing routes, disabling the address
>>    family, and resetting sessions.
>>
>>    RFC 7606 specifies the error handling methods of some existing
>>    attributes and provides guidance for error handling of new
>>    attributes.
>>
>>    This document supplements the error handling methods for common
>>    attributes that are not specified in RFC7606, and provides
>>    suggestions for revising the error handling methods for some
>>    attributes.  The general principle remains unchanged: Maintain
>>    established BGP sessions and keep valid routes updated.  However,
>>    discard or delete incorrect attributes or packets to minimize the
>>    impact on the current session.
>>
>>
>>
>> The IETF datatracker status page for this draft is:
>> https://datatracker.ietf.org/doc/draft-wang-idr-bgp-error-enhance/
>>
>> There is also an htmlized version available at:
>> https://datatracker.ietf.org/doc/html/draft-wang-idr-bgp-error-enhance-00
>>
>>
>> Internet-Drafts are also available by anonymous FTP at:
>> ftp://ftp.ietf.org/internet-drafts/
>>
>>
>> _______________________________________________
>> I-D-Announce mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/i-d-announce
>> Internet-Draft directories: http://www.ietf.org/shadow.html
>> or ftp://ftp.ietf.org/ietf/1shadow-sites.txt
>
> _______________________________________________
> Idr mailing list
> [e-mail]
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_mailman_listinfo_idr&d=DwICAg&c=V9IgWpI5PvzTw83UyHGVSoW3Uc1MFWe5J8PTfkrzVSo&r=OPLTTSu-451-QhDoSINhI2xYdwiMmfF5A2l8luvN11E&m=SI6tanfZVs9gnmPKxxAheSBJdycZtcVWA5R6ub0TPz0&s=Kx7piVPCBN7qQf0IR_Ue8GQC2a_BNjdKR_7ybyNWWBc&e=