Changes handling of AS_PATH_CONFED_* segments in AS_PATH.

Although standard says that if we receive AS_PATH_CONFED_*
(and we are not a part of a confederation) segment, we should
drop session, nobody does that and it is unwise to do that.

Now we drop session just in case that peer ASN is in
AS_PATH_CONFED_* segment (to detect peer that considers BIRD
as a part of its confederation).

diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c
index 8a849e73cb96ca05cc8d5fde8717f214c50fdcd6..e50f6a9c843d23d43928d8811a45ed475b99bce3 100644 (file)
--- a/proto/bgp/attrs.c
+++ b/proto/bgp/attrs.c
@@ -55,25 +55,97 @@ bgp_format_origin(eattr *a, byte *buf, int buflen)
 }
 
 static int
-bgp_check_path(byte *a, int len, int bs, int errcode)
+path_segment_contains(byte *p, int bs, u32 asn)
 {
-  while (len)
+  int i;
+  int len = p[1];
+  p += 2;
+
+  for(i=0; i<len; i++)
     {
-      DBG("Path segment %02x %02x\n", a[0], a[1]);
-      if (len < 2 ||
-         (a[0] != AS_PATH_SET && a[0] != AS_PATH_SEQUENCE) ||
-         bs * a[1] + 2 > len)
-       return errcode;
-      len -= bs * a[1] + 2;
-      a += bs * a[1] + 2;
+      u32 asn2 = (bs == 4) ? get_u32(p) : get_u16(p);
+      if (asn2 == asn)
+       return 1;
+      p += bs;
     }
+
   return 0;
 }
 
+/* Validates path attribute, removes AS_CONFED_* segments, and also returns path length */
 static int
-bgp_check_as_path(struct bgp_proto *p, byte *a, int len)
+validate_path(struct bgp_proto *p, int as_path, int bs, byte *idata, unsigned int *ilength)
+{
+  int res = 0;
+  u8 *a, *dst;
+  int len, plen, copy;
+
+  dst = a = idata;
+  len = *ilength;
+
+  while (len)
+    {
+      if (len < 2)
+       return -1;
+
+      plen = 2 + bs * a[1];
+      if (len < plen)
+       return -1;
+
+      switch (a[0])
+       {
+       case AS_PATH_SET:
+         copy = 1;
+         res++;
+         break;
+
+       case AS_PATH_SEQUENCE:
+         copy = 1;
+         res += a[1];
+         break;
+
+       case AS_PATH_CONFED_SEQUENCE:
+       case AS_PATH_CONFED_SET:
+         if (as_path && path_segment_contains(a, bs, p->remote_as))
+           {
+             log(L_WARN "%s: AS_CONFED_* segment with peer ASN found, misconfigured confederation?", p->p.name);
+             return -1;
+           }
+
+         log(L_WARN "%s: %s_PATH attribute contains AS_CONFED_* segment, skipping segment",
+             p->p.name, as_path ? "AS" : "AS4");
+         copy = 0;
+         break;
+
+       default:
+         return -1;
+       }
+
+      if (copy)
+       {
+         if (dst != a)
+           memmove(dst, a, plen);
+         dst += plen;
+       }
+
+      len -= plen;
+      a += plen;
+    }
+
+  *ilength = dst - idata;
+  return res;
+}
+
+static inline int
+validate_as_path(struct bgp_proto *p, byte *a, int *len)
 {
-  return bgp_check_path(a, len, p->as4_session ? 4 : 2, 11);
+  return validate_path(p, 1, p->as4_session ? 4 : 2, a, len);
+}
+
+static inline int
+validate_as4_path(struct bgp_proto *p, struct adata *path)
+{
+  return validate_path(p, 0, 4, path->data, &path->length);
 }
 
 static int
@@ -160,7 +232,7 @@ static struct attr_desc bgp_attr_table[] = {
   { "origin", 1, BAF_TRANSITIVE, EAF_TYPE_INT, 1,                              /* BA_ORIGIN */
     bgp_check_origin, bgp_format_origin },
   { "as_path", -1, BAF_TRANSITIVE, EAF_TYPE_AS_PATH, 1,                                /* BA_AS_PATH */
-    bgp_check_as_path, NULL },
+    NULL, NULL }, /* is checked by validate_as_path() as a special case */
   { "next_hop", 4, BAF_TRANSITIVE, EAF_TYPE_IP_ADDRESS, 1,                     /* BA_NEXT_HOP */
     bgp_check_next_hop, NULL },
   { "med", 4, BAF_OPTIONAL, EAF_TYPE_INT, 1,                                   /* BA_MULTI_EXIT_DISC */
@@ -1061,73 +1133,6 @@ as4_aggregator_valid(struct adata *aggr)
     return 0;
 }
 
