[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 3fa3837..70bcc0f 100644
--- 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 b463b3c..dd3cc96 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -892,7 +892,8 @@
*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 @@
*/
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 @@
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 @@
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 1af9ce3..e152b9f 100644
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -44,7 +44,7 @@
#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 38431d4..1b13a45 100644
--- a/bgpd/bgp_open.c
+++ b/bgpd/bgp_open.c
@@ -461,7 +461,7 @@
[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 1fa2fdf..8319a88 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -1960,11 +1960,14 @@
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 @@
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 94f5874..16412bd 100644
--- 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 6771b57..0dbf4fb 100644
--- a/tests/bgp_capability_test.c
+++ b/tests/bgp_capability_test.c
@@ -362,6 +362,36 @@
},
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 },