Subject: Re: [Idr] I-D Action: draft-ietf-idr-error-handling-01.txt

From: "Simpson, Adam (Adam)" <[email-masked]>
Date: Thu, 1 Mar 2012 14:55:19 -0600

Authors,

Thank you for your work on this.

One topic that I would like see discussed more thoroughly in a next version of the draft is how attribute-length errors should be handled and whether it's even appropriate for these types of errors to fall under the scope of the treat-as-withdrawn or attribute-discard approaches.

In section 5 of the current draft you give examples of a malformed community attribute as one with a length that is not a multiple of 4 bytes and a malformed aggregator attribute as one with a length that is not 6 or 8 bytes. The trouble I have with treating these types of errors using treat-as-withdrawn or attribute-discard is that you are forcing the receiver of a malformed Update message to guess what the sender actually encoded. If an aggregator attribute with a reported length of 7 bytes is received did the sender encode 6 bytes and incorrectly put 7 in the length field or did they put an extra byte of garbage in the attribute?

Fundamentally, I think it would be better to restrict treat-as-withdraw and attribute-discard to cases where the length of an attribute is not in question. If you disagree then I think you must add text to the draft that describes what an implementation should assume when it sees a length error such as the example discussed above - e.g. should it assume the "closest" valid value? There is probably a small chance of making a wrong "guess" that actually results in successful parsing of the message but if that chance is at all greater than 0 it seems we are taking treat-as-wthdrawn a step too far.

Adam

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf Of
> [email-masked]
> Sent: Thursday, December 15, 2011 6:39 PM
> To: [email-masked]
> Cc: [email-masked]
> Subject: [Idr] I-D Action: draft-ietf-idr-error-handling-01.txt
> 
> 
> A New Internet-Draft is available from the on-line Internet-Drafts
> directories. This draft is a work item of the Inter-Domain Routing
> Working Group of the IETF.
> 
> 	Title           : Revised Error Handling for BGP UPDATE Messages
> 	Author(s)       : John G. Scudder
>                           Enke Chen
>                           Pradosh Mohapatra
>                           Keyur Patel
> 	Filename        : draft-ietf-idr-error-handling-01.txt
> 	Pages           : 10
> 	Date            : 2011-12-15
> 
>    According to the base BGP specification, a BGP speaker that receives
>    an UPDATE message containing a malformed attribute is required to
>    reset the session over which the offending attribute was received.
>    This behavior is undesirable as a session reset would impact not
> only
>    routes with the offending attribute, but also other valid routes
>    exchanged over the session.  This document partially revises the
>    error handling for UPDATE messages, and provides guidelines for the
>    authors of documents defining new attributes.  Finally, it revises
>    the error handling procedures for several existing attributes.
> 
> 
> A URL for this Internet-Draft is:
> http://www.ietf.org/internet-drafts/draft-ietf-idr-error-handling-
> 01.txt
> 
> Internet-Drafts are also available by anonymous FTP at:
> ftp://ftp.ietf.org/internet-drafts/
> 
> This Internet-Draft can be retrieved at:
> ftp://ftp.ietf.org/internet-drafts/draft-ietf-idr-error-handling-01.txt
> 
> _______________________________________________
> Idr mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/idr

---

From: Enke Chen <[email-masked]>
Date: Thu, 01 Mar 2012 13:56:08 -0800

Hi, Adam:

Not sure if we need extra text.  The receiver must use the attribute 
length encoded in the message to continue parsing the message.

-- Enke

---

From: Robert Raszuk <[email-masked]>
Date: Fri, 02 Mar 2012 06:33:25 +0100

Hi Adam,

> The trouble I have with treating these types of errors using
> treat-as-withdrawn or attribute-discard is that you are forcing the
> receiver of a malformed Update message to guess what the sender
> actually encoded

