Skip to content

Commit

Permalink
[FRR] Adding patches for CVE-2023-41358 CVE-2023-41909 CVE-2023-38802
Browse files Browse the repository at this point in the history
  • Loading branch information
@DavidZagury
DavidZagury committed Sep 21, 2023
1 parent d2fe623 commit 29cff73
Show file tree
Hide file tree
Showing 4 changed files with 266 additions and 0 deletions.
97 changes: 97 additions & 0 deletions src/sonic-frr/patch/0032-bgpd-Do-not-process-NLRIs-if-the-attribute-length-is-zero.patch
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
From f291f1ee9434f56d4b185db0652794a92e313b00 Mon Sep 17 00:00:00 2001
From: Donatas Abraitis <donatas@opensourcerouting.org>
Date: Tue, 22 Aug 2023 22:52:04 +0300
Subject: [PATCH] bgpd: Do not process NLRIs if the attribute length is zero

```
3 0x00007f423aa42476 in __GI_raise (sig=sig@entry=11) at ../sysdeps/posix/raise.c:26
4 0x00007f423aef9740 in core_handler (signo=11, siginfo=0x7fffc414deb0, context=<optimized out>) at lib/sigevent.c:246
5 <signal handler called>
6 0x0000564dea2fc71e in route_set_aspath_prepend (rule=0x564debd66d50, prefix=0x7fffc414ea30, object=0x7fffc414e400)
at bgpd/bgp_routemap.c:2258
7 0x00007f423aeec7e0 in route_map_apply_ext (map=<optimized out>, prefix=prefix@entry=0x7fffc414ea30,
match_object=match_object@entry=0x7fffc414e400, set_object=set_object@entry=0x7fffc414e400, pref=pref@entry=0x0) at lib/routemap.c:2690
8 0x0000564dea2d277e in bgp_input_modifier (peer=peer@entry=0x7f4238f59010, p=p@entry=0x7fffc414ea30, attr=attr@entry=0x7fffc414e770,
afi=afi@entry=AFI_IP, safi=safi@entry=SAFI_UNICAST, rmap_name=rmap_name@entry=0x0, label=0x0, num_labels=0, dest=0x564debdd5130)
at bgpd/bgp_route.c:1772
9 0x0000564dea2df762 in bgp_update (peer=peer@entry=0x7f4238f59010, p=p@entry=0x7fffc414ea30, addpath_id=addpath_id@entry=0,
attr=0x7fffc414eb50, afi=afi@entry=AFI_IP, safi=<optimized out>, safi@entry=SAFI_UNICAST, type=9, sub_type=0, prd=0x0, label=0x0,
num_labels=0, soft_reconfig=0, evpn=0x0) at bgpd/bgp_route.c:4374
10 0x0000564dea2e2047 in bgp_nlri_parse_ip (peer=0x7f4238f59010, attr=attr@entry=0x7fffc414eb50, packet=0x7fffc414eaf0)
at bgpd/bgp_route.c:6249
11 0x0000564dea2c5a58 in bgp_nlri_parse (peer=peer@entry=0x7f4238f59010, attr=attr@entry=0x7fffc414eb50,
packet=packet@entry=0x7fffc414eaf0, mp_withdraw=mp_withdraw@entry=false) at bgpd/bgp_packet.c:339
12 0x0000564dea2c5d66 in bgp_update_receive (peer=peer@entry=0x7f4238f59010, size=size@entry=109) at bgpd/bgp_packet.c:2024
13 0x0000564dea2c901d in bgp_process_packet (thread=<optimized out>) at bgpd/bgp_packet.c:2933
14 0x00007f423af0bf71 in event_call (thread=thread@entry=0x7fffc414ee40) at lib/event.c:1995
15 0x00007f423aebb198 in frr_run (master=0x564deb73c670) at lib/libfrr.c:1213
16 0x0000564dea261b83 in main (argc=<optimized out>, argv=<optimized out>) at bgpd/bgp_main.c:505
```

With the configuration:

```
frr version 9.1-dev-MyOwnFRRVersion
frr defaults traditional
hostname ip-172-31-13-140
log file /tmp/debug.log
log syslog
service integrated-vtysh-config
!
debug bgp keepalives
debug bgp neighbor-events
debug bgp updates in
debug bgp updates out
!
router bgp 100
bgp router-id 9.9.9.9
no bgp ebgp-requires-policy
bgp bestpath aigp
neighbor 172.31.2.47 remote-as 200
!
address-family ipv4 unicast
neighbor 172.31.2.47 default-originate
neighbor 172.31.2.47 route-map RM_IN in
exit-address-family
exit
!
route-map RM_IN permit 10
set as-path prepend 200
exit
!
```

