]> git.proxmox.com Git - mirror_frr.git/commitdiff
bgpd: Rollback some of the changes made for invalid AS_PATH segment fix
authorPaul Jakma <paul@quagga.net>
Sat, 27 Nov 2010 22:48:34 +0000 (22:48 +0000)
committerPaul Jakma <paul@quagga.net>
Mon, 21 Mar 2011 13:51:14 +0000 (13:51 +0000)
Some of the changes made in commit cddb8112b80fa9867156c637d63e6e79eeac67bb
don't work particularly well for other changes that need to be made to
address BGP attribute error handling problems. In particular, returning
a pointer from complex attribute data parsing functions will not suffice
to express the require range of return status conditions.

* bgp_aspath.c: (assegments_parse) Rollback to a more minimal set of
  changes to fix the original problem.
  (aspath_parse) Slightly needless pushing around of code, and taking
  2 parameters to say whether ot use 2 or 4 byte encoding seems unnecessary.
* bgp_attr.c: (bgp_attr_as{,4}path) Rollback, in preparation for BGP
  attribute error handling update.

bgpd/bgp_aspath.c
bgpd/bgp_aspath.h
bgpd/bgp_attr.c

index 5a73eeffae8650d503d36a3c3af227d9fdd7ba7c..89a27531851c0a76ccac0a8d0361ae3ea4728b53 100644 (file)
@@ -671,78 +671,77 @@ aspath_hash_alloc (void *arg)
   return aspath;
 }
 
-/* parse as-segment byte stream in struct assegment
- *
- * Returns NULL if the AS_PATH or AS4_PATH is not valid.
- */
+/* parse as-segment byte stream in struct assegment */
 static struct assegment *
-assegments_parse (struct stream *s, size_t length, int use32bit, int as4_path)
+assegments_parse (struct stream *s, size_t length, int use32bit)
 {
   struct assegment_header segh;
   struct assegment *seg, *prev = NULL, *head = NULL;
+  size_t bytes = 0;
   
-  assert (length > 0);  /* does not expect empty AS_PATH or AS4_PATH    */
+  /* empty aspath (ie iBGP or somesuch) */
+  if (length == 0)
+    return NULL;
   
   if (BGP_DEBUG (as4, AS4_SEGMENT))
     zlog_debug ("[AS4SEG] Parse aspath segment: got total byte length %lu",
                (unsigned long) length);
-
-  /* double check that length does not exceed stream    */
-  if (STREAM_READABLE(s) < length)
+  /* basic checks */
+  if ((STREAM_READABLE(s) < length)
+      || (STREAM_READABLE(s) < AS_HEADER_SIZE) 
+      || (length % AS16_VALUE_SIZE ))
     return NULL;
   
-  /* deal with each segment in turn                             */
-  while (length > 0)
+  while (bytes < length)
     {
       int i;
       size_t seg_size;
       
-      /* softly softly, get the header first on its own */
-      if (length >= AS_HEADER_SIZE)
+      if ((length - bytes) <= AS_HEADER_SIZE)
         {
+          if (head)
+            assegment_free_all (head);
+          return NULL;
+        }
+      
+      /* softly softly, get the header first on its own */
       segh.type = stream_getc (s);
       segh.length = stream_getc (s);
       
       seg_size = ASSEGMENT_SIZE(segh.length, use32bit);
-                                      /* includes the header bytes */
 
       if (BGP_DEBUG (as4, AS4_SEGMENT))
        zlog_debug ("[AS4SEG] Parse aspath segment: got type %d, length %d",
                     segh.type, segh.length);
       
-          switch (segh.type)
-          {
-            case AS_SEQUENCE:
-            case AS_SET:
-              break ;
-
-            case AS_CONFED_SEQUENCE:
-            case AS_CONFED_SET:
-              if (!as4_path)
-                break ;
-              /* RFC4893 3: "invalid for the AS4_PATH attribute"            */
-              /* fall through */
-
-            default:    /* reject unknown or invalid AS_PATH segment types  */
-              seg_size = 0 ;
-          } ;
-        }
-      else
-        seg_size = 0 ;
-
-     /* Stop now if segment is not valid (discarding anything collected to date)
-      *
-      * RFC4271 4.3, Path Attributes, b) AS_PATH:
-      *
-      *   "path segment value field contains one or more AS numbers"
+      /* check it.. */
+      if ( ((bytes + seg_size) > length)
+          /* 1771bis 4.3b: seg length contains one or more */
+          || (segh.length == 0) 
+          /* Paranoia in case someone changes type of segment length.
+           * Shift both values by 0x10 to make the comparison operate
+           * on more, than 8 bits (otherwise it's a warning, bug #564).
            */
-      if ((seg_size == 0) || (seg_size > length) || (segh.length == 0))
+          || ((sizeof segh.length > 1) 
+              && (0x10 + segh.length > 0x10 + AS_SEGMENT_MAX)))
         {
+          if (head)
             assegment_free_all (head);
           return NULL;
         }
       
-      length -= seg_size ;
+      switch (segh.type)
+        {
+          case AS_SEQUENCE:
+          case AS_SET:
+          case AS_CONFED_SEQUENCE:
+          case AS_CONFED_SET:
+            break;
+          default:
+            if (head)
+              assegment_free_all (head);
+            return NULL;
+        }
       
       /* now its safe to trust lengths */
       seg = assegment_new (segh.type, segh.length);
@@ -755,9 +754,11 @@ assegments_parse (struct stream *s, size_t length, int use32bit, int as4_path)
       for (i = 0; i < segh.length; i++)
        seg->as[i] = (use32bit) ? stream_getl (s) : stream_getw (s);
 
+      bytes += seg_size;
+      
       if (BGP_DEBUG (as4, AS4_SEGMENT))
-       zlog_debug ("[AS4SEG] Parse aspath segment: length left: %lu",
-                   (unsigned long) length);
+       zlog_debug ("[AS4SEG] Parse aspath segment: Bytes now: %lu",
+                   (unsigned long) bytes);
       
       prev = seg;
     }
