bgpd: well-known attr check only run for v4/uni, which could cause a crash.

* 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.

(cherry picked from commit 055086f70febc30fdfd94bb4406e9075d6934cd8)

Conflicts:
	bgpd/bgp_attr.c
	bgpd/bgp_attr.h
	bgpd/bgp_packet.c

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index c2c86e51a29e3b6d6b08a71eb88cff39a0c01074..646d16200f7eb1ea152f23a11f76267c83d6198c 100644 (file)
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1359,7 +1359,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
@@ -1439,12 +1449,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;
 }
@@ -1808,11 +1815,48 @@ 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, bgp_size_t nlri_len)
+{
+  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;
+
+  /* As per RFC 2858, NEXT_HOP is needed only if the update contains NLRI
+   * other than in MP_REACH. In fact, if only MP_REACH_NLRI is present, the
+   * update should not contain NEXT_HOP but it should be ignored, if recvd.
+   */
+  if (nlri_len)
+    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_warn ("%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
 bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
-               struct bgp_nlri *mp_update, struct bgp_nlri *mp_withdraw)
+               struct bgp_nlri *mp_update, struct bgp_nlri *mp_withdraw,
+                bgp_size_t nlri_len)
 {
   int ret;
   u_char flag = 0;
@@ -2048,6 +2092,17 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
       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, nlri_len)) < 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.
@@ -2108,42 +2163,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, bgp_size_t nlri_len)
-{
-  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;
-
-  /* As per RFC 2858, NEXT_HOP is needed only if the update contains NLRI
-   * other than in MP_REACH. In fact, if only MP_REACH_NLRI is present, the
-   * update should not contain NEXT_HOP but it should be ignored, if recvd.
-   */
-  if (nlri_len)
-    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_warn ("%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;
-}
-
 size_t
 bgp_packet_mpattr_start (struct stream *s, afi_t afi, safi_t safi, afi_t nh_afi,
                         struct bpacket_attr_vec_arr *vecarr,

diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index 5a5492aad4f901304f0b7a2a089dbe56d17d08a9..a9e8b4f5d6baa7ec0247b4955f7a130b119d3a1c 100644 (file)
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -171,8 +171,7 @@ extern void bgp_attr_init (void);
 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 *, bgp_size_t);
+                                           struct bgp_nlri *, bgp_size_t);
 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 025cfbaeebb2a9582bc6c5c55ff5bbe65072c1b0..e2b8a3346afd8a468ec8c4f6691553e55911bed1 100644 (file)
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -1430,7 +1430,7 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
   if (attribute_len)
     {
       attr_parse_ret = bgp_attr_parse (peer, &attr, attribute_len, 
-                           &mp_update, &mp_withdraw);
+                           &mp_update, &mp_withdraw, update.length);
       if (attr_parse_ret == BGP_ATTR_PARSE_ERROR)
        {
          bgp_attr_unintern_sub (&attr);
@@ -1483,22 +1483,6 @@ bgp_update_receive (struct peer *peer, bgp_size_t size)
                peer->host, withdraw_len, num_pfx_wd, attribute_len,
                update_len, num_pfx_adv);
 
-  /*
-   * Validate for well-known mandatory attributes. We only do this if
-   * we intend to process the update further - i.e., the AFI/SAFI is
-   * enabled.
-   */
-  if ((update.length && peer->afc[AFI_IP][SAFI_UNICAST]) ||
-      (mp_update.length && peer->afc[mp_update.afi][mp_update.safi]))
-    {
-      ret = bgp_attr_check (peer, &attr, update.length);
-      if (ret < 0)
-        {
-          bgp_attr_unintern_sub (&attr);
-          return -1;
-        }
-    }
-
   /* NLRI is processed only when the the corresponding address-family
    * has been negotiated with the peer.
    */