Invalid length of the attribute is a fatal error as Enke said you just 
can't parse the message further and you either treat-as-withdraw (if 
MP_(UN)REACH_NLRI attributes were at the beginning) and you know what 
prefixes were sent/withdrawn in this update message or you drop the 
session. That's pretty much two alternatives you have.

Can you explain where any "guessing" would be taking place ?

Thx,
R.

> One topic that I would like see discussed more thoroughly in a next
> version of the draft is how attribute-length errors should be
> handled and whether it's even appropriate for these types of errors
> to fall under the scope of the treat-as-withdrawn or
> attribute-discard approaches.
>
> In section 5 of the current draft you give examples of a malformed
> community attribute as one with a length that is not a multiple of 4
> bytes and a malformed aggregator attribute as one with a length that
> is not 6 or 8 bytes. The trouble I have with treating these types of
> errors using treat-as-withdrawn or attribute-discard is that you are
> forcing the receiver of a malformed Update message to guess what the
> sender actually encoded. If an aggregator attribute with a reported
> length of 7 bytes is received did the sender encode 6 bytes and
> incorrectly put 7 in the length field or did they put an extra byte
> of garbage in the attribute?
>
> Fundamentally, I think it would be better to restrict
> treat-as-withdraw and attribute-discard to cases where the length of
> an attribute is not in question. If you disagree then I think you
> must add text to the draft that describes what an implementation
> should assume when it sees a length error such as the example
> discussed above - e.g. should it assume the "closest" valid value?
> There is probably a small chance of making a wrong "guess" that
> actually results in successful parsing of the message but if that
> chance is at all greater than 0 it seems we are taking
> treat-as-wthdrawn a step too far.
>
> Adam
>
>> -----Original Message----- From: [email-masked]
>> [mailto:[email-masked]] On Behalf Of [email-masked]
>> Sent: Thursday, December 15, 2011 6:39 PM To: [email-masked]
>> Cc: [email-masked] Subject: [Idr] I-D Action:
>> draft-ietf-idr-error-handling-01.txt
>>
>>
>> Author(s)       : John G. Scudder Enke Chen Pradosh Mohapatra
>> Keyur Patel Filename        : draft-ietf-idr-error-handling-01.txt
>> Pages : 10 Date            : 2011-12-15
>>
>> According to the base BGP specification, a BGP speaker that
>> receives an UPDATE message containing a malformed attribute is
>> required to reset the session over which the offending attribute
>> was received. This behavior is undesirable as a session reset
>> would impact not only routes with the offending attribute, but also
>> other valid routes exchanged over the session.  This document
>> partially revises the error handling for UPDATE messages, and
>> provides guidelines for the authors of documents defining new
>> attributes. Finally, it revises the error handling procedures for
>> several existing attributes.
>>
>>
>> Idr mailing list [email-masked]
> _______________________________________________ Idr mailing list
> [email-masked] https://www.ietf.org/mailman/listinfo/idr
>
>

---

From: "Simpson, Adam (Adam)" <[email-masked]>
Date: Fri, 2 Mar 2012 09:06:31 -0600

Thanks for your reply Robert.

See inline.

>session. That's pretty much two alternatives you have.

I agree these are the two options. Based on the current text in the draft
you might also be led to believe that attribute discard is another option
-- if the attribute with the length error does not influence path
selection -- but that's clearly invalid in this type of error case.

I am OK with treat-as-withdraw as an option but I think the processing
rules need to be spelled out clearly - for example:
1. Apply treat as withdraw if the first attribute in the message is
MP_REACH_NLRI and the message length, withdrawn routes length and total
path attributes length indicate that there are no non-multiprotocol IPv4
NLRI at the end of the message.
2. Trust that the message length and total path attributes length were
correct in order to skip to the end of the Update message.

>Can you explain where any "guessing" would be taking place ?

If you look at the above rules both are based on the assumption that the
overall message length and total path attributes length were correct even
though one (or more) of the individual attribute lengths were "incorrect".
I think in practice this will rarely pan out and the session will be
dropped anyway.

---

