[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.

diff --git a/bgpd/ChangeLog b/bgpd/ChangeLog
index 3fa3837a3743d9827f7954263744595efb768b68..70bcc0fb98a31e14ecb8cee77d1246582fb16b7b 100644 (file)
--- a/bgpd/ChangeLog
+++ b/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

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index b463b3c0fc36a3b6b29c984a5b752791caeb97d5..dd3cc965a907902db2b0d12533afee45e60555cf 100644 (file)
--- a/bgpd/bgp_attr.c
+++ b/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));

diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index 1af9ce30c8cbc458f2b374ea6e56c6040bfe0b4a..e152b9f42c210c08bcbdbabce605e0ac67b21560 100644 (file)
--- a/bgpd/bgp_attr.h
+++ b/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.

diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c
index 38431d4c63d91875c69663c7a466006a3160de8b..1b13a458746e045adc17b88d64d3e5b1bb620116 100644 (file)
--- a/bgpd/bgp_open.c
+++ b/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,

diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 1fa2fdfde988a7a99a71814e9899196e9320a68f..8319a8853b9633660ec3e99fe0d5c8ea7f5471a1 100644 (file)
--- a/bgpd/bgp_packet.c
+++ b/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);
 

diff --git a/tests/ChangeLog b/tests/ChangeLog
index 94f587492b46b3e8705dcde28d4c358875d8c886..16412bdead5cd2a192468a4cc28efb26366706c9 100644 (file)
--- a/tests/ChangeLog
+++ b/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.

diff --git a/tests/bgp_capability_test.c b/tests/bgp_capability_test.c
index 6771b57998e4485d5ea7a515f1bb0fb5c89ab717..0dbf4fb95615baf548d354e7f1d1b8f4f6eb4646 100644 (file)
--- a/tests/bgp_capability_test.c
+++ b/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 },