[bgpd] Bug #354: Take care to keep reads of MP_(UN)REACH_NLRI in bounds

2007-04-08 Paul Jakma <paul.jakma@sun.com>

	* bgp_attr.c: (general) Bug #354: parsing of MP_REACH_NLRI and
	  MP_UNREACH_NLRI does not take sufficient care to ensure reads
	  from stream buffer stay in-bounds. Hence bgpd may attempt to read
	  beyond end of stream, if given a crafted packet. As it uses the
	  stream access methods to do so, this will typically result in
	  assert() being hit in stream.c. Where code is compiled without
	  assert() enabled, result is unknown.
	  (struct message attr_str) should be static.
	  (bgp_mp_reach_parse) Carefully check length remaining in stream
	  against amount desired to read from stream, prior to each read,
	  particularly where lengths are conditional on data obtained from
	  stream - using STREAM_READABLE.
	  Remove code to parse SNPA-number, it's a defunct field and changed
	  to a fixed size in latest BGP MP update RFC - log warning if
	  SNPA-number is not 0.
	  (bgp_mp_unreach_parse) Check withdraw_length carefully against
	  STREAM_READABLE.
	  (bgp_attr_parse) If attribute-parser function returns error, log
	  warning.
	  Log attribute type on mismatch.

diff --git a/bgpd/ChangeLog b/bgpd/ChangeLog
index 653ce86e24c7c6e230bce81b14cfddee0551ae2f..4d6f1ef9d1c69217b5a3f3acfb51280ec51d60cc 100644 (file)
--- a/bgpd/ChangeLog
+++ b/bgpd/ChangeLog
@@ -7,6 +7,26 @@
          (bgp_update_rsclient) Ignore REMOVED bgp_info for duplicate,
          restore route instead.
          (bgp_update_main) Ditto.
+       * bgp_attr.c: (general) Bug #354: parsing of MP_REACH_NLRI and
+         MP_UNREACH_NLRI does not take sufficient care to ensure reads
+         from stream buffer stay in-bounds. Hence bgpd may attempt to read
+         beyond end of stream, if given a crafted packet. As it uses the
+         stream access methods to do so, this will typically result in
+         assert() being hit in stream.c. Where code is compiled without
+         assert() enabled, result is unknown.
+         (struct message attr_str) should be static.
+         (bgp_mp_reach_parse) Carefully check length remaining in stream
+         against amount desired to read from stream, prior to each read,
+         particularly where lengths are conditional on data obtained from
+         stream - using STREAM_READABLE.
+         Remove code to parse SNPA-number, it's a defunct field and changed
+         to a fixed size in latest BGP MP update RFC - log warning if
+         SNPA-number is not 0.
+         (bgp_mp_unreach_parse) Check withdraw_length carefully against
+         STREAM_READABLE.
+         (bgp_attr_parse) If attribute-parser function returns error, log
+         warning.
+         Log attribute type on mismatch.
 
 2007-04-07 Paul Jakma <paul.jakma@sun.com>
 

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 4c72d80ab7579851821bc287b86b71be04f936e5..fc25d213079eacaa90f42f55295f1d1d73d00d82 100644 (file)
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -39,7 +39,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
 #include "bgpd/bgp_ecommunity.h"
 \f
 /* Attribute strings for logging. */
-struct message attr_str [] = 
+static struct message attr_str [] = 
 {
   { BGP_ATTR_ORIGIN,           "ORIGIN" }, 
   { BGP_ATTR_AS_PATH,          "AS_PATH" }, 
@@ -58,6 +58,7 @@ struct message attr_str [] =
   { BGP_ATTR_MP_UNREACH_NLRI,  "MP_UNREACH_NLRI" },
   { 0, NULL }
 };
+int attr_str_max = sizeof(attr_str)/sizeof(attr_str[0]);
 \f
 struct hash *cluster_hash;
 