From: "Smith, Donald" <[email-masked]>
Date: Fri, 2 Mar 2012 08:42:59 -0700

But if the length is invalid the packet is somehow malformed. How can you take any action on a malformed packet or trust any of the data in said packet?

I would vote for discard not dropping the session or taking any action on a malformed packet.

(coffee != sleep) & (!coffee == sleep)
 [email-masked]<mailto:[email-masked]>
________________________________
From: [email-masked] [[email-masked]] On Behalf Of Robert Raszuk [[email-masked]]
Sent: Thursday, March 01, 2012 10:33 PM
To: Simpson, Adam (Adam)
Cc: [email-masked]
Subject: Re: [Idr] I-D Action: draft-ietf-idr-error-handling-01.txt

Hi Adam,

Thx,
R.

________________________________
This communication is the property of CenturyLink and may contain confidential or privileged information. Unauthorized use of this communication is strictly
prohibited and may be unlawful. If you have received this communication
in error, please immediately notify the sender by reply e-mail and destroy
all copies of the communication and any attachments.

---

From: Tony Li <[email-masked]>
Date: Fri, 2 Mar 2012 08:05:29 -0800

> I would vote for discard not dropping the session or taking any action on a malformed packet.

It's a message in a byte stream.  If you no longer trust where the message ended, where is the start of the next message?

Tony

---

From: Robert Raszuk <[email-masked]>
Date: Fri, 02 Mar 2012 17:53:58 +0100

Hi Donald,

> But if the length is invalid the packet is somehow malformed. How can
> you take any action on a malformed packet or trust any of the data in
> said packet?

Just to clarify I am assuming UPDATE message length is ok, but some 
attribute(s) length within given message is bad.

> I would vote for discard not dropping the session or taking any action

If UPDATE message length itself is wrong you drop session. How and what 
would you discard ?

Thx,
R.

---

From: "Smith, Donald" <[email-masked]>
Date: Fri, 2 Mar 2012 10:43:51 -0700

That was my assumption too (packet length is correct but some attribute length is incorrect).
I would drop the packet but not the session assuming something along the way mundged the packet somewhat. Now what could you do if ALL bgp update packets from a peer were malformed? At some point you would have to treat it as a bad peer and drop the session or something right?

(coffee != sleep) & (!coffee == sleep)
 [email-masked]<mailto:[email-masked]>
________________________________
From: Robert Raszuk [[email-masked]]
Sent: Friday, March 02, 2012 9:53 AM
To: Smith, Donald
Cc: Simpson, Adam (Adam); [email-masked]
Subject: Re: [Idr] I-D Action: draft-ietf-idr-error-handling-01.txt

Hi Donald,

Thx,
R.

---

From: "Smith, Donald" <[email-masked]>
Date: Fri, 2 Mar 2012 10:49:12 -0700

I wasn't thinking about the packet length its self. In that case there is a decent chance the os stack, a middle box or something is going to drop it anyways as malformed.

(coffee != sleep) & (!coffee == sleep)
 [email-masked]<mailto:[email-masked]>
________________________________
From: Tony Li [[email-masked]]
Sent: Friday, March 02, 2012 9:05 AM
To: Smith, Donald
Cc: [email-masked]; Simpson, Adam (Adam); [email-masked]
Subject: Re: [Idr] I-D Action: draft-ietf-idr-error-handling-01.txt

Tony

---

From: Tony Li <[email-masked]>
Date: Fri, 2 Mar 2012 09:49:13 -0800

> That was my assumption too (packet length is correct but some attribute length is incorrect).

How do you know that it was the attribute length and not the message length?   In reality, all you can tell is that they are inconsistent.

Tony

---

From: Robert Raszuk <[email-masked]>
Date: Fri, 02 Mar 2012 18:56:22 +0100

Hi Donald,

> Now what could you do if ALL bgpupdate packets from a peer were
> malformed? At some point you would have to treat it as a bad peer and
> drop the session or something right?

