From be18a0fef9a2582a40292de12c409dff452547cd Mon Sep 17 00:00:00 2001 From: Naveen Thanikachalam Date: Thu, 9 Apr 2020 00:27:54 -0700 Subject: [PATCH] bgpd: Enforce self-next-hop check in next-hop update. Problem Description: ===================== +--+ +--+ |R1|-(192.201.202.1)----iBGP----(192.201.202.2)-|R2| +--+ +--+ Routes on R2: ============= S>* 202.202.202.202/32 [1/0] via 192.201.78.1, ens256, 00:40:48 Where, the next-hop network, 192.201.78.0/24, is a directly connected network address. C>* 192.201.78.0/24 is directly connected, ens256, 00:40:48 Configurations on R1: ===================== ! router bgp 201 bgp router-id 192.168.0.1 neighbor 192.201.202.2 remote-as 201 ! Configurations on R2: ===================== ! ip route 202.202.202.202/32 192.201.78.1 ! router bgp 201 bgp router-id 192.168.0.2 neighbor 192.201.202.1 remote-as 201 ! address-family ipv4 unicast redistribute static exit-address-family ! Step-1: ======= R1 receives the route 202.202.202.202/32 from R2. R1 installs the route in its BGP RIB. Step-2: ======= On R1, a connected interface address is added. The address is the same as the next-hop of the BGP route received from R2 (192.201.78.1). Point of Failure: ================= R1 resolves the BGP route even though the route's next-hop is its own connected address. Even though this appears to be a misconfiguration it would still be better to safeguard the code against it. Fix: ==== When BGP receives a connected route from Zebra, it processes the routes for the next-hop update. While doing so, BGP must ignore routes whose next-hop address matches the address of the connected route for which Zebra sent the next-hop update message. Signed-off-by: NaveenThanikachalam nthanikachal@vmware.com --- bgpd/bgp_nht.c | 13 +++++++++++-- bgpd/bgp_route.c | 6 +++--- bgpd/bgp_route.h | 3 +++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 0531542a38f0..baf4e5796166 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -745,8 +745,17 @@ static void evaluate_paths(struct bgp_nexthop_cache *bnc) bnc_is_valid_nexthop = bgp_isvalid_labeled_nexthop(bnc) ? 1 : 0; } else { - bnc_is_valid_nexthop = - bgp_isvalid_nexthop(bnc) ? 1 : 0; + if (bgp_update_martian_nexthop( + bnc->bgp, afi, safi, path->type, + path->sub_type, path->attr, rn)) { + if (BGP_DEBUG(nht, NHT)) + zlog_debug( + "%s: Prefix %pRN (vrf %s), ignoring path due to martian or self-next-hop", + __func__, rn, bgp_path->name); + } else { + bnc_is_valid_nexthop = + bgp_isvalid_nexthop(bnc) ? 1 : 0; + } } if (BGP_DEBUG(nht, NHT)) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index a11e1d7c693c..95b18f4e6dbb 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3226,9 +3226,9 @@ static bool overlay_index_equal(afi_t afi, struct bgp_path_info *path, } /* Check if received nexthop is valid or not. */ -static bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, - uint8_t type, uint8_t stype, - struct attr *attr, struct bgp_node *rn) +bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, + uint8_t type, uint8_t stype, struct attr *attr, + struct bgp_node *rn) { bool ret = 0; diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index ad08bbf44024..7532dc123aec 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -677,4 +677,7 @@ extern int bgp_show_table_rd(struct vty *vty, struct bgp *bgp, safi_t safi, enum bgp_show_type type, void *output_arg, bool use_json); extern int bgp_best_path_select_defer(struct bgp *bgp, afi_t afi, safi_t safi); +extern bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, + uint8_t type, uint8_t stype, + struct attr *attr, struct bgp_node *rn); #endif /* _QUAGGA_BGP_ROUTE_H */