From eb9e2865116777661c44963769c1a5fed764b7f9 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 28 Sep 2023 15:08:23 +0200 Subject: [PATCH 1/6] bgpd: do not check attr in bgp_packet_attribute Fix the following coverity issue. attr cannot be NULL. > CID 1568376 (#1 of 1): Dereference before null check (REVERSE_INULL) > check_after_deref: Null-checking attr suggests that it may be null, but it has already been dereferenced on all paths leading to the check. Fixes: 8b531b1107 ("bgpd: store and send bgp link-state attributes") Signed-off-by: Louis Scalbert --- bgpd/bgp_attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index cc7afbe74f..05cf63e053 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -4982,7 +4982,7 @@ bgp_size_t bgp_packet_attribute(struct bgp *bgp, struct peer *peer, } /* BGP Link-State */ - if (attr && attr->link_state) { + if (attr->link_state) { stream_putc(s, BGP_ATTR_FLAG_OPTIONAL); stream_putc(s, BGP_ATTR_LINK_STATE); stream_putc(s, attr->link_state->length); From dae5791c446cd18d8cda93a1e578fff2cd27be10 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 28 Sep 2023 15:27:27 +0200 Subject: [PATCH 2/6] bgpd: fix illegal memory access in bgp_ls_tlv_check_size() Fix illegal memory access bgp_ls_tlv_check_size() if type is 1253. > CID 1568377 (#4 of 4): Out-of-bounds read (OVERRUN) > 5. overrun-local: Overrunning array bgp_linkstate_tlv_infos of 1253 16-byte elements at element index 1253 (byte offset 20063) using index type (which evaluates to 1253). Fixes: 7e0d9ff8ba ("bgpd: display link-state prefixes detail") Signed-off-by: Louis Scalbert --- bgpd/bgp_linkstate_tlv.c | 8 ++++---- bgpd/bgp_linkstate_tlv.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/bgpd/bgp_linkstate_tlv.c b/bgpd/bgp_linkstate_tlv.c index 5538f7a761..6b7d8d2f3e 100644 --- a/bgpd/bgp_linkstate_tlv.c +++ b/bgpd/bgp_linkstate_tlv.c @@ -31,7 +31,7 @@ struct bgp_linkstate_tlv_info { #define UNDEF_MULTPL 1 /* clang-format off */ -struct bgp_linkstate_tlv_info bgp_linkstate_tlv_infos[BGP_LS_TLV_MAX] = { +struct bgp_linkstate_tlv_info bgp_linkstate_tlv_infos[BGP_LS_TLV_MAX + 1] = { /* NLRI TLV */ [BGP_LS_TLV_LOCAL_NODE_DESCRIPTORS] = {"Local Node Descriptors", 1, MAX_SZ, UNDEF_MULTPL}, [BGP_LS_TLV_REMOTE_NODE_DESCRIPTORS] = {"Remote Node Descriptors", 1, MAX_SZ, UNDEF_MULTPL}, @@ -1706,7 +1706,7 @@ void bgp_linkstate_tlv_attribute_display(struct vty *vty, json_tlv = json_object_new_object(); json_object_object_add(json, tlv_type, json_tlv); - if (type < BGP_LS_TLV_MAX && + if (type <= BGP_LS_TLV_MAX && bgp_linkstate_tlv_infos[type].descr != NULL) json_object_string_add( json_tlv, "description", @@ -1721,7 +1721,7 @@ void bgp_linkstate_tlv_attribute_display(struct vty *vty, "too high length received: %u", length); break; } - if (type < BGP_LS_TLV_MAX && + if (type <= BGP_LS_TLV_MAX && bgp_linkstate_tlv_infos[type].descr != NULL && !bgp_ls_tlv_check_size(type, length)) json_object_string_addf( @@ -1729,7 +1729,7 @@ void bgp_linkstate_tlv_attribute_display(struct vty *vty, "unexpected length received: %u", length); } else { - if (type < BGP_LS_TLV_MAX && + if (type <= BGP_LS_TLV_MAX && bgp_linkstate_tlv_infos[type].descr != NULL) vty_out(vty, "%*s%s: ", indent, "", bgp_linkstate_tlv_infos[type].descr); diff --git a/bgpd/bgp_linkstate_tlv.h b/bgpd/bgp_linkstate_tlv.h index ad3b2570d6..cc543735b7 100644 --- a/bgpd/bgp_linkstate_tlv.h +++ b/bgpd/bgp_linkstate_tlv.h @@ -197,7 +197,7 @@ enum bgp_linkstate_tlv { 1251, /* draft-ietf-idr-bgpls-srv6-ext-08 */ BGP_LS_TLV_SRV6_SID_STRUCTURE_TLV = 1252, /* draft-ietf-idr-bgpls-srv6-ext-08 */ - BGP_LS_TLV_MAX = 1253, /* max TLV value for table size*/ + BGP_LS_TLV_MAX = 1252, /* max TLV value for table size*/ }; /* RFC7752 #3.2.1.4 IGP router-ID */ From 25408c8dbf7d9e0149ceb2dcbd2058860ce4f6c5 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 28 Sep 2023 15:33:58 +0200 Subject: [PATCH 3/6] bgpd: fix link_state_hash_cmp() Fix comparaison of link state attributes pointers in link_state_hash_cmp(). > CID 1568379 (#1 of 1): Logically dead code (DEADCODE) > dead_error_line: Execution cannot reach this statement: return false;. Fixes: 8b531b1107 ("bgpd: store and send bgp link-state attributes") Signed-off-by: Louis Scalbert --- bgpd/bgp_attr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 05cf63e053..808b98e419 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -780,7 +780,7 @@ static bool link_state_hash_cmp(const void *p1, const void *p2) if (!link_state1 && link_state2) return false; - if (!link_state1 && link_state2) + if (link_state1 && !link_state2) return false; if (!link_state1 && !link_state2) return true; From 54222f921305edbce74e81996e9303c0c6b03823 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 28 Sep 2023 16:53:35 +0200 Subject: [PATCH 4/6] bgpd: fix insecure data write with ip addresses Fix issues where an attacker may inject a tainted length value to corrupt the memory. > CID 1568378 (#1-6 of 6): Untrusted value as argument (TAINTED_SCALAR) > 16. tainted_data: Passing tainted expression length to bgp_linkstate_tlv_attribute_value_display, which uses it as an offset. [show details] Fixes: 7e0d9ff8ba ("bgpd: display link-state prefixes detail") Signed-off-by: Louis Scalbert --- bgpd/bgp_linkstate_tlv.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_linkstate_tlv.c b/bgpd/bgp_linkstate_tlv.c index 6b7d8d2f3e..1594d8fd95 100644 --- a/bgpd/bgp_linkstate_tlv.c +++ b/bgpd/bgp_linkstate_tlv.c @@ -577,7 +577,8 @@ static bool bgp_linkstate_nlri_value_display(char *buf, size_t size, break; case BGP_LS_TLV_IP_REACHABILITY_INFORMATION: mask_length = pnt_decode8(&pnt); - if (nlri_type == BGP_LINKSTATE_PREFIX4) { + if (nlri_type == BGP_LINKSTATE_PREFIX4 && + ((length - sizeof(mask_length)) <= sizeof(ipv4.s_addr))) { memcpy(&ipv4.s_addr, pnt, length - sizeof(mask_length)); if (json) json_object_string_addf(json, "ipReachability", @@ -587,7 +588,8 @@ static bool bgp_linkstate_nlri_value_display(char *buf, size_t size, snprintfrr(buf, size, "%sIPv4:%pI4/%u", first ? "" : " ", &ipv4, mask_length); - } else if (nlri_type == BGP_LINKSTATE_PREFIX6) { + } else if (nlri_type == BGP_LINKSTATE_PREFIX6 && + ((length - sizeof(mask_length)) <= sizeof(ipv6))) { memcpy(&ipv6, pnt, length - sizeof(mask_length)); if (json) json_object_string_addf(json, "ipReachability", From 57d0dc565f6a99c3d61b9b67a40ad88a83773eb6 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 28 Sep 2023 16:55:43 +0200 Subject: [PATCH 5/6] bgpd: fix insecure data write with area addresses Fix an issue where an attacker may inject a tainted length value to corrupt the memory. > CID 1568380 (#1 of 1): Untrusted value as argument (TAINTED_SCALAR) > 9. tainted_data: Passing tainted expression length to bgp_linkstate_nlri_value_display, which uses it as an offset Fixes: 8b531b1107 ("bgpd: store and send bgp link-state attributes") Signed-off-by: Louis Scalbert --- bgpd/bgp_linkstate_tlv.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bgpd/bgp_linkstate_tlv.c b/bgpd/bgp_linkstate_tlv.c index 1594d8fd95..f2bd36524d 100644 --- a/bgpd/bgp_linkstate_tlv.c +++ b/bgpd/bgp_linkstate_tlv.c @@ -1528,6 +1528,11 @@ static void bgp_linkstate_tlv_isis_area_indentifier_display(struct vty *vty, { struct iso_address addr; + if (length > sizeof(addr.area_addr)) { + bgp_linkstate_tlv_hexa_display(vty, pnt, length, json); + return; + } + addr.addr_len = length; memcpy(addr.area_addr, pnt, length); From e1333d12e0d3ba8a9a0ca914dc72d0908dea494e Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 28 Sep 2023 17:38:13 +0200 Subject: [PATCH 6/6] bgpd: fix printing link state ospf opaque data Fix printing link state ospf opaque data. pnt address was not moving in the loop. Fixes: 8b531b1107 ("bgpd: store and send bgp link-state attributes") Signed-off-by: Louis Scalbert --- bgpd/bgp_linkstate_tlv.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/bgpd/bgp_linkstate_tlv.c b/bgpd/bgp_linkstate_tlv.c index f2bd36524d..3669d68b85 100644 --- a/bgpd/bgp_linkstate_tlv.c +++ b/bgpd/bgp_linkstate_tlv.c @@ -1354,7 +1354,6 @@ static void bgp_linkstate_tlv_opaque_display(struct vty *vty, uint8_t *pnt, uint8_t *lim = pnt + length; bool ospf_tlv_header; char tlv_type[6]; - int i; if (json) { @@ -1400,21 +1399,15 @@ static void bgp_linkstate_tlv_opaque_display(struct vty *vty, uint8_t *pnt, continue; } - vty_out(vty, "\n%*sTLV type %u: 0x", indent, "", sub_type); + vty_out(vty, "\n%*sTLV type %u: ", indent, "", sub_type); if (pnt + sub_length > lim) { vty_out(vty, "Bad length received: %u\n", sub_length); break; } - for (i = 0; i < sub_length; i++) { - if (i != 0 && i % 8 == 0) - vty_out(vty, " "); - vty_out(vty, "%02x", *pnt); - } + bgp_linkstate_tlv_hexa_display(vty, pnt, sub_length, NULL); } - if (!json) - vty_out(vty, "\n"); } static void bgp_linkstate_tlv_rtm_capability_display(struct vty *vty,