Actually the way specs and implementations are/will be there is no 
notion of "at some point". The implicit assumption is that error happens 
sporadically .. and here I must say that I am not convinced this is the 
right assumption.

Another point is that this is to become a default when you upgrade the 
router. So we all need to carefully study release notes and as soon as 
possible after upgrade configure the knob not to follow this spec till 
our NOC scripts are able to catch those events from the syslog and 
generate alarms.

Session will stay unless one of the fatal errors happen (invalid BGP 
message length itself is one of them).

Cheers,
R.

---

From: "Smith, Donald" <[email-masked]>
Date: Fri, 2 Mar 2012 11:09:45 -0700

If the checksum is correct and the packet length matches the actual packet length I think you could assume just the attribute length was incorrect for some reason.

(coffee != sleep) & (!coffee == sleep)
 [email-masked]<mailto:[email-masked]>
________________________________
From: Tony Li [[email-masked]]
Sent: Friday, March 02, 2012 10:49 AM
To: Smith, Donald
Cc: [email-masked]; [email-masked]
Subject: Re: [Idr] I-D Action: draft-ietf-idr-error-handling-01.txt

Tony

---

From: Tony Li <[email-masked]>
Date: Fri, 2 Mar 2012 10:17:13 -0800

> If the checksum is correct and the packet length matches the actual packet length I think you could assume just the attribute length was incorrect for some reason.

I'm sorry.  We are talking about BGP here, yes???  This is IDR, yes???

The checksum is on the TCP packet, which is the transport wrapper for the byte stream.  The IP packet length only gives you the length of the TCP segment and has no bearing whatsoever on BGP.
It is completely orthogonal to the message length or attribute length.

Tony

---

From: "Simpson, Adam (Adam)" <[email-masked]>
Date: Fri, 2 Mar 2012 13:25:53 -0600

Enke,

If this is the assumption then I think it should be stated explicitly.

But this still does not address the root of my concern. Suppose, hypothetically, some implementation builds a correct 8-byte aggregator attribute but reports that it is 12-bytes long and adds 12 bytes for it when computing the total path attributes length. If the treat-as-withdraw logic is enabled the receiver could incorrectly conclude that all the NLRI were upfront and miss that there was in fact a single 4-byte NLRI field at the end. The receiver misses it because it jumped to the end of the Update on the basis of trusting length fields that were consistent, but unfortunately consistently wrong. Wouldn't it have been safer to reset the session?

-Adam

> Sent: Thursday, March 01, 2012 4:56 PM
> To: [email-masked]
> version of the draft is how attribute-length errors should be handled
> and whether it's even appropriate for these types of errors to fall
> under the scope of the treat-as-withdrawn or attribute-discard
> approaches.
> >
> bytes and a malformed aggregator attribute as one with a length that is
> not 6 or 8 bytes. The trouble I have with treating these types of
> incorrectly put 7 in the length field or did they put an extra byte of
> garbage in the attribute?
> >
> > Fundamentally, I think it would be better to restrict treat-as-
> withdraw and attribute-discard to cases where the length of an
> attribute is not in question. If you disagree then I think you must add
> text to the draft that describes what an implementation should assume
> when it sees a length error such as the example discussed above - e.g.
> should it assume the "closest" valid value? There is probably a small
> chance of making a wrong "guess" that actually results in successful
> parsing of the message but if that chance is at all greater than 0 it
> seems we are taking treat-as-wthdrawn a step too far.
> >
> > Adam
> >
> receives

---

From: Enke Chen <[email-masked]>
Date: Fri, 02 Mar 2012 11:41:52 -0800

Adam,

There is a tradeoff - impact on one route vs all routes carried over the 
session in your example.  That is why the draft states that the error 
conditions must be logged and analyzed.

-- Enke

---

From: "Simpson, Adam (Adam)" <[email-masked]>
Date: Fri, 2 Mar 2012 13:47:51 -0600

