From: nikos <nikos@apstra.com>
Date: Sat, 4 May 2019 06:22:30 +0000 (-0700)
Subject: bgpd: IPv6 session flapping with MP_REACH_NLRI and 0.0.0.0 in NEXT_HOP attribute
X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=9378dfd25f2311e9754f4171f513c06a3b770846;p=matthieu%2Ffrr.git

bgpd: IPv6 session flapping with MP_REACH_NLRI and 0.0.0.0 in NEXT_HOP attribute

This is causing interop issues with vendors. According to the RFC,
receiver should ignore the NEXT_HOP attribute with MP_REACH_NLRI
present.

Signed-off-by: nikos ntriantafillis@gmail.com
---

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 05e103142b..a662b8c35d 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1264,8 +1264,6 @@ static bgp_attr_parse_ret_t bgp_attr_nexthop(struct bgp_attr_parser_args *args)
 	struct attr *const attr = args->attr;
 	const bgp_size_t length = args->length;
 
-	in_addr_t nexthop_h, nexthop_n;
-
 	/* Check nexthop attribute length. */
 	if (length != 4) {
 		flog_err(EC_BGP_ATTR_LEN,
@@ -1275,30 +1273,7 @@ static bgp_attr_parse_ret_t bgp_attr_nexthop(struct bgp_attr_parser_args *args)
 					  args->total);
 	}
 
-	/* According to section 6.3 of RFC4271, syntactically incorrect NEXT_HOP
-	   attribute must result in a NOTIFICATION message (this is implemented
-	   below).
-	   At the same time, semantically incorrect NEXT_HOP is more likely to
-	   be just
-	   logged locally (this is implemented somewhere else). The UPDATE
-	   message
-	   gets ignored in any of these cases. */
-	nexthop_n = stream_get_ipv4(peer->curr);
-	nexthop_h = ntohl(nexthop_n);
-	if ((IPV4_NET0(nexthop_h) || IPV4_NET127(nexthop_h)
-	     || IPV4_CLASS_DE(nexthop_h))
-	    && !BGP_DEBUG(
-		       allow_martians,
-		       ALLOW_MARTIANS)) /* loopbacks may be used in testing */
-	{
-		char buf[INET_ADDRSTRLEN];
-		inet_ntop(AF_INET, &nexthop_n, buf, INET_ADDRSTRLEN);
-		flog_err(EC_BGP_ATTR_MARTIAN_NH, "Martian nexthop %s", buf);
-		return bgp_attr_malformed(
-			args, BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP, args->total);
-	}
-
-	attr->nexthop.s_addr = nexthop_n;
+	attr->nexthop.s_addr = stream_get_ipv4(peer->curr);
 	attr->flag |= ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP);
 
 	return BGP_ATTR_PARSE_PROCEED;
@@ -2669,6 +2644,40 @@ bgp_attr_parse_ret_t bgp_attr_parse(struct peer *peer, struct attr *attr,
 		return BGP_ATTR_PARSE_ERROR;
 	}
 
+	/*
+	 * RFC4271: If the NEXT_HOP attribute field is syntactically incorrect,
+	 * then the Error Subcode MUST be set to Invalid NEXT_HOP Attribute.
+	 * This is implemented below and will result in a NOTIFICATION. If the
+	 * NEXT_HOP attribute is semantically incorrect, the error SHOULD be
+	 * logged, and the route SHOULD be ignored. In this case, a NOTIFICATION
+	 * message SHOULD NOT be sent. This is implemented elsewhere.
+	 *
+	 * RFC4760: An UPDATE message that carries no NLRI, other than the one
+	 * encoded in the MP_REACH_NLRI attribute, SHOULD NOT carry the NEXT_HOP
+	 * attribute. If such a message contains the NEXT_HOP attribute, the BGP
+	 * speaker that receives the message SHOULD ignore this attribute.
+	 */
+	if (CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_NEXT_HOP))
+	    && !CHECK_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_MP_REACH_NLRI))) {
+
+		in_addr_t nexthop_h;
+
+		nexthop_h = ntohl(attr->nexthop.s_addr);
+		if ((IPV4_NET0(nexthop_h) || IPV4_NET127(nexthop_h)
+		     || IPV4_CLASS_DE(nexthop_h))
+		    && !BGP_DEBUG(allow_martians, ALLOW_MARTIANS)) {
+			char buf[INET_ADDRSTRLEN];
+
+			inet_ntop(AF_INET, &attr->nexthop.s_addr, buf,
+				  INET_ADDRSTRLEN);
+			flog_err(EC_BGP_ATTR_MARTIAN_NH, "Martian nexthop %s",
+				 buf);
+			bgp_notify_send(peer, BGP_NOTIFY_UPDATE_ERR,
+					BGP_NOTIFY_UPDATE_INVAL_NEXT_HOP);
+			return BGP_ATTR_PARSE_ERROR;
+		}
+	}
+
 	/* Check all mandatory well-known attributes are present */
 	if ((ret = bgp_attr_check(peer, attr)) < 0) {
 		if (as4_path)