@@ -765,42 +766,30 @@ assegments_parse (struct stream *s, size_t length, int use32bit, int as4_path)
   return assegment_normalise (head);
 }
 
-/* AS path parse function -- parses AS_PATH and AS4_PATH attributes
- *
- * Requires: s        -- stream, currently positioned before first segment
- *                       of AS_PATH or AS4_PATH (ie after attribute header)
- *           length   -- length of the value of the AS_PATH or AS4_PATH
- *           use32bit -- true <=> 4Byte ASN, otherwise 2Byte ASN
- *           as4_path -- true <=> AS4_PATH, otherwise AS_PATH
- *
- * Returns: if valid: address of struct aspath in the hash of known aspaths,
- *                    with reference count incremented.
- *              else: NULL
- *
- * NB: empty AS path (length == 0) is valid.  The returned struct aspath will
- *     have segments == NULL and str == zero length string (unique).
- */
+/* AS path parse function.  pnt is a pointer to byte stream and length
+   is length of byte stream.  If there is same AS path in the the AS
+   path hash then return it else make new AS path structure. */
 struct aspath *
-aspath_parse (struct stream *s, size_t length, int use32bit, int as4_path)
+aspath_parse (struct stream *s, size_t length, int use32bit)
 {
   struct aspath as;
   struct aspath *find;
 
-  /* Parse each segment and construct normalised list of struct assegment */
-  memset (&as, 0, sizeof (struct aspath));
-  if (length != 0)
-    {
-      as.segments = assegments_parse (s, length, use32bit, as4_path);
+  /* If length is odd it's malformed AS path. */
+  /* Nit-picking: if (use32bit == 0) it is malformed if odd,
+   * otherwise its malformed when length is larger than 2 and (length-2) 
+   * is not dividable by 4.
+   * But... this time we're lazy
+   */
+  if (length % AS16_VALUE_SIZE )
+    return NULL;
 
-      if (as.segments == NULL)
-        return NULL ;   /* Invalid AS_PATH or AS4_PATH  */
-    } ;
+  memset (&as, 0, sizeof (struct aspath));
+  as.segments = assegments_parse (s, length, use32bit);
   
   /* If already same aspath exist then return it. */
   find = hash_get (ashash, &as, aspath_hash_alloc);
   
-  assert(find) ;        /* valid aspath, so must find or create */
-  
   /* aspath_hash_alloc dupes segments too. that probably could be
    * optimised out.
    */
@@ -808,6 +797,8 @@ aspath_parse (struct stream *s, size_t length, int use32bit, int as4_path)
   if (as.str)
     XFREE (MTYPE_AS_STR, as.str);
   
+  if (! find)
+    return NULL;
   find->refcnt++;
 
   return find;
@@ -1631,7 +1622,7 @@ aspath_segment_add (struct aspath *as, int type)
 struct aspath *
 aspath_empty (void)
 {
-  return aspath_parse (NULL, 0, 1, 0); /* 32Bit ;-) not AS4_PATH */
+  return aspath_parse (NULL, 0, 1); /* 32Bit ;-) */
 }
 
 struct aspath *
index d63b914c18ff864045d0e9f44c0b608390e809cc..b8a5dfabae4fd8f432a1fc346c455392d3f930c0 100644 (file)
@@ -65,7 +65,7 @@ struct aspath
 /* Prototypes. */
 extern void aspath_init (void);
 extern void aspath_finish (void);
-extern struct aspath *aspath_parse (struct stream *, size_t, int, int);
+extern struct aspath *aspath_parse (struct stream *, size_t, int);
 extern struct aspath *aspath_dup (struct aspath *);
 extern struct aspath *aspath_aggregate (struct aspath *, struct aspath *);
 extern struct aspath *aspath_prepend (struct aspath *, struct aspath *);
index e2b6054b06b8cdfdb966c9238d5f99091be52cd6..7a0ab5607a3b605eb3d5953a5203e34ff65a2085 100644 (file)
@@ -746,78 +746,51 @@ bgp_attr_origin (struct peer *peer, bgp_size_t length,
 
   return 0;
 }
-/* Parse AS path information.  This function is wrapper of aspath_parse.
- *
- * Parses AS_PATH or AS4_PATH.
- *
- * Returns: if valid: address of struct aspath in the hash of known aspaths,
- *                    with reference count incremented.
- *              else: NULL
- *
- * NB: empty AS path (length == 0) is valid.  The returned struct aspath will
- *     have segments == NULL and str == zero length string (unique).
- */
-static struct aspath *
+
+/* Parse AS path information.  This function is wrapper of
+   aspath_parse. */
+static int
 bgp_attr_aspath (struct peer *peer, bgp_size_t length, 
-                struct attr *attr, u_char flag, u_char *startp, int as4_path)
+                struct attr *attr, u_char flag, u_char *startp)
 {
-  u_char require ;
-  struct aspath *asp ;
+  bgp_size_t total;
 
-  /* Check the attribute flags                                          */
-  require = as4_path ? BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS
-                     :                          BGP_ATTR_FLAG_TRANS ;
+  total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
 
-  if ((flag & (BGP_ATTR_FLAG_OPTIONAL | BGP_ATTR_FLAG_TRANS)) != require)
+  /* Flag check. */
+  if (CHECK_FLAG (flag, BGP_ATTR_FLAG_OPTIONAL)
+      || ! CHECK_FLAG (flag, BGP_ATTR_FLAG_TRANS))
     {
-      const char* path_type ;
-      bgp_size_t total;
-
-      path_type = as4_path ? "AS4_PATH" : "AS_PATH" ;
-
-      if (!CHECK_FLAG(flag, BGP_ATTR_FLAG_TRANS))
       zlog (peer->log, LOG_ERR, 
-            "%s attribute flag isn't transitive %d", path_type, flag) ;
-
-      if ((flag & BGP_ATTR_FLAG_OPTIONAL) != (require & BGP_ATTR_FLAG_OPTIONAL))
-        zlog (peer->log, LOG_ERR,
-            "%s attribute flag must %sbe optional %d", path_type,
-            (flag & BGP_ATTR_FLAG_OPTIONAL) ? "not " : "", flag) ;
-
-      total = length + (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN) ? 4 : 3);
-
+           "As-Path attribute flag isn't transitive %d", flag);
       bgp_notify_send_with_data (peer, 
                                 BGP_NOTIFY_UPDATE_ERR, 
                                 BGP_NOTIFY_UPDATE_ATTR_FLAG_ERR,
                                 startp, total);
