[quagga-dev 5128] bgpd crash fixes (for review)

Paul Jakma paul at clubi.ie
Mon Nov 19 23:22:06 GMT 2007


Hi,

I've been sitting on the attached fixes for a while, hoping that the 
reporter would be able to verify whether it fixed the problems. 
Unfortunately that hasn't possible, so the patch needs review here.

- Fixes for a problem in the AS4 updates, where as4_path could be
   dereferenced even if aspath_parse failed to set as4_path (should be
   obviously correct)

- incorrect assert, testing that AS4_AGGREGATOR implied we'd received
   an aggregator already (and hence attr->extra was assumed valid).
   That need not be the case. (fix again is reasonably obvious)

- Failure to properly sanity check extended length attribute header
   for minimum length
   (this bit of the patch looks incomplete - i'll need to go recheck
    to see if i was being clever or just stupid)

- some bugs in the capability cleanup - GR capability was testing
   against wrong minimum size (stray -2 left in from an older working
   of the capability cleanup patch)

   - unit tested

- ORF length parameter in route-refresh wasn't being checked
   correctly

   (this part of the patch, in bgp_packet.c, I can't easily test -
    extra eyeballs would be appreciated)

regards,
-- 
Paul Jakma	paul at clubi.ie	paul at jakma.org	Key ID: 64A2FF6A
Fortune:
All Finagle Laws may be bypassed by learning the simple art of doing
without thinking.
-------------- next part --------------
? build
? doc/quagga.info-1
? doc/quagga.info-2
Index: bgpd/bgp_attr.c
===================================================================
RCS file: /var/cvsroot/quagga/bgpd/bgp_attr.c,v
retrieving revision 1.25
diff -u -p -r1.25 bgp_attr.c
--- bgpd/bgp_attr.c	14 Oct 2007 22:32:21 -0000	1.25
+++ bgpd/bgp_attr.c	19 Nov 2007 23:09:47 -0000
@@ -892,7 +892,8 @@ bgp_attr_as4_path (struct peer *peer, bg
   *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 *p
    */
   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 *p
             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,20 @@ bgp_attr_parse (struct peer *peer, struc
       flag = stream_getc (BGP_INPUT (peer));
       type = stream_getc (BGP_INPUT (peer));
 
+      /* Check whether Extended-Length applies and is in bounds */
+      if ((endp - BGP_INPUT_PNT (peer) < (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));
Index: bgpd/bgp_attr.h
===================================================================
RCS file: /var/cvsroot/quagga/bgpd/bgp_attr.h,v
retrieving revision 1.12
diff -u -p -r1.12 bgp_attr.h
--- bgpd/bgp_attr.h	6 Aug 2007 15:24:51 -0000	1.12
+++ bgpd/bgp_attr.h	19 Nov 2007 23:09:47 -0000
@@ -44,7 +44,7 @@ Software Foundation, Inc., 59 Temple Pla
 #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.
Index: bgpd/bgp_open.c
===================================================================
RCS file: /var/cvsroot/quagga/bgpd/bgp_open.c,v
retrieving revision 1.11
diff -u -p -r1.11 bgp_open.c
--- bgpd/bgp_open.c	14 Oct 2007 22:32:21 -0000	1.11
+++ bgpd/bgp_open.c	19 Nov 2007 23:09:47 -0000
@@ -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,
Index: bgpd/bgp_packet.c
===================================================================
RCS file: /var/cvsroot/quagga/bgpd/bgp_packet.c,v
retrieving revision 1.30
diff -u -p -r1.30 bgp_packet.c
--- bgpd/bgp_packet.c	14 Oct 2007 22:32:21 -0000	1.30
+++ bgpd/bgp_packet.c	19 Nov 2007 23:09:47 -0000
@@ -1960,11 +1960,14 @@ bgp_route_refresh_receive (struct peer *
       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->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);
 
Index: tests/bgp_capability_test.c
===================================================================
RCS file: /var/cvsroot/quagga/tests/bgp_capability_test.c,v
retrieving revision 1.3
diff -u -p -r1.3 bgp_capability_test.c
--- tests/bgp_capability_test.c	14 Oct 2007 22:32:22 -0000	1.3
+++ tests/bgp_capability_test.c	19 Nov 2007 23:09:47 -0000
@@ -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 },


More information about the Quagga-dev mailing list