[bgpd] Fix number of DoS security issues, restricted to configured peers.

2007-12-22 Paul Jakma <paul.jakma@sun.com>

	* Fix series of vulnerabilities reported by "Mu Security
	  Research Team", where bgpd can be made to crash by sending
	  malformed packets - requires that bgpd be configured with a
	  session to the peer.
	* bgp_attr.c: (bgp_attr_as4_path) aspath_parse may fail, only
	  set the attribute flag indicating AS4_PATH if we actually managed
	  to parse one.
	  (bgp_attr_munge_as4_attrs) Assert was too general, it is possible
	  to receive AS4_AGGREGATOR before AGGREGATOR.
	  (bgp_attr_parse) Check that we have actually received the extra
	  byte of header for Extended-Length attributes.
	* bgp_attr.h: Fix BGP_ATTR_MIN_LEN to account for the length byte.
	* bgp_open.c: (cap_minsizes) Fix size of CAPABILITY_CODE_RESTART,
	  incorrect -2 left in place from a development version of as4-path
	  patch.
	* bgp_packet.c: (bgp_route_refresh_receive) ORF length parameter
	  needs to be properly sanity checked.
	* tests/bgp_capability_test.c: Test for empty capabilities.

bgpd/ChangeLog

@@ -1,3 +1,23 @@
+2007-12-22 Paul Jakma <paul.jakma@sun.com>
+
+	* Fix series of vulnerabilities reported by "Mu Security
+	  Research Team", where bgpd can be made to crash by sending
+	  malformed packets - requires that bgpd be configured with a
+	  session to the peer.
+	* bgp_attr.c: (bgp_attr_as4_path) aspath_parse may fail, only
+	  set the attribute flag indicating AS4_PATH if we actually managed
+	  to parse one.
+	  (bgp_attr_munge_as4_attrs) Assert was too general, it is possible
+	  to receive AS4_AGGREGATOR before AGGREGATOR.
+	  (bgp_attr_parse) Check that we have actually received the extra
+	  byte of header for Extended-Length attributes.
+	* bgp_attr.h: Fix BGP_ATTR_MIN_LEN to account for the length byte.
+	* bgp_open.c: (cap_minsizes) Fix size of CAPABILITY_CODE_RESTART,
+	  incorrect -2 left in place from a development version of as4-path
+	  patch.
+	* bgp_packet.c: (bgp_route_refresh_receive) ORF length parameter
+	  needs to be properly sanity checked.
+ 
 2007-12-18 Denis Ovsienko
 
 	* bgp_routemap.c: (no_set_aspath_prepend) This command cancelled

bgpd/bgp_attr.c

@@ -892,7 +892,8 @@ bgp_attr_as4_path (struct peer *peer, bgp_size_t length,
   *as4_path = aspath_parse (peer->ibuf, length, 1);
 
   /* Set aspath attribute flag. */
-  attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH);
+  if (as4_path)
+    attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_AS4_PATH);
 
   return 0;
 }
@@ -1126,10 +1127,10 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr,
    */
   if (attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AS4_AGGREGATOR) ) )
     {
-      assert (attre);
-      
       if ( attr->flag & (ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR) ) )
         {
+          assert (attre);
+          
           /* received both.
            * if the as_number in aggregator is not AS_TRANS,
            *  then AS4_AGGREGATOR and AS4_PATH shall be ignored
@@ -1170,7 +1171,7 @@ bgp_attr_munge_as4_attrs (struct peer *peer, struct attr *attr,
             zlog_debug ("[AS4] %s BGP not AS4 capable peer send"
                         " AS4_AGGREGATOR but no AGGREGATOR, will take"
                         " it as if AGGREGATOR with AS_TRANS had been there", peer->host);
-          attre->aggregator_as = as4_aggregator;
+          (attre = bgp_attr_extra_get (attr))->aggregator_as = as4_aggregator;
           /* sweep it under the carpet and simulate a "good" AGGREGATOR */
           attr->flag |= (ATTR_FLAG_BIT (BGP_ATTR_AGGREGATOR));
         }
@@ -1543,6 +1544,21 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
       flag = stream_getc (BGP_INPUT (peer));
       type = stream_getc (BGP_INPUT (peer));
 
+      /* Check whether Extended-Length applies and is in bounds */
+      if (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN)
+          && ((endp - startp) < (BGP_ATTR_MIN_LEN + 1)))
+	{
+	  zlog (peer->log, LOG_WARNING, 
+		"%s Extended length set, but just %u bytes of attr header",
+		peer->host,
+		(unsigned long) (endp - STREAM_PNT (BGP_INPUT (peer))));
+
+	  bgp_notify_send (peer, 
+			   BGP_NOTIFY_UPDATE_ERR, 
+			   BGP_NOTIFY_UPDATE_ATTR_LENG_ERR);
+	  return -1;
+	}
+
       /* Check extended attribue length bit. */
       if (CHECK_FLAG (flag, BGP_ATTR_FLAG_EXTLEN))
 	length = stream_getw (BGP_INPUT (peer));

