[quagga-dev 11543] [PATCH] bgpd: well-known attr check only run for v4/uni, which could cause a crash.

Paul Jakma paul at opensourcerouting.org
Wed Oct 1 09:41:44 BST 2014


* ANVL testing by Martin Winter threw up a crash in bgpd in aspath_dup
  called from bgp_packet_attribute, if attr->aspath was NULL, on an IPv6
  UPDATE.

  This root cause is that the checks for well-known, mandatory attributes
  were being applied only if an UPDATE contained the IPv4 NLRI and the
  peer was configured for v4/unicast (i.e. not deconfigured). This is
  something inherited from GNU Zebra, and never noticed before.

* bgp_attr.c: (bgp_attr_parse) Move the well-known mandatory attribute
  check to here, so that it can be run immediately after all attributes
  are parsed, and before any further processing of attributes that might
  assume the existence of WK/M attributes (e.g. AS4-Path).
  (bgp_attr_munge_as4_attrs) Missing AS_PATH shouldn't happen here anymore,
  but retain a check anyway for robustness - it's definitely a hard error
  though.
* bgp_attr.h: (bgp_attr_check) No longer needs to be exported, make static.
* bgp_packet.c: (bgp_update_receive) Responsibility for well-known check
  now in bgp_attr_parse.
---
 bgpd/bgp_attr.c   | 101 +++++++++++++++++++++++++++++++-----------------------
 bgpd/bgp_attr.h   |   1 -
 bgpd/bgp_packet.c |  11 ------
 3 files changed, 59 insertions(+), 54 deletions(-)

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index b62a4f8..69ca786 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1282,7 +1282,17 @@ bgp_attr_munge_as4_attrs (struct peer *const peer,
   int ignore_as4_path = 0;
   struct aspath *newpath;
   struct attr_extra *attre = attr->extra;
-    
+  
+  if (!attr->aspath)
+    {
+      /* NULL aspath shouldn't be possible as bgp_attr_parse should have
+       * checked that all well-known, mandatory attributes were present.
+       * 
+       * Can only be a problem with peer itself - hard error
+       */
+      return BGP_ATTR_PARSE_ERROR;
+    }
+  
   if (CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV))
     {
       /* peer can do AS4, so we ignore AS4_PATH and AS4_AGGREGATOR
@@ -1362,12 +1372,9 @@ bgp_attr_munge_as4_attrs (struct peer *const peer,
   /* need to reconcile NEW_AS_PATH and AS_PATH */
   if (!ignore_as4_path && (attr->flag & (ATTR_FLAG_BIT( BGP_ATTR_AS4_PATH))))
     {
-       if (!attr->aspath)
-         return BGP_ATTR_PARSE_PROCEED;
-
-       newpath = aspath_reconcile_as4 (attr->aspath, as4_path);
-       aspath_unintern (&attr->aspath);
-       attr->aspath = aspath_intern (newpath);
+      newpath = aspath_reconcile_as4 (attr->aspath, as4_path);
+      aspath_unintern (&attr->aspath);
+      attr->aspath = aspath_intern (newpath);
     }
   return BGP_ATTR_PARSE_PROCEED;
 }
@@ -1726,6 +1733,39 @@ bgp_attr_unknown (struct bgp_attr_parser_args *args)
   return BGP_ATTR_PARSE_PROCEED;
 }
 
+/* Well-known attribute check. */
+static int
+bgp_attr_check (struct peer *peer, struct attr *attr)
+{
+  u_char type = 0;
+  
+  if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_ORIGIN)))
+    type = BGP_ATTR_ORIGIN;
+
+  if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_AS_PATH)))
+    type = BGP_ATTR_AS_PATH;
+
+  if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP)))
+    type = BGP_ATTR_NEXT_HOP;
+
+  if (peer->sort == BGP_PEER_IBGP
+      && ! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF)))
+    type = BGP_ATTR_LOCAL_PREF;
+
+  if (type)
+    {
+      zlog (peer->log, LOG_WARNING, 
+	    "%s Missing well-known attribute %d.",
+	    peer->host, type);
+      bgp_notify_send_with_data (peer, 
+				 BGP_NOTIFY_UPDATE_ERR, 
+				 BGP_NOTIFY_UPDATE_MISS_ATTR,
+				 &type, 1);
+      return BGP_ATTR_PARSE_ERROR;
+    }
+  return BGP_ATTR_PARSE_PROCEED;
+}
+
 /* Read attribute of update packet.  This function is called from
    bgp_update_receive() in bgp_packet.c.  */
 bgp_attr_parse_ret_t
