From b571176aea651de626e830b9e79e848851eb982d Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 16 Jan 2024 14:35:10 +0200 Subject: [PATCH 1/6] bgpd: Set AS4 capability received flag only if parsed correctly If we receive a malformed packet, we might end-up with a bad state. Signed-off-by: Donatas Abraitis --- bgpd/bgp_open.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index 09f16bbce6..b85e717457 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -622,8 +622,6 @@ static int bgp_capability_llgr(struct peer *peer, /* Unlike other capability parsing routines, this one returns 0 on error */ static as_t bgp_capability_as4(struct peer *peer, struct capability_header *hdr) { - SET_FLAG(peer->cap, PEER_CAP_AS4_RCV); - if (hdr->length != CAPABILITY_CODE_AS4_LEN) { flog_err(EC_BGP_PKT_OPEN, "%s AS4 capability has incorrect data length %d", @@ -633,6 +631,8 @@ static as_t bgp_capability_as4(struct peer *peer, struct capability_header *hdr) as_t as4 = stream_getl(BGP_INPUT(peer)); + SET_FLAG(peer->cap, PEER_CAP_AS4_RCV); + if (BGP_DEBUG(as4, AS4)) zlog_debug( "%s [AS4] about to set cap PEER_CAP_AS4_RCV, got as4 %u", From 9b178d246ebeade3beced818eb03d5dac2c1fc2b Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 16 Jan 2024 14:36:20 +0200 Subject: [PATCH 2/6] bgpd: Set ADD-PATH capability received flag only if parsed correctly If we receive a malformed packet, we might end-up with a bad state. Signed-off-by: Donatas Abraitis --- bgpd/bgp_open.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index b85e717457..8b4f97d625 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -662,8 +662,6 @@ static int bgp_capability_addpath(struct peer *peer, struct stream *s = BGP_INPUT(peer); size_t end = stream_get_getp(s) + hdr->length; - SET_FLAG(peer->cap, PEER_CAP_ADDPATH_RCV); - /* Verify length is a multiple of 4 */ if (hdr->length % CAPABILITY_CODE_ADDPATH_LEN) { flog_warn( @@ -673,6 +671,8 @@ static int bgp_capability_addpath(struct peer *peer, return -1; } + SET_FLAG(peer->cap, PEER_CAP_ADDPATH_RCV); + while (stream_get_getp(s) + CAPABILITY_CODE_ADDPATH_LEN <= end) { afi_t afi; safi_t safi; From 0c74220c6e841a3ca0c8534b34a138d5d05a1ebb Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 16 Jan 2024 14:37:16 +0200 Subject: [PATCH 3/6] bgpd: Set hostname capability received flag only if parsed correctly If we receive a malformed packet, we might end-up with a bad state. Signed-off-by: Donatas Abraitis --- bgpd/bgp_open.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index 8b4f97d625..cb13801745 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -818,8 +818,6 @@ static int bgp_capability_hostname(struct peer *peer, size_t end = stream_get_getp(s) + hdr->length; uint8_t len; - SET_FLAG(peer->cap, PEER_CAP_HOSTNAME_RCV); - len = stream_getc(s); if (stream_get_getp(s) + len > end) { flog_warn( @@ -877,6 +875,8 @@ static int bgp_capability_hostname(struct peer *peer, peer->domainname = XSTRDUP(MTYPE_BGP_PEER_HOST, str); } + SET_FLAG(peer->cap, PEER_CAP_HOSTNAME_RCV); + if (bgp_debug_neighbor_events(peer)) { zlog_debug("%s received hostname %s, domainname %s", peer->host, peer->hostname, peer->domainname); From 722195d4eca0279093140727fe4b5d3143a827b7 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 16 Jan 2024 14:37:51 +0200 Subject: [PATCH 4/6] bgpd: Set role capability received flag only if parsed correctly If we receive a malformed packet, we might end-up with a bad state. Signed-off-by: Donatas Abraitis --- bgpd/bgp_open.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index cb13801745..ac7fbc215b 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -887,14 +887,16 @@ static int bgp_capability_hostname(struct peer *peer, static int bgp_capability_role(struct peer *peer, struct capability_header *hdr) { - SET_FLAG(peer->cap, PEER_CAP_ROLE_RCV); if (hdr->length != CAPABILITY_CODE_ROLE_LEN) { flog_warn(EC_BGP_CAPABILITY_INVALID_LENGTH, "Role: Received invalid length %d", hdr->length); return -1; } + uint8_t role = stream_getc(BGP_INPUT(peer)); + SET_FLAG(peer->cap, PEER_CAP_ROLE_RCV); + peer->remote_role = role; return 0; } From 02a5da440b40efa3fd80eb60f81997250f98c561 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 16 Jan 2024 15:29:32 +0200 Subject: [PATCH 5/6] bgpd: Send notification if AS4 capability failed to parse (malformed) Signed-off-by: Donatas Abraitis --- bgpd/bgp_open.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index ac7fbc215b..154efdedaf 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -626,7 +626,7 @@ static as_t bgp_capability_as4(struct peer *peer, struct capability_header *hdr) flog_err(EC_BGP_PKT_OPEN, "%s AS4 capability has incorrect data length %d", peer->host, hdr->length); - return 0; + return -1; } as_t as4 = stream_getl(BGP_INPUT(peer)); From 90254e7bc2ff70218f89df1211eb6e3df98b97e6 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 16 Jan 2024 16:28:41 +0200 Subject: [PATCH 6/6] tests: Adopt tests for AS4 handling When received malformed AS4 capability, it should return -1 (notification send), and the received flag SHOULD NOT be set. Signed-off-by: Donatas Abraitis --- tests/bgpd/test_aspath.c | 10 +++++----- tests/bgpd/test_capability.c | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/bgpd/test_aspath.c b/tests/bgpd/test_aspath.c index 799733b7b5..29d2e4cf6e 100644 --- a/tests/bgpd/test_aspath.c +++ b/tests/bgpd/test_aspath.c @@ -653,7 +653,7 @@ static struct aspath_tests { "8466 3 52737 4096", AS4_DATA, -1, - PEER_CAP_AS4_RCV, + 0, { COMMON_ATTRS, BGP_ATTR_FLAG_TRANS | BGP_ATTR_FLAG_OPTIONAL, @@ -685,7 +685,7 @@ static struct aspath_tests { "8466 3 52737 4096", AS4_DATA, -1, - PEER_CAP_AS4_RCV | PEER_CAP_AS4_ADV, + 0, { COMMON_ATTRS, BGP_ATTR_FLAG_TRANS, @@ -701,7 +701,7 @@ static struct aspath_tests { "8466 3 52737 4096", AS4_DATA, -1, - PEER_CAP_AS4_RCV | PEER_CAP_AS4_ADV, + 0, { COMMON_ATTRS, BGP_ATTR_FLAG_TRANS, @@ -717,7 +717,7 @@ static struct aspath_tests { "8466 3 52737 4096", AS4_DATA, -1, - PEER_CAP_AS4_RCV | PEER_CAP_AS4_ADV, + 0, { COMMON_ATTRS, BGP_ATTR_FLAG_TRANS, @@ -733,7 +733,7 @@ static struct aspath_tests { "8466 3 52737 4096", AS4_DATA, -1, - PEER_CAP_AS4_RCV | PEER_CAP_AS4_ADV, + 0, { COMMON_ATTRS, BGP_ATTR_FLAG_TRANS | BGP_ATTR_FLAG_OPTIONAL, diff --git a/tests/bgpd/test_capability.c b/tests/bgpd/test_capability.c index 38f896b30c..1ee47a38e4 100644 --- a/tests/bgpd/test_capability.c +++ b/tests/bgpd/test_capability.c @@ -617,6 +617,7 @@ static struct test_segment misc_segments[] = }, 2, SHOULD_ERR, + -1, }, { "dyn-empty",