-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Created patches to address two CVEs from FRR CVE-2023-41358 and CVE-2023-38802. Patch FRR commit CVE fixed 0024-bgpd-Do-not-process-NLRIs-if-the-attribute-length-is.patch FRRouting/frr@f291f1e CVE-2023-41358 0025-bgpd-Use-treat-as-withdraw-for-tunnel-encapsulation-.patch FRRouting/frr@8a4a88c CVE-2023-38802
- Loading branch information
1 parent
c31ccba
commit eea4da3
Showing
3 changed files
with
228 additions
and
0 deletions.
There are no files selected for viewing
97 changes: 97 additions & 0 deletions
97
src/sonic-frr/patch/0024-bgpd-Do-not-process-NLRIs-if-the-attribute-length-is.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| From fc5cb12121cb1858c162f9ed8f6206fc61439455 Mon Sep 17 00:00:00 2001 | ||
| From: Donatas Abraitis <[email protected]> | ||
| 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 <[email protected]> | ||
| Signed-off-by: Donatas Abraitis <[email protected]> | ||
| (cherry picked from commit 28ccc24d38df1d51ed8a563507e5d6f6171fdd38) | ||
|
|
||
| diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c | ||
| index 60f1dcbcd..a02d54894 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; | ||
| -- | ||
| 2.17.1 | ||
|
|
129 changes: 129 additions & 0 deletions
129
src/sonic-frr/patch/0025-bgpd-Use-treat-as-withdraw-for-tunnel-encapsulation-.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| From 35b98f89f6a2ca5a79ed8893b4df612c0c6b4a37 Mon Sep 17 00:00:00 2001 | ||
| From: Donatas Abraitis <[email protected]> | ||
| 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 <[email protected]> | ||
| (cherry picked from commit bcb6b58d9530173df41d3a3cbc4c600ee0b4b186) | ||
|
|
||
| diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c | ||
| index 2ef50ffe5..ee20da332 100644 | ||
| --- a/bgpd/bgp_attr.c | ||
| +++ b/bgpd/bgp_attr.c | ||
| @@ -1416,6 +1416,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: | ||
| case BGP_ATTR_OTC: | ||
| return BGP_ATTR_PARSE_WITHDRAW; | ||
| case BGP_ATTR_MP_REACH_NLRI: | ||
| @@ -2644,26 +2645,21 @@ ipv6_ext_community_ignore: | ||
| } | ||
|
|
||
| /* 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) { | ||
| @@ -2671,12 +2667,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)); | ||
| @@ -2706,13 +2701,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 */ | ||
| @@ -2760,13 +2753,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; | ||
| @@ -3718,8 +3708,7 @@ enum bgp_attr_parse_ret 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); | ||
| -- | ||
| 2.17.1 | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters