Fixes some problems in BGP error handling.

2 files changed, 156 insertions, 84 deletions

diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c
index ff231b17..e1a3671a 100644
--- a/proto/bgp/attrs.c
+++ b/proto/bgp/attrs.c
@@ -22,6 +22,42 @@
 
 #include "bgp.h"
 
+/*
+ *   UPDATE message error handling
+ *
+ * All checks from RFC 4271 6.3 are done as specified with these exceptions:
+ *  - The semantic check of an IP address from NEXT_HOP attribute is missing.
+ *  - Checks of some optional attribute values are missing.
+ *  - Syntactic and semantic checks of NLRIs (done in DECODE_PREFIX())
+ *    are probably inadequate.
+ *
+ * Loop detection based on AS_PATH causes updates to be withdrawn. RFC
+ * 4271 does not explicitly specifiy the behavior in that case.
+ *
+ * Loop detection related to route reflection (based on ORIGINATOR_ID
+ * and CLUSTER_LIST) causes updates to be withdrawn. RFC 4456 8
+ * specifies that such updates should be ignored, but that is generally
+ * a bad idea.
+ *
+ * Error checking of optional transitive attributes is done according to
+ * draft-ietf-idr-optional-transitive-03, but errors are handled always
+ * as withdraws.
+ *
+ * Unexpected AS_CONFED_* segments in AS_PATH are logged and removed,
+ * but unknown segments cause a session drop with Malformed AS_PATH
+ * error (see validate_path()). The behavior in such case is not
+ * explicitly specified by RFC 4271. RFC 5065 specifies that
+ * inconsistent AS_CONFED_* segments should cause a session drop, but
+ * implementations that pass invalid AS_CONFED_* segments are
+ * widespread.
+ *
+ * Error handling of AS4_* attributes is done as specified by
+ * draft-ietf-idr-rfc4893bis-03. There are several possible
+ * inconsistencies between AGGREGATOR and AS4_AGGREGATOR that are not
+ * handled by that draft, these are logged and ignored (see
+ * bgp_reconstruct_4b_attrs()).
+ */
+
 static byte bgp_mandatory_attrs[] = { BA_ORIGIN, BA_AS_PATH
 #ifndef IPV6
 ,BA_NEXT_HOP
@@ -38,6 +74,9 @@ struct attr_desc {
   void (*format)(eattr *ea, byte *buf, int buflen);
 };
 
+#define IGNORE -1
+#define WITHDRAW -2
+
 static int
 bgp_check_origin(struct bgp_proto *p UNUSED, byte *a, int len UNUSED)
 {
@@ -152,7 +191,7 @@ static int
 bgp_check_next_hop(struct bgp_proto *p UNUSED, byte *a, int len)
 {
 #ifdef IPV6
-  return -1;
+  return IGNORE;
 #else
   ip_addr addr;
 
@@ -186,7 +225,7 @@ bgp_check_aggregator(struct bgp_proto *p, byte *a UNUSED, int len)
 {
   int exp_len = p->as4_session ? 8 : 6;
   
-  return (len == exp_len) ? 0 : 5;
+  return (len == exp_len) ? 0 : WITHDRAW;
 }
 
 static void
@@ -203,6 +242,13 @@ bgp_format_aggregator(eattr *a, byte *buf, int buflen UNUSED)
 }
 
 static int
+bgp_check_community(struct bgp_proto *p UNUSED, byte *a UNUSED, int len)
+{
+  return ((len % 4) == 0) ? 0 : WITHDRAW;
+}
+
+
+static int
 bgp_check_cluster_list(struct bgp_proto *p UNUSED, byte *a UNUSED, int len)
 {
   return ((len % 4) == 0) ? 0 : 5;
@@ -221,7 +267,7 @@ bgp_check_reach_nlri(struct bgp_proto *p UNUSED, byte *a UNUSED, int len UNUSED)
   p->mp_reach_start = a;
   p->mp_reach_len = len;
 #endif
-  return -1;
+  return IGNORE;
 }
 
 static int
@@ -231,7 +277,7 @@ bgp_check_unreach_nlri(struct bgp_proto *p UNUSED, byte *a UNUSED, int len UNUSE
   p->mp_unreach_start = a;
   p->mp_unreach_len = len;
 #endif
-  return -1;
+  return IGNORE;
 }
 
 static struct attr_desc bgp_attr_table[] = {
@@ -252,19 +298,19 @@ static struct attr_desc bgp_attr_table[] = {
   { "aggregator", -1, BAF_OPTIONAL | BAF_TRANSITIVE, EAF_TYPE_OPAQUE, 1,	/* BA_AGGREGATOR */
     bgp_check_aggregator, bgp_format_aggregator },
   { "community", -1, BAF_OPTIONAL | BAF_TRANSITIVE, EAF_TYPE_INT_SET, 1,	/* BA_COMMUNITY */
-    NULL, NULL },
+    bgp_check_community, NULL },
   { "originator_id", 4, BAF_OPTIONAL, EAF_TYPE_ROUTER_ID, 0,			/* BA_ORIGINATOR_ID */
     NULL, NULL },
   { "cluster_list", -1, BAF_OPTIONAL, EAF_TYPE_INT_SET, 0,			/* BA_CLUSTER_LIST */
     bgp_check_cluster_list, bgp_format_cluster_list }, 
   { .name = NULL },								/* BA_DPA */
-  { .name = NULL },									/* BA_ADVERTISER */
-  { .name = NULL },									/* BA_RCID_PATH */
+  { .name = NULL },								/* BA_ADVERTISER */
+  { .name = NULL },								/* BA_RCID_PATH */
   { "mp_reach_nlri", -1, BAF_OPTIONAL, EAF_TYPE_OPAQUE, 1,			/* BA_MP_REACH_NLRI */
     bgp_check_reach_nlri, NULL },
   { "mp_unreach_nlri", -1, BAF_OPTIONAL, EAF_TYPE_OPAQUE, 1,			/* BA_MP_UNREACH_NLRI */
     bgp_check_unreach_nlri, NULL },
-  {  .name = NULL },									/* BA_EXTENDED_COMM */
+  {  .name = NULL },								/* BA_EXTENDED_COMM */
   { "as4_path", -1, BAF_OPTIONAL | BAF_TRANSITIVE, EAF_TYPE_OPAQUE, 1,		/* BA_AS4_PATH */
     NULL, NULL },
   { "as4_aggregator", -1, BAF_OPTIONAL | BAF_TRANSITIVE, EAF_TYPE_OPAQUE, 1,	/* BA_AS4_PATH */
@@ -1163,15 +1209,7 @@ bgp_merge_as_paths(struct adata *old2, struct adata *old4, int req_as, struct li
 static int
 as4_aggregator_valid(struct adata *aggr)
 {
-  if (aggr->length != 8)
-    return 0;
-
-  u32 *a = (u32 *) aggr->data;
-
-  if ((a[0] == 0) || (a[1] == 0))
-    return 0;
-
-  return 1;
+  return aggr->length == 8;
 }
 
 
@@ -1258,7 +1296,7 @@ bgp_remove_as4_attrs(struct bgp_proto *p, rta *a)
 	{
 	  *el = (*el)->next;
 	  if (p->as4_session)
-	    log(L_WARN "BGP: Unexpected AS4_* attributes received");
+	    log(L_WARN "%s: Unexpected AS4_* attributes received", p->p.name);
 	}
       else
 	el = &((*el)->next);
@@ -1288,6 +1326,7 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin
   byte seen[256/8];
   ea_list *ea;
   struct adata *ad;
+  int withdraw = 0;
 
   bzero(a, sizeof(rta));
   a->proto = &bgp->p;
@@ -1345,8 +1384,14 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin
 	      errcode = desc->validate(bgp, z, l);
 	      if (errcode > 0)
 		goto err;
-	      if (errcode < 0)
+	      if (errcode == IGNORE)
 		continue;
+	      if (errcode <= WITHDRAW)
+		{
+		  log(L_WARN "%s: Attribute %s is malformed, withdrawing update",
+		      bgp->p.name, desc->name);
+		  withdraw = 1;
+		}
 	    }
 	  else if (code == BA_AS_PATH)
 	    {
@@ -1407,6 +1452,9 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin
 	}
     }
 
+  if (withdraw)
+    goto withdraw;
+
 #ifdef IPV6
   /* If we received MP_REACH_NLRI we should check mandatory attributes */
   if (bgp->mp_reach_len != 0)
@@ -1438,12 +1486,12 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin
 
   /* If the AS path attribute contains our AS, reject the routes */
   if (bgp_as_path_loopy(bgp, a))
-    goto loop;
+    goto withdraw;
 
   /* Two checks for IBGP loops caused by route reflection, RFC 4456 */ 
   if (bgp_originator_id_loopy(bgp, a) ||
       bgp_cluster_list_loopy(bgp, a))
-    goto loop;
+    goto withdraw;
 
   /* If there's no local preference, define one */
   if (!(seen[0] & (1 << BA_LOCAL_PREF)))
@@ -1451,8 +1499,7 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin
 
   return a;
 
-loop:
-  DBG("BGP: Path loop!\n");
+withdraw:
   return NULL;
 
 malformed:
diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c
index bf986401..0b41244b 100644
--- a/proto/bgp/packets.c
+++ b/proto/bgp/packets.c
@@ -790,9 +790,9 @@ bgp_rx_open(struct bgp_conn *conn, byte *pkt, int len)
   int b = *pp++;				\
   int q;					\
   ll--;						\
-  if (b > BITS_PER_IP_ADDRESS) { err=10; goto bad; } \
+  if (b > BITS_PER_IP_ADDRESS) { err=10; goto done; } \
   q = (b+7) / 8;				\
-  if (ll < q) { err=1; goto bad; }		\
+  if (ll < q) { err=1; goto done; }		\
   memcpy(&prefix, pp, q);			\
   pp += q;					\
   ll -= q;					\
@@ -840,11 +840,10 @@ bgp_do_rx_update(struct bgp_conn *conn,
 		 byte *attrs, int attr_len)
 {
   struct bgp_proto *p = conn->bgp;
-  rta *a0;
-  rta *a = NULL;
-  ip_addr prefix;
   net *n;
-  int err = 0, pxlen;
+  rta *a0, *a = NULL;
+  ip_addr prefix;
+  int pxlen, err = 0;
 
   /* Withdraw routes */
   while (withdrawn_len)
@@ -859,32 +858,43 @@ bgp_do_rx_update(struct bgp_conn *conn,
     return;
 
   a0 = bgp_decode_attrs(conn, attrs, attr_len, bgp_linpool, nlri_len);
-  if (a0 && nlri_len && bgp_set_next_hop(p, a0))
+
+  if (conn->state != BS_ESTABLISHED)	/* fatal error during decoding */
+    return;
+
+  if (a0 && bgp_set_next_hop(p, a0))
+    a = rta_lookup(a0);
+
+  while (nlri_len)
     {
-      a = rta_lookup(a0);
-      while (nlri_len)
+      DECODE_PREFIX(nlri, nlri_len);
+      DBG("Add %I/%d\n", prefix, pxlen);
+
+      if (a)
 	{
-	  rte *e;
-	  DECODE_PREFIX(nlri, nlri_len);
-	  DBG("Add %I/%d\n", prefix, pxlen);
-	  e = rte_get_temp(rta_clone(a));
-	  n = net_get(p->p.table, prefix, pxlen);
-	  e->net = n;
+	  rte *e = rte_get_temp(rta_clone(a));
+	  e->net = net_get(p->p.table, prefix, pxlen);
 	  e->pflags = 0;
-	  rte_update(p->p.table, n, &p->p, &p->p, e);
-	  if (bgp_apply_limits(p) < 0)
-	    goto bad2;
+	  rte_update(p->p.table, e->net, &p->p, &p->p, e);
+	}
+      else
+	{
+	  /* Forced withdraw as a result of soft error */
+	  if (n = net_find(p->p.table, prefix, pxlen))
+	    rte_update(p->p.table, n, &p->p, &p->p, NULL);
 	}
-      rta_free(a);
-    }
 
-  return;
+      if (bgp_apply_limits(p) < 0)
+	goto done;
+    }
 
- bad:
-  bgp_error(conn, 3, err, NULL, 0);
- bad2:
+ done:
   if (a)
     rta_free(a);
+
+  if (err)
+    bgp_error(conn, 3, err, NULL, 0);
+
   return;
 }
 
@@ -895,7 +905,7 @@ bgp_do_rx_update(struct bgp_conn *conn,
   len = len0 = p->name##_len;				\
   if (len)						\
     {							\
-      if (len < 3) goto bad;				\
+      if (len < 3) { err=9; goto done; }		\
       af = get_u16(x);					\
       sub = x[2];					\
       x += 3;						\
@@ -907,6 +917,24 @@ bgp_do_rx_update(struct bgp_conn *conn,
   if (af == BGP_AF_IPV6)
 
 static void
+bgp_attach_next_hop(rta *a0, byte *x)
+{
+  ip_addr *nh = (ip_addr *) bgp_attach_attr_wa(&a0->eattrs, bgp_linpool, BA_NEXT_HOP, NEXT_HOP_LENGTH);
+  memcpy(nh, x+1, 16);
+  ipa_ntoh(nh[0]);
+
+  /* We store received link local address in the other part of BA_NEXT_HOP eattr. */
+  if (*x == 32)
+    {
+      memcpy(nh+1, x+17, 16);
+      ipa_ntoh(nh[1]);
+    }
+  else
+    nh[1] = IPA_NONE;
+}
+
+
+static void
 bgp_do_rx_update(struct bgp_conn *conn,
 		 byte *withdrawn, int withdrawn_len,
 		 byte *nlri, int nlri_len,
@@ -916,16 +944,16 @@ bgp_do_rx_update(struct bgp_conn *conn,
   byte *start, *x;
   int len, len0;
   unsigned af, sub;
-  rta *a0;
-  rta *a = NULL;
-  ip_addr prefix;
   net *n;
-  int err = 0, pxlen;
+  rta *a0, *a = NULL;
+  ip_addr prefix;
+  int pxlen, err = 0;
 
   p->mp_reach_len = 0;
   p->mp_unreach_len = 0;
   a0 = bgp_decode_attrs(conn, attrs, attr_len, bgp_linpool, 0);
-  if (!a0)
+
+  if (conn->state != BS_ESTABLISHED)	/* fatal error during decoding */
     return;
 
   DO_NLRI(mp_unreach)
@@ -943,52 +971,49 @@ bgp_do_rx_update(struct bgp_conn *conn,
     {
       /* Create fake NEXT_HOP attribute */
       if (len < 1 || (*x != 16 && *x != 32) || len < *x + 2)
-	goto bad;
+	{ err = 9; goto done; }
 
-      ip_addr *nh = (ip_addr *) bgp_attach_attr_wa(&a0->eattrs, bgp_linpool, BA_NEXT_HOP, NEXT_HOP_LENGTH);
-      memcpy(nh, x+1, 16);
-      ipa_ntoh(nh[0]);
-
-      /* We store received link local address in the other part of BA_NEXT_HOP eattr. */
-      if (*x == 32)
-	{
-	  memcpy(nh+1, x+17, 16);
-	  ipa_ntoh(nh[1]);
-	}
-      else
-	nh[1] = IPA_NONE;
+      if (a0)
+	bgp_attach_next_hop(a0, x);
 
       /* Also ignore one reserved byte */
       len -= *x + 2;
       x += *x + 2;
 
-      if (bgp_set_next_hop(p, a0))
+      if (a0 && bgp_set_next_hop(p, a0))
+	a = rta_lookup(a0);
+
+      while (len)
 	{
-	  a = rta_lookup(a0);
-	  while (len)
+	  DECODE_PREFIX(x, len);
+	  DBG("Add %I/%d\n", prefix, pxlen);
+
+	  if (a)
 	    {
-	      rte *e;
-	      DECODE_PREFIX(x, len);
-	      DBG("Add %I/%d\n", prefix, pxlen);
-	      e = rte_get_temp(rta_clone(a));
-	      n = net_get(p->p.table, prefix, pxlen);
-	      e->net = n;
+	      rte *e = rte_get_temp(rta_clone(a));
+	      e->net = net_get(p->p.table, prefix, pxlen);
 	      e->pflags = 0;
-	      rte_update(p->p.table, n, &p->p, &p->p, e);
-	      if (bgp_apply_limits(p) < 0)
-		goto bad2;
+	      rte_update(p->p.table, e->net, &p->p, &p->p, e);
+	    }
+	  else
+	    {
+	      /* Forced withdraw as a result of soft error */
+	      if (n = net_find(p->p.table, prefix, pxlen))
+		rte_update(p->p.table, n, &p->p, &p->p, NULL);
 	    }
-	  rta_free(a);
+
+	  if (bgp_apply_limits(p) < 0)
+	    goto done;
 	}
     }
 
-  return;
-
- bad:
-  bgp_error(conn, 3, 9, start, len0);
- bad2:
+ done:
   if (a)
     rta_free(a);
+
+  if (err) /* Use subcode 9, not err */
+    bgp_error(conn, 3, 9, NULL, 0);
+
   return;
 }