+      return -1;
+    }
 
-      return NULL ;
-    } ;
-
-  /* Parse the AS_PATH/AS4_PATH body.
-   *
-   * For AS_PATH  peer with AS4 => 4Byte ASN otherwise 2Byte ASN
-   *     AS4_PATH 4Byte ASN
+  /*
+   * peer with AS4 => will get 4Byte ASnums
+   * otherwise, will get 16 Bit
    */
-  asp = aspath_parse (peer->ibuf, length,
-               as4_path || CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV), as4_path) ;
+  attr->aspath = aspath_parse (peer->ibuf, length, 
+                               CHECK_FLAG (peer->cap, PEER_CAP_AS4_RCV));
 
-  if (asp != NULL)
-    {
-      attr->flag |= ATTR_FLAG_BIT (as4_path ? BGP_ATTR_AS4_PATH
-                                            : BGP_ATTR_AS_PATH) ;
-    }
-  else
+  /* In case of IBGP, length will be zero. */
+  if (! attr->aspath)
     {
       zlog (peer->log, LOG_ERR, "Malformed AS path length is %d", length);
-
-      /* TODO: should BGP_NOTIFY_UPDATE_MAL_AS_PATH be sent for AS4_PATH ??  */
       bgp_notify_send (peer, 
                       BGP_NOTIFY_UPDATE_ERR, 
                       BGP_NOTIFY_UPDATE_MAL_AS_PATH);
-    } ;
+      return -1;
+    }
+
+  /* Set aspath attribute flag. */
+  attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS_PATH);
 