bgpd/bgp_attr.h

@@ -44,7 +44,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 #define BGP_ATTR_FLAG_EXTLEN    0x10	/* Extended length flag. */
 
 /* BGP attribute header must bigger than 2. */
-#define BGP_ATTR_MIN_LEN        2       /* Attribute flag and type. */
+#define BGP_ATTR_MIN_LEN        3       /* Attribute flag, type length. */
 #define BGP_ATTR_DEFAULT_WEIGHT 32768
 
 /* Additional/uncommon BGP attributes.

bgpd/bgp_open.c

@@ -461,7 +461,7 @@ static size_t cap_minsizes[] =
   [CAPABILITY_CODE_MP]		= sizeof (struct capability_mp_data),
   [CAPABILITY_CODE_REFRESH]	= CAPABILITY_CODE_REFRESH_LEN,
   [CAPABILITY_CODE_ORF]		= sizeof (struct capability_orf_entry),
-  [CAPABILITY_CODE_RESTART]	= sizeof (struct capability_gr) - 2,
+  [CAPABILITY_CODE_RESTART]	= sizeof (struct capability_gr),
   [CAPABILITY_CODE_AS4]		= CAPABILITY_CODE_AS4_LEN,
   [CAPABILITY_CODE_DYNAMIC]	= CAPABILITY_CODE_DYNAMIC_LEN,
   [CAPABILITY_CODE_REFRESH_OLD]	= CAPABILITY_CODE_REFRESH_LEN,

bgpd/bgp_packet.c

@@ -1960,11 +1960,14 @@ bgp_route_refresh_receive (struct peer *peer, bgp_size_t size)
       when_to_refresh = stream_getc (s);
       end = stream_pnt (s) + (size - 5);
 
-      while (stream_pnt (s) < end)
+      while ((stream_pnt (s) + 2) < end)
 	{
 	  orf_type = stream_getc (s); 
 	  orf_len = stream_getw (s);
-
+	  
+	  /* orf_len in bounds? */
+	  if ((stream_pnt (s) + orf_len) > end)
+	    break; /* XXX: Notify instead?? */
 	  if (orf_type == ORF_TYPE_PREFIX
 	      || orf_type == ORF_TYPE_PREFIX_OLD)
 	    {
@@ -1984,6 +1987,12 @@ bgp_route_refresh_receive (struct peer *peer, bgp_size_t size)
 			     peer->host, orf_type, orf_len);
 		}
 
+              /* we're going to read at least 1 byte of common ORF header,
+               * and 7 bytes of ORF Address-filter entry from the stream
+               */
+              if (orf_len < 7)
+                break; 
+                
 	      /* ORF prefix-list name */
 	      sprintf (name, "%s.%d.%d", peer->host, afi, safi);
 

tests/ChangeLog

@@ -1,3 +1,7 @@
+2007-12-22 Paul Jakma <paul.jakma@sun.com>
+
+	* bgp_capability_test.c: Test for empty capabilities.
+
 2007-09-27 Paul Jakma <paul.jakma@sun.com>
 
 	* aspath_test.c: Test dupe-weeding from sets.

tests/bgp_capability_test.c

@@ -362,6 +362,36 @@ static struct test_segment misc_segments[] =
     },
     15, SHOULD_ERR,
   },
+  { "GR-empty",
+    "GR capability, but empty.",
+    { /* hdr */		0x40, 0x0,
+    },
+    2, SHOULD_ERR,
+  },
+  { "MP-empty",
+    "MP capability, but empty.",
+    { /* hdr */		0x1, 0x0,
+    },
+    2, SHOULD_ERR,
+  },
+  { "ORF-empty",
+    "ORF capability, but empty.",
+    { /* hdr */		0x3, 0x0,
+    },
+    2, SHOULD_ERR,
+  },
+  { "AS4-empty",
+    "AS4 capability, but empty.",
+    { /* hdr */		0x41, 0x0,
+    },
+    2, SHOULD_ERR,
+  },
+  { "dyn-empty",
+    "Dynamic capability, but empty.",
+    { /* hdr */		0x42, 0x0,
+    },
+    2, SHOULD_PARSE,
+  },
   { "dyn-old",
     "Dynamic capability (deprecated version)",
     { CAPABILITY_CODE_DYNAMIC, 0x0 },