Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
From d8d77d3733bc299ed5dd7b44c4d464ba2bfed288 Mon Sep 17 00:00:00 2001
From: Donald Sharp <[email protected]>
Date: Wed, 20 Jul 2022 16:43:17 -0400
Subject: [PATCH 1/3] ospfclient: Ensure ospf_apiclient_lsa_originate cannot
Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PATCH 1/3

I see "PATCH 1/3" is backport of frr upstream commit. How about others? Could you explain 2/3 and 3/3 in PR description?

I see all 3 parts in the PR link https://github.com/FRRouting/frr/pull/12086/files

accidently write into stack

Even though OSPF_MAX_LSA_SIZE is quite large and holds the upper bound
on what can be written into a lsa, let's add a small check to ensure
it is not possible to do a bad thing.

This wins one of the long standing bug awards. 2003!

Fixes: #11602
Signed-off-by: Donald Sharp <[email protected]>
---
ospfclient/ospf_apiclient.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/ospfclient/ospf_apiclient.c b/ospfclient/ospf_apiclient.c
index 29f1c0807db..51c8a5b8c06 100644
--- a/ospfclient/ospf_apiclient.c
+++ b/ospfclient/ospf_apiclient.c
@@ -447,6 +447,12 @@ int ospf_apiclient_lsa_originate(struct ospf_apiclient *oclient,
return OSPF_API_ILLEGALLSATYPE;
}

+ if ((size_t)opaquelen > sizeof(buf) - sizeof(struct lsa_header)) {
+ fprintf(stderr, "opaquelen(%d) is larger than buf size %zu\n",
+ opaquelen, sizeof(buf));
+ return OSPF_API_NOMEMORY;
+ }
+
/* Make a new LSA from parameters */
lsah = (struct lsa_header *)buf;
lsah->ls_age = 0;

From 519929cdd47ac4d9f7f33e13922e1a063f69bb24 Mon Sep 17 00:00:00 2001
From: Donald Sharp <[email protected]>
Date: Wed, 20 Jul 2022 16:49:09 -0400
Subject: [PATCH 2/3] isisd: Ensure rcap is freed in error case

unpack_tlv_router_cap allocates memory that in the error
case is not being freed.

Signed-off-by: Donald Sharp <[email protected]>
---
isisd/isis_tlvs.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/isisd/isis_tlvs.c b/isisd/isis_tlvs.c
index f1aae7caf10..dad271225b3 100644
--- a/isisd/isis_tlvs.c
+++ b/isisd/isis_tlvs.c
@@ -2966,9 +2966,9 @@ static int pack_tlv_router_cap(const struct isis_router_cap *router_cap,
}

static int unpack_tlv_router_cap(enum isis_tlv_context context,
- uint8_t tlv_type, uint8_t tlv_len,
- struct stream *s, struct sbuf *log,
- void *dest, int indent)
+ uint8_t tlv_type, uint8_t tlv_len,
+ struct stream *s, struct sbuf *log, void *dest,
+ int indent)
{
struct isis_tlvs *tlvs = dest;
struct isis_router_cap *rcap;
@@ -3013,7 +3013,7 @@ static int unpack_tlv_router_cap(enum isis_tlv_context context,
log, indent,
"WARNING: Router Capability subTLV length too large compared to expected size\n");
stream_forward_getp(s, STREAM_READABLE(s));
-
+ XFREE(MTYPE_ISIS_TLV, rcap);
return 0;
}


From 3c4821679f2362bcd38fcc7803f28a5210441ddb Mon Sep 17 00:00:00 2001
From: Donald Sharp <[email protected]>
Date: Thu, 21 Jul 2022 08:11:58 -0400
Subject: [PATCH 3/3] bgpd: Make sure hdr length is at a minimum of what is
expected

Ensure that if the capability length specified is enough data.

Signed-off-by: Donald Sharp <[email protected]>
---
bgpd/bgp_packet.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 7c92a8d9e83..bcd47e32d45 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -2440,6 +2440,14 @@ static int bgp_capability_msg_parse(struct peer *peer, uint8_t *pnt,
"%s CAPABILITY has action: %d, code: %u, length %u",
peer->host, action, hdr->code, hdr->length);

+ if (hdr->length < sizeof(struct capability_mp_data)) {
+ zlog_info(
+ "%s Capability structure is not properly filled out, expected at least %zu bytes but header length specified is %d",
+ peer->host, sizeof(struct capability_mp_data),
+ hdr->length);
+ return BGP_Stop;
+ }
+
/* Capability length check. */
if ((pnt + hdr->length + 3) > end) {
zlog_info("%s Capability length error", peer->host);
1 change: 1 addition & 0 deletions src/sonic-frr/patch/series
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ cross-compile-changes.patch
0009-ignore-route-from-default-table.patch
0010-zebra-Note-when-the-netlink-DUMP-command-is-interrup.patch
0011-bgpd-enhanced-capability-is-always-turned-on-for-int.patch
0012-Ensure-ospf_apiclient_lsa_originate-cannot-accidently-write-into-stack.patch