Browse Source

bgpd: Remove the double-pass parsing of NLRIs

* bgpd parses NLRIs twice, a first pass "sanity check" and then a second pass
  that changes actual state. For most AFI/SAFIs this is done by
  bgp_nlri_sanity_check and bgp_nlri_parse, which are almost identical.

  As the required action on a syntactic error in an NLRI is to NOTIFY and
  shut down the session, it should be acceptable to just do a one pass
  parse.  There is no need to atomically handle the NLRIs.

* bgp_route.h: (bgp_nlri_sanity_check) Delete
* bgp_route.c: (bgp_nlri_parse) Make the prefixlen size check more general
  and don't hard-code AFI/SAFI details, e.g. use prefix_blen library function.

  Add error logs consistent with bgp_nlri_sanity_check as much as possible.

  Add a "defense in depth" type check of the prefixlen against the sizeof
  the (struct prefix) storage - ala bgp_nlri_parse_vpn.
  Update standards text from draft RFC4271 to the actual RFC4271 text.

  Extend the semantic consistency test of IPv6. E.g. it should skip mcast
  NLRIs for unicast safi as v4 does.

* bgp_mplsvpn.{c,h}: Delete bgp_nlri_sanity_check_vpn and make
  bgp_nlri_parse_vpn_body the bgp_nlri_parse_vpn function again.

  (bgp_nlri_parse_vpn) Remove the notifies.  The sanity checks were
  responsible for this, but bgp_update_receive handles sending NOTIFY
  generically for bgp_nlri_parse.

* bgp_attr.c: (bgp_mp_reach_parse,bgp_mp_unreach_parse) Delete sanity check.
  NLRI parsing done after attr parsing by bgp_update_receive.

Arising out of discussions on the need for two-pass NLRI parse with:

Lou Berger <lberger@labn.net>
Donald Sharp <sharpd@cumulusnetworks.com>
Paul Jakma 3 years ago
parent
commit
405e9e19eb
6 changed files with 65 additions and 172 deletions
  1. 0 12
      bgpd/bgp_attr.c
  2. 9 35
      bgpd/bgp_mplsvpn.c
  3. 0 1
      bgpd/bgp_mplsvpn.h
  4. 0 18
      bgpd/bgp_packet.c
  5. 56 105
      bgpd/bgp_route.c
  6. 0 1
      bgpd/bgp_route.h

+ 0 - 12
bgpd/bgp_attr.c

@@ -1602,7 +1602,6 @@ bgp_mp_reach_parse (struct bgp_attr_parser_args *args,
   safi_t safi;
   bgp_size_t nlri_len;
   size_t start;
-  int ret;
   struct stream *s;
   struct peer *const peer = args->peer;  
   struct attr *const attr = args->attr;
@@ -1731,14 +1730,6 @@ bgp_mp_reach_parse (struct bgp_attr_parser_args *args,
   mp_update->nlri = stream_pnt (s);
   mp_update->length = nlri_len;
 
-  ret = bgp_nlri_sanity_check (peer, mp_update);
-  if (ret < 0) 
-    {
-      zlog_info ("%s: (%s) NLRI doesn't pass sanity check",
-                 __func__, peer->host);
-      return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
-    }
-
   stream_forward_getp (s, nlri_len);
 
   attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MP_REACH_NLRI);
@@ -1776,9 +1767,6 @@ bgp_mp_unreach_parse (struct bgp_attr_parser_args *args,
   mp_withdraw->nlri = stream_pnt (s);
   mp_withdraw->length = withdraw_len;
 
-  if (bgp_nlri_sanity_check (peer, mp_withdraw) < 0)
-    return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
-
   stream_forward_getp (s, withdraw_len);
 
   attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MP_UNREACH_NLRI);

+ 9 - 35
bgpd/bgp_mplsvpn.c

@@ -93,9 +93,9 @@ decode_rd_ip (u_char *pnt, struct rd_ip *rd_ip)
   rd_ip->val |= (u_int16_t) *pnt;
 }
 
-static int
-bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr, 
-                         struct bgp_nlri *packet, bool update)
+int
+bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr, 
+                    struct bgp_nlri *packet)
 {
   u_char *pnt;
   u_char *lim;
@@ -137,8 +137,6 @@ bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
                     "%s [Error] Update packet error / VPNv4"
                      " (prefix length %d less than VPNv4 min length)",
                     peer->host, prefixlen);