-  return asp ;
+  return 0;
 }
 
 static int bgp_attr_aspath_check( struct peer *peer, 
@@ -875,6 +848,21 @@ static int bgp_attr_aspath_check( struct peer *peer,
 
 }
 
+/* Parse AS4 path information.  This function is another wrapper of
+   aspath_parse. */
+static int
+bgp_attr_as4_path (struct peer *peer, bgp_size_t length, 
+                struct attr *attr, struct aspath **as4_path)
+{
+  *as4_path = aspath_parse (peer->ibuf, length, 1);
+
+  /* Set aspath attribute flag. */
+  if (as4_path)
+    attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH);
+
+  return 0;
+}
+
 /* Nexthop attribute. */
 static int
 bgp_attr_nexthop (struct peer *peer, bgp_size_t length, 
@@ -1613,12 +1601,10 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
          ret = bgp_attr_origin (peer, length, attr, flag, startp);
          break;
        case BGP_ATTR_AS_PATH:
-          attr->aspath = bgp_attr_aspath (peer, length, attr, flag, startp, 0);
-          ret = attr->aspath ? 0 : -1 ;
+         ret = bgp_attr_aspath (peer, length, attr, flag, startp);
          break;
        case BGP_ATTR_AS4_PATH:
-          as4_path = bgp_attr_aspath (peer, length, attr, flag, startp, 1);
-          ret = as4_path  ? 0 : -1 ;
+         ret = bgp_attr_as4_path (peer, length, attr, &as4_path );
          break;
        case BGP_ATTR_NEXT_HOP: 
          ret = bgp_attr_nexthop (peer, length, attr, flag, startp);