@@ -934,24 +935,30 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr,
 {
   u_int16_t afi;
   u_char safi;
-  u_char snpa_num;
-  u_char snpa_len;
-  u_char *lim;
   bgp_size_t nlri_len;
+  size_t start;
   int ret;
   struct stream *s;
   
   /* Set end of packet. */
-  s = peer->ibuf;
-  lim = stream_pnt (s) + length;
-
+  s = BGP_INPUT(peer);
+  start = stream_get_getp(s);
+  
+  /* safe to read statically sized header? */
+#define BGP_MP_REACH_MIN_SIZE 5
+  if ((length > STREAM_READABLE(s)) || (length < BGP_MP_REACH_MIN_SIZE))
+    return -1;
+  
   /* Load AFI, SAFI. */
   afi = stream_getw (s);
   safi = stream_getc (s);
 
   /* Get nexthop length. */
   attr->mp_nexthop_len = stream_getc (s);
-
+  
+  if (STREAM_READABLE(s) < attr->mp_nexthop_len)
+    return -1;
+  
   /* Nexthop length check. */
   switch (attr->mp_nexthop_len)
     {
@@ -997,15 +1004,20 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr,
       return -1;
     }
 
-  snpa_num = stream_getc (s);
-
-  while (snpa_num--)
-    {
-      snpa_len = stream_getc (s);
-      stream_forward_getp (s, (snpa_len + 1) >> 1);
-    }
+  if (!STREAM_READABLE(s))
+    return -1;
   
-  nlri_len = lim - stream_pnt (s);
+  {
+    u_char val; 
+    if ((val = stream_getc (s)))
+    zlog_warn ("%s sent non-zero value, %u, for defunct SNPA-length field",
+                peer->host, val);
+  }
+  
+  /* must have nrli_len, what is left of the attribute */
+  nlri_len = length - (stream_get_getp(s) - start);
+  if ((!nlri_len) || (nlri_len > STREAM_READABLE(s)))
+    return -1;
  
   if (safi != BGP_SAFI_VPNV4)
     {
@@ -1026,23 +1038,25 @@ bgp_mp_reach_parse (struct peer *peer, bgp_size_t length, struct attr *attr,
 
 /* Multiprotocol unreachable parse */
 static int
-bgp_mp_unreach_parse (struct peer *peer, int length, 
+bgp_mp_unreach_parse (struct peer *peer, bgp_size_t length, 
                      struct bgp_nlri *mp_withdraw)
 {
   struct stream *s;
   u_int16_t afi;
   u_char safi;
-  u_char *lim;
   u_int16_t withdraw_len;
   int ret;
 
   s = peer->ibuf;
-  lim = stream_pnt (s) + length;
-
+  
+#define BGP_MP_UNREACH_MIN_SIZE 3
+  if ((length > STREAM_READABLE(s)) || (length <  BGP_MP_UNREACH_MIN_SIZE))
+    return -1;
+  
   afi = stream_getw (s);
   safi = stream_getc (s);
-
-  withdraw_len = lim - stream_pnt (s);
+  
+  withdraw_len = length - BGP_MP_UNREACH_MIN_SIZE;
 
   if (safi != BGP_SAFI_VPNV4)
     {
@@ -1278,13 +1292,23 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
 
       /* If error occured immediately return to the caller. */
       if (ret < 0)
-       return ret;
+        {
+          zlog (peer->log, LOG_WARNING,
+                "%s: Attribute %s, parse error", 
+                peer->host, 
+                LOOKUP (attr_str, type));
+           bgp_notify_send (peer, 
+                            BGP_NOTIFY_UPDATE_ERR,
+                            BGP_NOTIFY_UPDATE_MAL_ATTR);
+           return ret;
+        }
 
       /* Check the fetched length. */
       if (BGP_INPUT_PNT (peer) != attr_endp)
        {
          zlog (peer->log, LOG_WARNING, 
-               "%s BGP attribute fetch error", peer->host);
+               "%s: BGP attribute %s, fetch error", 
+                peer->host, LOOKUP (attr_str, type));
          bgp_notify_send (peer, 
                           BGP_NOTIFY_UPDATE_ERR, 
                           BGP_NOTIFY_UPDATE_ATTR_LENG_ERR);
@@ -1296,7 +1320,8 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
   if (BGP_INPUT_PNT (peer) != endp)
     {
       zlog (peer->log, LOG_WARNING, 
-           "%s BGP attribute length mismatch", peer->host);
+           "%s BGP attribute %s, length mismatch",
+           peer->host, LOOKUP (attr_str, type));
       bgp_notify_send (peer, 
                       BGP_NOTIFY_UPDATE_ERR, 
                       BGP_NOTIFY_UPDATE_ATTR_LENG_ERR);