@@ -1958,7 +1998,6 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
 	  return BGP_ATTR_PARSE_ERROR;
 	}
     }
-
   /* Check final read pointer is same as end pointer. */
   if (BGP_INPUT_PNT (peer) != endp)
     {
@@ -1972,7 +2011,18 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
         aspath_unintern (&as4_path);
       return BGP_ATTR_PARSE_ERROR;
     }
-
+  
+  /* Check all mandatory well-known attributes are present */
+  {
+    bgp_attr_parse_ret_t ret;
+    if ((ret = bgp_attr_check (peer, attr)) < 0)
+      {
+        if (as4_path)
+          aspath_unintern (&as4_path);
+        return ret;
+      }
+  }
+  
   /* 
    * At this place we can see whether we got AS4_PATH and/or
    * AS4_AGGREGATOR from a 16Bit peer and act accordingly.
@@ -2033,39 +2083,6 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
   return BGP_ATTR_PARSE_PROCEED;
 }
 
-/* Well-known attribute check. */
-int
-bgp_attr_check (struct peer *peer, struct attr *attr)
-{
-  u_char type = 0;
-  
-  if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_ORIGIN)))
-    type = BGP_ATTR_ORIGIN;
-
-  if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_AS_PATH)))
-    type = BGP_ATTR_AS_PATH;
-
-  if (! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_NEXT_HOP)))
-    type = BGP_ATTR_NEXT_HOP;
-
-  if (peer->sort == BGP_PEER_IBGP
-      && ! CHECK_FLAG (attr->flag, ATTR_FLAG_BIT (BGP_ATTR_LOCAL_PREF)))
-    type = BGP_ATTR_LOCAL_PREF;
-
-  if (type)
-    {
-      zlog (peer->log, LOG_WARNING, 
-	    "%s Missing well-known attribute %d.",
-	    peer->host, type);
-      bgp_notify_send_with_data (peer, 
-				 BGP_NOTIFY_UPDATE_ERR, 
-				 BGP_NOTIFY_UPDATE_MISS_ATTR,
-				 &type, 1);
-      return BGP_ATTR_PARSE_ERROR;
-    }
-  return BGP_ATTR_PARSE_PROCEED;
-}
-
 int stream_put_prefix (struct stream *, struct prefix *);
 
 size_t
diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index cb401e7..b59fa8e 100644
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -147,7 +147,6 @@ extern void bgp_attr_finish (void);
 extern bgp_attr_parse_ret_t bgp_attr_parse (struct peer *, struct attr *,
                                            bgp_size_t, struct bgp_nlri *,
                                            struct bgp_nlri *);
-extern int bgp_attr_check (struct peer *, struct attr *);
 extern struct attr_extra *bgp_attr_extra_get (struct attr *);
 extern void bgp_attr_extra_free (struct attr *);
 extern void bgp_attr_dup (struct attr *, struct attr *);
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 0fab1b0..35a22c1 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -1775,18 +1775,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
 	bgp_nlri_parse (peer, NULL, &withdraw);
 
       if (update.length)
-	{
-	  /* We check well-known attribute only for IPv4 unicast
-	     update. */
-	  ret = bgp_attr_check (peer, &attr);
-	  if (ret < 0)
-	    {
-	      bgp_attr_unintern_sub (&attr);
-	      return -1;
-            }
-
 	  bgp_nlri_parse (peer, NLRI_ATTR_ARG, &update);
-	}
 
       if (mp_update.length
 	  && mp_update.afi == AFI_IP 
-- 
1.7.11.7





More information about the Quagga-dev mailing list