Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: A couple more bgpd crash fixes for malformed packets #14716

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
bgpd: Ignore handling NLRIs if we received MP_UNREACH_NLRI
If we receive MP_UNREACH_NLRI, we should stop handling remaining NLRIs if
no mandatory path attributes received.

In other words, if MP_UNREACH_NLRI received, the remaining NLRIs should be handled
as a new data, but without mandatory attributes, it's a malformed packet.

In normal case, this MUST not happen at all, but to avoid crashing bgpd, we MUST
handle that.

Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
  • Loading branch information
ton31337 committed Oct 31, 2023
commit c37119df45bbf4ef713bc10475af2ee06e12f3bf
19 changes: 10 additions & 9 deletions bgpd/bgp_attr.c
Original file line number Diff line number Diff line change
Expand Up @@ -3399,15 +3399,6 @@ static int bgp_attr_check(struct peer *peer, struct attr *attr,
!length)
return BGP_ATTR_PARSE_WITHDRAW;

/* "An UPDATE message that contains the MP_UNREACH_NLRI is not required
to carry any other path attributes.", though if MP_REACH_NLRI or NLRI
are present, it should. Check for any other attribute being present
instead.
*/
if ((!CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_MP_REACH_NLRI)) &&
CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_MP_UNREACH_NLRI))))
return BGP_ATTR_PARSE_PROCEED;

if (!CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_ORIGIN)))
type = BGP_ATTR_ORIGIN;

Expand All @@ -3426,6 +3417,16 @@ static int bgp_attr_check(struct peer *peer, struct attr *attr,
&& !CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_LOCAL_PREF)))
type = BGP_ATTR_LOCAL_PREF;

/* An UPDATE message that contains the MP_UNREACH_NLRI is not required
* to carry any other path attributes. Though if MP_REACH_NLRI or NLRI
* are present, it should. Check for any other attribute being present
* instead.
*/
if (!CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_MP_REACH_NLRI)) &&
CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_MP_UNREACH_NLRI)))
return type ? BGP_ATTR_PARSE_MISSING_MANDATORY
: BGP_ATTR_PARSE_PROCEED;

/* If any of the well-known mandatory attributes are not present
* in an UPDATE message, then "treat-as-withdraw" MUST be used.
*/
Expand Down
1 change: 1 addition & 0 deletions bgpd/bgp_attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ enum bgp_attr_parse_ret {
/* only used internally, send notify + convert to BGP_ATTR_PARSE_ERROR
*/
BGP_ATTR_PARSE_ERROR_NOTIFYPLS = -3,
BGP_ATTR_PARSE_MISSING_MANDATORY = -4,
};

struct bpacket_attr_vec_arr;
Expand Down
7 changes: 6 additions & 1 deletion bgpd/bgp_packet.c
Original file line number Diff line number Diff line change
Expand Up @@ -2359,7 +2359,12 @@ static int bgp_update_receive(struct peer_connection *connection,
/* Network Layer Reachability Information. */
update_len = end - stream_pnt(s);

if (update_len && attribute_len) {
/* If we received MP_UNREACH_NLRI attribute, but also NLRIs, then
* NLRIs should be handled as a new data. Though, if we received
* NLRIs without mandatory attributes, they should be ignored.
*/
if (update_len && attribute_len &&
attr_parse_ret != BGP_ATTR_PARSE_MISSING_MANDATORY) {
/* Set NLRI portion to structure. */
nlris[NLRI_UPDATE].afi = AFI_IP;
nlris[NLRI_UPDATE].safi = SAFI_UNICAST;
Expand Down
Loading