-static int
-as4_path_sanitize_and_get_length(struct adata *path)
-{
-  int res = 0;
-  u8 *p, *dst;
-  int len, plen, copy;
-
-  dst = p = path->data;
-  len = path->length;
-
-  while (len)
-    {
-      if (len <= 2) /* We do not allow empty segments */
-       goto inconsistent_path;
-
-      switch (p[0])
-       {
-       case AS_PATH_SET:
-         plen = 2 + 4 * p[1];
-         copy = 1;
-         res++;
-         break;
-
-       case AS_PATH_SEQUENCE:
-         plen = 2 + 4 * p[1];
-         copy = 1;
-         res += p[1];
-         break;
-
-       case AS_PATH_CONFED_SEQUENCE:
-       case AS_PATH_CONFED_SET:
-         log(L_WARN "BGP: AS4_PATH attribute contains AS_CONFED_* segment, skipping segment");
-         plen = 2 + 4 * p[1];
-         copy = 0;
-         break;
-
-       default:
-         goto unknown_segment;
-       }
-
-      if (len < plen)
-       goto inconsistent_path;
-
-      if (copy)
-       {
-         if (dst != p)
-           memmove(dst, p, plen);
-         dst += plen;
-       }
-
-      len -= plen;
-      p += plen;
-    }
-
-  path->length = dst - path->data;
-  return res;
-
- inconsistent_path:
-  log(L_WARN "BGP: AS4_PATH attribute is inconsistent, skipping attribute");
-  return -1;
-
- unknown_segment:
-  log(L_WARN "BGP: AS4_PATH attribute contains unknown segment, skipping attribute");
-  return -1;
-}
-
-
 
 /* Reconstruct 4B AS_PATH and AGGREGATOR according to RFC 4893 4.2.3 */
 static void
@@ -1141,7 +1146,7 @@ bgp_reconstruct_4b_atts(struct bgp_proto *p, rta *a, struct linpool *pool)
 
   if (a4 && !as4_aggregator_valid(a4->u.ptr))
     {
-      log(L_WARN "BGP: AS4_AGGREGATOR attribute is invalid, skipping attribute");
+      log(L_WARN "%s: AS4_AGGREGATOR attribute is invalid, skipping attribute", p->p.name);
       a4 = NULL;
       a4_removed = 1;
     }
@@ -1177,15 +1182,18 @@ bgp_reconstruct_4b_atts(struct bgp_proto *p, rta *a, struct linpool *pool)
          a2->u.ptr = bgp_aggregator_convert_to_new(a2->u.ptr, pool);
 
          if ((a2_as == AS_TRANS) && !a4_removed)
-           log(L_WARN "BGP: AGGREGATOR attribute contain AS_TRANS, but AS4_AGGREGATOR is missing");
+           log(L_WARN "%s: AGGREGATOR attribute contain AS_TRANS, but AS4_AGGREGATOR is missing", p->p.name);
        }
     }
   else
     if (a4)
-      log(L_WARN "BGP: AS4_AGGREGATOR attribute received, but AGGREGATOR attribute is missing");
+      log(L_WARN "%s: AS4_AGGREGATOR attribute received, but AGGREGATOR attribute is missing", p->p.name);
 
   int p2_len = as_path_getlen(p2->u.ptr);
-  int p4_len = p4 ? as4_path_sanitize_and_get_length(p4->u.ptr) : -1;
+  int p4_len = p4 ? validate_as4_path(p, p4->u.ptr) : -1;
+
+  if (p4 && (p4_len < 0))
+    log(L_WARN "%s: AS4_PATH attribute is malformed, skipping attribute", p->p.name);
 
   if ((p4_len <= 0) || (p2_len < p4_len))
     p2->u.ptr = bgp_merge_as_paths(p2->u.ptr, NULL, AS_PATH_MAXLEN, pool);
@@ -1200,7 +1208,7 @@ bgp_remove_as4_attrs(struct bgp_proto *p, rta *a)
   unsigned id2 = EA_CODE(EAP_BGP, BA_AS4_AGGREGATOR);
   ea_list **el = &(a->eattrs);
 
-  /* We know that ea_lists constructed in bgp_decode_attrs have one attribute per ea_list struct */
+  /* We know that ea_lists constructed in bgp_decode attrs have one attribute per ea_list struct */
   while (*el != NULL)
     {
       unsigned fid = (*el)->attrs[0].id;
@@ -1302,6 +1310,12 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin
              if (errcode < 0)
                continue;
            }
+         else if (code == BA_AS_PATH)
+           {
+             /* Special case as it might also trim the attribute */
+             if (validate_as_path(bgp, z, &l) < 0)
+               { errcode = 11; goto err; }
+           }
          type = desc->type;
        }
       else                             /* Unknown attribute */
@@ -1310,6 +1324,11 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin
            { errcode = 2; goto err; }
          type = EAF_TYPE_OPAQUE;
        }
+      
+      // Only OPTIONAL and TRANSITIVE attributes may have non-zero PARTIAL flag
+      // if (!((flags & BAF_OPTIONAL) && (flags & BAF_TRANSITIVE)) && (flags & BAF_PARTIAL))
+      //   { errcode = 4; goto err; }
+
       seen[code/8] |= (1 << (code%8));
       ea = lp_alloc(pool, sizeof(ea_list) + sizeof(eattr));
       ea->next = a->eattrs;