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 crashes on malformed attributes #14645

Merged
merged 2 commits into from
Oct 25, 2023
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
Next Next commit
bgpd: Handle MP_REACH_NLRI malformed packets with session reset
Avoid crashing bgpd.

```
(gdb)
bgp_mp_reach_parse (args=<optimized out>, mp_update=0x7fffffffe140) at bgpd/bgp_attr.c:2341
2341			stream_get(&attr->mp_nexthop_global, s, IPV6_MAX_BYTELEN);
(gdb)
stream_get (dst=0x7fffffffe1ac, s=0x7ffff0006e80, size=16) at lib/stream.c:320
320	{
(gdb)
321		STREAM_VERIFY_SANE(s);
(gdb)
323		if (STREAM_READABLE(s) < size) {
(gdb)
34	  return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
(gdb)

Thread 1 "bgpd" received signal SIGSEGV, Segmentation fault.
0x00005555556e37be in route_set_aspath_prepend (rule=0x555555aac0d0, prefix=0x7fffffffe050,
    object=0x7fffffffdb00) at bgpd/bgp_routemap.c:2282
2282		if (path->attr->aspath->refcnt)
(gdb)
```

With the configuration:

```
 neighbor 127.0.0.1 remote-as external
 neighbor 127.0.0.1 passive
 neighbor 127.0.0.1 ebgp-multihop
 neighbor 127.0.0.1 disable-connected-check
 neighbor 127.0.0.1 update-source 127.0.0.2
 neighbor 127.0.0.1 timers 3 90
 neighbor 127.0.0.1 timers connect 1
 address-family ipv4 unicast
  redistribute connected
  neighbor 127.0.0.1 default-originate
  neighbor 127.0.0.1 route-map RM_IN in
 exit-address-family
!
route-map RM_IN permit 10
 set as-path prepend 200
exit
```

Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
  • Loading branch information
ton31337 committed Oct 24, 2023
commit b08afc81c60607a4f736f418f2e3eb06087f1a35
6 changes: 1 addition & 5 deletions bgpd/bgp_attr.c
Original file line number Diff line number Diff line change
Expand Up @@ -2421,7 +2421,7 @@ int bgp_mp_reach_parse(struct bgp_attr_parser_args *args,

mp_update->afi = afi;
mp_update->safi = safi;
return BGP_ATTR_PARSE_EOR;
return bgp_attr_malformed(args, BGP_NOTIFY_UPDATE_MAL_ATTR, 0);
}

mp_update->afi = afi;
Expand Down Expand Up @@ -3759,10 +3759,6 @@ enum bgp_attr_parse_ret bgp_attr_parse(struct peer *peer, struct attr *attr,
goto done;
}

if (ret == BGP_ATTR_PARSE_EOR) {
goto done;
}

if (ret == BGP_ATTR_PARSE_ERROR) {
flog_warn(EC_BGP_ATTRIBUTE_PARSE_ERROR,
"%s: Attribute %s, parse error", peer->host,
Expand Down
1 change: 0 additions & 1 deletion bgpd/bgp_attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,6 @@ 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_EOR = -4,
};

struct bpacket_attr_vec_arr;
Expand Down
6 changes: 1 addition & 5 deletions bgpd/bgp_packet.c
Original file line number Diff line number Diff line change
Expand Up @@ -2397,8 +2397,7 @@ static int bgp_update_receive(struct peer_connection *connection,
* Non-MP IPv4/Unicast EoR is a completely empty UPDATE
* and MP EoR should have only an empty MP_UNREACH
*/
if ((!update_len && !withdraw_len && nlris[NLRI_MP_UPDATE].length == 0)
|| (attr_parse_ret == BGP_ATTR_PARSE_EOR)) {
if (!update_len && !withdraw_len && nlris[NLRI_MP_UPDATE].length == 0) {
afi_t afi = 0;
safi_t safi;
struct graceful_restart_info *gr_info;
Expand All @@ -2419,9 +2418,6 @@ static int bgp_update_receive(struct peer_connection *connection,
&& nlris[NLRI_MP_WITHDRAW].length == 0) {
afi = nlris[NLRI_MP_WITHDRAW].afi;
safi = nlris[NLRI_MP_WITHDRAW].safi;
} else if (attr_parse_ret == BGP_ATTR_PARSE_EOR) {
afi = nlris[NLRI_MP_UPDATE].afi;
safi = nlris[NLRI_MP_UPDATE].safi;
}

if (afi && peer->afc[afi][safi]) {
Expand Down