I agree, but I think there has been a general consensus in these discussions that we should keep the new/revised error handling procedures as simple as possible. Using treat as withdraw with length-related attribute errors is going to increase implementation complexity for what I suspect is very little net benefit.

-Adam

> From: Enke Chen [mailto:[email-masked]]
> Sent: Friday, March 02, 2012 2:42 PM
> To: Simpson, Adam (Adam)
> Cc: [email-masked]; Enke Chen
> > But this still does not address the root of my concern. Suppose,
> hypothetically, some implementation builds a correct 8-byte aggregator
> attribute but reports that it is 12-bytes long and adds 12 bytes for it
> when computing the total path attributes length. If the treat-as-
> withdraw logic is enabled the receiver could incorrectly conclude that
> all the NLRI were upfront and miss that there was in fact a single 4-
> byte NLRI field at the end. The receiver misses it because it jumped to
> the end of the Update on the basis of trusting length fields that were
> consistent, but unfortunately consistently wrong. Wouldn't it have been
> safer to reset the session?
> >
> > -Adam
> >
> handled
> is
> >>>>      an UPDATE message containing a malformed attribute is
> required
> >> to
> >>>>      error handling for UPDATE messages, and provides guidelines
> for
> >> the

---

From: Enke Chen <[email-masked]>
Date: Fri, 02 Mar 2012 11:57:37 -0800

Well, the case of the "attr length of 3000 bytes" in the field states 
loudly for using "treat-as-withdraw".

-- Enke

---

From: "Simpson, Adam (Adam)" <[email-masked]>
Date: Mon, 5 Mar 2012 13:03:39 -0600

I think there is a major difference between a 3000-byte optional transitive attribute that is new and unknown vs. a standardized attribute such as aggregator or extended community. A router receiving the optional transitive attribute for the first time has no idea whether 3000 bytes is a correct/valid length or not but it certainly knows that a 12-byte aggregator attribute is invalid.

> Sent: Friday, March 02, 2012 2:58 PM
> To: Simpson, Adam (Adam)
> > I agree, but I think there has been a general consensus in these
> discussions that we should keep the new/revised error handling
> procedures as simple as possible. Using treat as withdraw with length-
> related attribute errors is going to increase implementation complexity
> for what I suspect is very little net benefit.
> >
> > -Adam
> >
> >>>> attribute is not in question. If you disagree then I think you
> must
> >> add
> >>>>>> From: [email-masked] [mailto:[email-masked]] On
> Behalf
> >>>> Of
> >>>>>> [email-masked]
> >>>>>>       This behavior is undesirable as a session reset would
> impact
> >> not
> >>>>>> only
> >>>>>>       exchanged over the session.  This document partially
> revises
> >> the
> >>>>>> ftp://ftp.ietf.org/internet-drafts/draft-ietf-idr-error-
> handling-
> >>>> 01.txt

---

From: Enke Chen <[email-masked]>
Date: Mon, 05 Mar 2012 11:42:17 -0800

Adam,

Consistent behavior is simpler operationally and more desirable.  
Whether a particular attribute is recognized or not can vary from router 
to router, and from release to release.

I am not sure if you know the details of the well-publicized "3000 byte 
attribute" incident in which the attribute length was inconsistency with 
the total attribute length.   Session resetting was the behavior and 
that was causing a huge problem in the field.

-- Enke

---

From: "Simpson, Adam (Adam)" <[email-masked]>
Date: Mon, 5 Mar 2012 16:17:57 -0600

Enke,

Yes, I know the incident you are referring to. For treat as withdraw to have successfully avoided session reset with this error the receiver would have had to correctly guess whether the total path attribute length was correct or whether the individual attribute lengths were correct. Your earlier email suggested that the individual lengths are the ones to trust and use to skip to the end of the Update. I think that statement needs to appear in the draft and we need to keep in mind that this will not avoid session reset whenever an individual attribute reports a length different than the number of bytes encoded. 

-Adam