The issue is that we try to process NLRIs even if the attribute length is 0.

Later bgp_update() will handle route-maps and a crash occurs because all the
attributes are NULL, including aspath, where we dereference.

According to the RFC 4271:

A value of 0 indicates that neither the Network Layer
Reachability Information field nor the Path Attribute field is
present in this UPDATE message.

But with a fuzzed UPDATE message this can be faked. I think it's reasonable
to skip processing NLRIs if both update_len and attribute_len are 0.

Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
(cherry picked from commit 28ccc24d38df1d51ed8a563507e5d6f6171fdd38)
---
bgpd/bgp_packet.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 60f1dcbcd67a..a02d54894148 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -1983,7 +1983,7 @@ static int bgp_update_receive(struct peer *peer, bgp_size_t size)
/* Network Layer Reachability Information. */
update_len = end - stream_pnt(s);

- if (update_len) {
+ if (update_len && attribute_len) {
/* Set NLRI portion to structure. */
nlris[NLRI_UPDATE].afi = AFI_IP;
nlris[NLRI_UPDATE].safi = SAFI_UNICAST;
35 changes: 35 additions & 0 deletions ...onic-frr/patch/0033-bgpd-Limit-flowspec-to-no-attribute-means-a-implicit-withdrawal.patch
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
From cc1a551cb007cc8ed8b1ea0605a7ab46c16de12b Mon Sep 17 00:00:00 2001
From: Donald Sharp <sharpd@nvidia.com>
Date: Wed, 5 Apr 2023 14:57:05 -0400
Subject: [PATCH] bgpd: Limit flowspec to no attribute means a implicit
withdrawal

All other parsing functions done from bgp_nlri_parse() assume
no attributes == an implicit withdrawal. Let's move
bgp_nlri_parse_flowspec() into the same alignment.

Reported-by: Matteo Memelli <mmemelli@amazon.it>
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
(cherry picked from commit cfd04dcb3e689754a72507d086ba3b9709fc5ed8)
---
bgpd/bgp_flowspec.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/bgpd/bgp_flowspec.c b/bgpd/bgp_flowspec.c
index 11396e374ff8..c4e32cd3bb5a 100644
--- a/bgpd/bgp_flowspec.c
+++ b/bgpd/bgp_flowspec.c
@@ -111,6 +111,13 @@ int bgp_nlri_parse_flowspec(struct peer *peer, struct attr *attr,
afi = packet->afi;
safi = packet->safi;

+ /*
+ * All other AFI/SAFI's treat no attribute as a implicit
+ * withdraw. Flowspec should as well.
+ */
+ if (!attr)
+ withdraw = 1;
+
if (packet->length >= FLOWSPEC_NLRI_SIZELIMIT_EXTENDED) {
flog_err(EC_BGP_FLOWSPEC_PACKET,
"BGP flowspec nlri length maximum reached (%u)",
131 changes: 131 additions & 0 deletions src/sonic-frr/patch/0034-bgpd-Use-treat-as-withdraw-for-tunnel-encapsulation-attribute.patch
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
From a3337f1bd534d1d2b47b149a14418c5d93d341e3 Mon Sep 17 00:00:00 2001
From: Donatas Abraitis <donatas@opensourcerouting.org>
Date: Thu, 13 Jul 2023 22:32:03 +0300
Subject: [PATCH] bgpd: Use treat-as-withdraw for tunnel encapsulation
attribute

Before this path we used session reset method, which is discouraged by rfc7606.

Handle this as rfc requires.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Signed-off-by: Christian Breunig <christian@breunig.cc>

(cherry picked from commit bcb6b58d9530173df41d3a3cbc4c600ee0b4b186)
---
bgpd/bgp_attr.c | 61 ++++++++++++++++++++-----------------------------
1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index f5e54267d484..4bc28fe2374a 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1225,6 +1225,7 @@ bgp_attr_malformed(struct bgp_attr_parser_args *args, uint8_t subcode,
case BGP_ATTR_LARGE_COMMUNITIES:
case BGP_ATTR_ORIGINATOR_ID:
case BGP_ATTR_CLUSTER_LIST:
+ case BGP_ATTR_ENCAP:
return BGP_ATTR_PARSE_WITHDRAW;
case BGP_ATTR_MP_REACH_NLRI:
case BGP_ATTR_MP_UNREACH_NLRI:
@@ -2320,26 +2321,21 @@ bgp_attr_ipv6_ext_communities(struct bgp_attr_parser_args *args)
}

/* Parse Tunnel Encap attribute in an UPDATE */
-static int bgp_attr_encap(uint8_t type, struct peer *peer, /* IN */
- bgp_size_t length, /* IN: attr's length field */
- struct attr *attr, /* IN: caller already allocated */
- uint8_t flag, /* IN: attr's flags field */
- uint8_t *startp)
+static int bgp_attr_encap(struct bgp_attr_parser_args *args)
{
- bgp_size_t total;
uint16_t tunneltype = 0;
-
- total = length + (CHECK_FLAG(flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
+ struct peer *const peer = args->peer;
+ struct attr *const attr = args->attr;
+ bgp_size_t length = args->length;
+ uint8_t type = args->type;
+ uint8_t flag = args->flags;

if (!CHECK_FLAG(flag, BGP_ATTR_FLAG_TRANS)
|| !CHECK_FLAG(flag, BGP_ATTR_FLAG_OPTIONAL)) {
- zlog_info(
- "Tunnel Encap attribute flag isn't optional and transitive %d",
- flag);
- bgp_notify_send_with_data(peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
- startp, total);
- return -1;
+ zlog_err("Tunnel Encap attribute flag isn't optional and transitive %d",
+ flag);
+ return bgp_attr_malformed(args, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
+ args->total);
}

if (BGP_ATTR_ENCAP == type) {
@@ -2347,12 +2343,11 @@ static int bgp_attr_encap(uint8_t type, struct peer *peer, /* IN */
uint16_t tlv_length;

if (length < 4) {
- zlog_info(
+ zlog_err(
"Tunnel Encap attribute not long enough to contain outer T,L");
- bgp_notify_send_with_data(
- peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, startp, total);
- return -1;
+ return bgp_attr_malformed(args,
+ BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
+ args->total);
}
tunneltype = stream_getw(BGP_INPUT(peer));
tlv_length = stream_getw(BGP_INPUT(peer));
@@ -2382,13 +2377,11 @@ static int bgp_attr_encap(uint8_t type, struct peer *peer, /* IN */
}

if (sublength > length) {
- zlog_info(
- "Tunnel Encap attribute sub-tlv length %d exceeds remaining length %d",
- sublength, length);
- bgp_notify_send_with_data(
- peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR, startp, total);
- return -1;
+ zlog_err("Tunnel Encap attribute sub-tlv length %d exceeds remaining length %d",
+ sublength, length);
+ return bgp_attr_malformed(args,
+ BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
+ args->total);
}

/* alloc and copy sub-tlv */
@@ -2434,13 +2427,10 @@ static int bgp_attr_encap(uint8_t type, struct peer *peer, /* IN */

if (length) {
/* spurious leftover data */
- zlog_info(
- "Tunnel Encap attribute length is bad: %d leftover octets",
- length);
- bgp_notify_send_with_data(peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
- startp, total);
- return -1;
+ zlog_err("Tunnel Encap attribute length is bad: %d leftover octets",
+ length);
+ return bgp_attr_malformed(args, BGP_NOTIFY_UPDATE_OPT_ATTR_ERR,
+ args->total);
}

return 0;
@@ -3128,8 +3118,7 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr,
case BGP_ATTR_VNC:
#endif
case BGP_ATTR_ENCAP:
- ret = bgp_attr_encap(type, peer, length, attr, flag,
- startp);
+ ret = bgp_attr_encap(&attr_args);
break;
case BGP_ATTR_PREFIX_SID:
ret = bgp_attr_prefix_sid(&attr_args);
3 changes: 3 additions & 0 deletions src/sonic-frr/patch/series
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,6 @@ Disable-ipv6-src-address-test-in-pceplib.patch
0029-bgpd-Change-log-level-for-graceful-restart-events.patch
0030-zebra-remove-duplicated-nexthops-when-sending-fpm-msg.patch
0031-lib-Fix-corruption-when-routemap-delete-add-sequence.patch
0032-bgpd-Do-not-process-NLRIs-if-the-attribute-length-is-zero.patch
0033-bgpd-Limit-flowspec-to-no-attribute-means-a-implicit-withdrawal.patch
0034-bgpd-Use-treat-as-withdraw-for-tunnel-encapsulation-attribute.patch

0 comments on commit 29cff73

Please sign in to comment.