-          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
           return -1;
         }
       if ((pnt + psize) > lim)
@@ -148,8 +146,6 @@ bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
                     " (psize %u exceeds packet size (%u)",
                     peer->host, 
                     prefixlen, (uint)(lim-pnt));
-          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
           return -1;
         }
       
@@ -161,8 +157,6 @@ bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
                     " (psize %u exceeds storage size (%zu)",
                     peer->host,
                     prefixlen - VPN_PREFIXLEN_MIN_BYTES*8, sizeof(p.u));
-          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
           return -1;
         }
       
@@ -175,8 +169,6 @@ bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
                     peer->host,
                     prefixlen - VPN_PREFIXLEN_MIN_BYTES*8, 
                     p.family, prefix_blen (&p));
-          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-                           BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
           return -1;
         }
       
@@ -212,15 +204,12 @@ bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
       memcpy (&p.u.prefix, pnt + VPN_PREFIXLEN_MIN_BYTES, 
               psize - VPN_PREFIXLEN_MIN_BYTES);
 
-      if (update)
-        {
-          if (attr)
-            bgp_update (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
-                        ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0);
-          else
-            bgp_withdraw (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
-                          ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt);
-        }
+      if (attr)
+        bgp_update (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
+                    ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0);
+      else
+        bgp_withdraw (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
+                      ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt);
     }
   /* Packet length consistency check. */
   if (pnt != lim)
@@ -229,8 +218,6 @@ bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
                 "%s [Error] Update packet error / VPNv4"
                 " (%zu data remaining after parsing)",
                 peer->host, lim - pnt);
-      bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-                       BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
       return -1;
     }
   
@@ -239,19 +226,6 @@ bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
 }
 
 int
-bgp_nlri_sanity_check_vpn (struct peer *peer, struct bgp_nlri *nlri)
-{
-  return bgp_nlri_parse_vpn_body (peer, NULL, nlri, false);
-}
-
-int
-bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr, 
-                    struct bgp_nlri *packet)
-{
-  return bgp_nlri_parse_vpn_body (peer, attr, packet, true);
-}
-
-int
 str2prefix_rd (const char *str, struct prefix_rd *prd)
 {
   int ret; /* ret of called functions */

+ 0 - 1
bgpd/bgp_mplsvpn.h

@@ -43,7 +43,6 @@ struct rd_ip
 
 extern void bgp_mplsvpn_init (void);
 extern int bgp_nlri_parse_vpn (struct peer *, struct attr *, struct bgp_nlri *);
-extern int bgp_nlri_sanity_check_vpn (struct peer *, struct bgp_nlri *);
 extern u_int32_t decode_label (u_char *);
 extern int str2prefix_rd (const char *, struct prefix_rd *);
 extern int str2tag (const char *, u_char *);

+ 0 - 18
bgpd/bgp_packet.c

@@ -1686,13 +1686,6 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
       nlris[NLRI_WITHDRAW].nlri = stream_pnt (s);
       nlris[NLRI_WITHDRAW].length = withdraw_len;
       
-      if (bgp_nlri_sanity_check (peer, &nlris[NLRI_WITHDRAW]) < 0)
-        {
-          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
-                                 BGP_NOTIFY_UPDATE_INVAL_NETWORK);
-          return -1;
-        }
-      
       if (BGP_DEBUG (packet, PACKET_RECV))
 	zlog_debug ("%s [Update:RECV] Unfeasible NLRI received", peer->host);
 
@@ -1782,17 +1775,6 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
       nlris[NLRI_UPDATE].nlri = stream_pnt (s);
       nlris[NLRI_UPDATE].length = update_len;
       
-      /* Check NLRI packet format and prefix length. */
-      ret = bgp_nlri_sanity_check (peer, &nlris[NLRI_UPDATE]);
-      if (ret < 0)
-        {
-          bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
-                           BGP_NOTIFY_UPDATE_INVAL_NETWORK);
-          bgp_attr_unintern_sub (&attr);
-          bgp_attr_flush (&attr);
-	  return -1;
-	}
-
       stream_forward_getp (s, update_len);
     }
   

+ 56 - 105
bgpd/bgp_route.c

@@ -3242,6 +3242,9 @@ bgp_nlri_parse_ip (struct peer *peer, struct attr *attr,
   pnt = packet->nlri;
   lim = pnt + packet->length;
 
+  /* RFC4771 6.3 The NLRI field in the UPDATE message is checked for
+     syntactic validity.  If the field is syntactically incorrect,
+     then the Error Subcode is set to Invalid Network Field. */
   for (; pnt < lim; pnt += psize)
     {
       /* Clear prefix structure. */
@@ -3249,20 +3252,41 @@ bgp_nlri_parse_ip (struct peer *peer, struct attr *attr,
 
       /* Fetch prefix length. */
       p.prefixlen = *pnt++;
+      /* afi/safi validity already verified by caller, bgp_update_receive */
       p.family = afi2family (packet->afi);
       
-      /* Already checked in nlri_sanity_check().  We do double check
-         here. */
-      if ((packet->afi == AFI_IP && p.prefixlen > 32)
-	  || (packet->afi == AFI_IP6 && p.prefixlen > 128))
-	return -1;
-
+      /* Prefix length check. */
+      if (p.prefixlen > prefix_blen (&p) * 8)
+        {
+          plog_err (peer->log,
+                    "%s [Error] Update packet error"
+                    " (wrong prefix length %u for afi %u)",
+                    peer->host, p.prefixlen, packet->afi);
+          return -1;
+        }
+      
       /* Packet size overflow check. */
       psize = PSIZE (p.prefixlen);
 
       /* When packet overflow occur return immediately. */
       if (pnt + psize > lim)
-	return -1;
+        {
+          plog_err (peer->log,
+                    "%s [Error] Update packet error"
+                    " (prefix length %u overflows packet)",
+                    peer->host, p.prefixlen);
+          return -1;
+        }
+      
+      /* Defensive coding, double-check the psize fits in a struct prefix */  
+      if (psize > (ssize_t) sizeof(p.u))
+        {
+          plog_err (peer->log,
+                    "%s [Error] Update packet error"
+                    " (prefix length %u too large for prefix storage %zu!?!!",
+                    peer->host, p.prefixlen, sizeof(p.u));
+          return -1;
+        }
 
       /* Fetch prefix from NLRI packet. */
       memcpy (&p.u.prefix, pnt, psize);
@@ -3273,17 +3297,16 @@ bgp_nlri_parse_ip (struct peer *peer, struct attr *attr,
 	  if (IN_CLASSD (ntohl (p.u.prefix4.s_addr)))
 	    {
 	     /* 
- 	      * From draft-ietf-idr-bgp4-22, Section 6.3: 
-	      * If a BGP router receives an UPDATE message with a
-	      * semantically incorrect NLRI field, in which a prefix is
-	      * semantically incorrect (eg. an unexpected multicast IP
-	      * address), it should ignore the prefix.
+ 	      * From RFC4271 Section 6.3: 
+	      * 
+	      * If a prefix in the NLRI field is semantically incorrect
+	      * (e.g., an unexpected multicast IP address), an error SHOULD
+	      * be logged locally, and the prefix SHOULD be ignored.
 	      */
 	      zlog (peer->log, LOG_ERR, 
-		    "IPv4 unicast NLRI is multicast address %s",
-		    inet_ntoa (p.u.prefix4));
-
-	      return -1;
+		    "%s: IPv4 unicast NLRI is multicast address %s, ignoring",
+		    peer->host, inet_ntoa (p.u.prefix4));
+	      continue;
 	    }
 	}
 
@@ -3294,13 +3317,23 @@ bgp_nlri_parse_ip (struct peer *peer, struct attr *attr,
 	    {
 	      char buf[BUFSIZ];
 
-	      zlog (peer->log, LOG_WARNING, 
-		    "IPv6 link-local NLRI received %s ignore this NLRI",
+	      zlog (peer->log, LOG_ERR, 
+		    "%s: IPv6 unicast NLRI is link-local address %s, ignoring",
+		    peer->host,
 		    inet_ntop (AF_INET6, &p.u.prefix6, buf, BUFSIZ));
+	      continue;
+	    }
+	  if (IN6_IS_ADDR_MULTICAST (&p.u.prefix6))
+	    {
+	      char buf[BUFSIZ];
 
+	      zlog (peer->log, LOG_ERR, 
+		    "%s: IPv6 unicast NLRI is multicast address %s, ignoring",
+		    peer->host,
+		    inet_ntop (AF_INET6, &p.u.prefix6, buf, BUFSIZ));
 	      continue;
 	    }
-	}
+        }
 
       /* Normal process. */
       if (attr)
@@ -3318,99 +3351,17 @@ bgp_nlri_parse_ip (struct peer *peer, struct attr *attr,
 
   /* Packet length consistency check. */
   if (pnt != lim)
-    return -1;
-
-  return 0;
-}
-
-static int
-bgp_nlri_sanity_check_ip (struct peer *peer, struct bgp_nlri *nlri)
-{
-  u_char *end;
-  u_char prefixlen;
-  int psize;
-  u_char *pnt = nlri->nlri;
-  afi_t afi = nlri->afi;
-  safi_t safi = nlri->safi;
-  end = pnt + nlri->length;
-
-  /* RFC1771 6.3 The NLRI field in the UPDATE message is checked for
-     syntactic validity.  If the field is syntactically incorrect,
-     then the Error Subcode is set to Invalid Network Field. */
-
-  while (pnt < end)
-    {
-      int	badlength;
-      prefixlen = *pnt++;
-      
-      /* Prefix length check. */
-      badlength = 0;
-      if (safi == SAFI_ENCAP) {
-	if (prefixlen > 128)
-	  badlength = 1;
-      } else {
-        if ((afi == AFI_IP && prefixlen > 32) ||
-	    (afi == AFI_IP6 && prefixlen > 128)) {
-
-	    badlength = 1;
-	}
-      }
-      if (badlength)
-	{
-	  plog_err (peer->log, 
-		    "%s [Error] Update packet error (wrong prefix length %d)",
-		    peer->host, prefixlen);
-	  bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-			   BGP_NOTIFY_UPDATE_INVAL_NETWORK);
-	  return -1;
-	}
-
-      /* Packet size overflow check. */
-      psize = PSIZE (prefixlen);
-
-      if (pnt + psize > end)
-	{
-	  plog_err (peer->log, 
-		    "%s [Error] Update packet error"
-		    " (prefix data overflow prefix size is %d)",
-		    peer->host, psize);
-	  bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-			   BGP_NOTIFY_UPDATE_INVAL_NETWORK);
-	  return -1;
-	}
-
-      pnt += psize;
-    }
-
-  /* Packet length consistency check. */
-  if (pnt != end)
     {
       plog_err (peer->log,
-		"%s [Error] Update packet error"
-		" (prefix length mismatch with total length)",
-		peer->host);
-      bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR, 
-		       BGP_NOTIFY_UPDATE_INVAL_NETWORK);
+                "%s [Error] Update packet error"
+                " (prefix length mismatch with total length)",
+                peer->host);
       return -1;
     }
+  
   return 0;
 }
 
-int
-bgp_nlri_sanity_check (struct peer *peer, struct bgp_nlri *nlri)
-{
-   switch (nlri->safi)
-     {
-       case SAFI_MPLS_LABELED_VPN:
-       case SAFI_MPLS_VPN:
-         return bgp_nlri_sanity_check_vpn (peer, nlri);
-       case SAFI_UNICAST:
-       case SAFI_MULTICAST:
-         return bgp_nlri_sanity_check_ip (peer, nlri);
-       default: return -1;
-     }
-}
-
 static struct bgp_static *
 bgp_static_new (void)
 {

+ 0 - 1
bgpd/bgp_route.h

@@ -196,7 +196,6 @@ extern struct bgp_info_extra *bgp_info_extra_get (struct bgp_info *);
 extern void bgp_info_set_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
 extern void bgp_info_unset_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
 
-extern int bgp_nlri_sanity_check (struct peer *, struct bgp_nlri *);
 extern int bgp_nlri_parse_ip (struct peer *, struct attr *, struct bgp_nlri *);
 
 extern int bgp_maximum_prefix_overflow (struct peer *, afi_t, safi_t, int);