quagga: add upstream security fixes
Fixes the following security issues: CVE-2018-5378 It was discovered that the Quagga BGP daemon, bgpd, does not properly bounds check data sent with a NOTIFY to a peer, if an attribute length is invalid. A configured BGP peer can take advantage of this bug to read memory from the bgpd process or cause a denial of service (daemon crash). https://www.quagga.net/security/Quagga-2018-0543.txt CVE-2018-5379 It was discovered that the Quagga BGP daemon, bgpd, can double-free memory when processing certain forms of UPDATE message, containing cluster-list and/or unknown attributes, resulting in a denial of service (bgpd daemon crash). https://www.quagga.net/security/Quagga-2018-1114.txt CVE-2018-5380 It was discovered that the Quagga BGP daemon, bgpd, does not properly handle internal BGP code-to-string conversion tables. https://www.quagga.net/security/Quagga-2018-1550.txt CVE-2018-5381 It was discovered that the Quagga BGP daemon, bgpd, can enter an infinite loop if sent an invalid OPEN message by a configured peer. A configured peer can take advantage of this flaw to cause a denial of service (bgpd daemon not responding to any other events; BGP sessions will drop and not be reestablished; unresponsive CLI interface). https://www.quagga.net/security/Quagga-2018-1975.txt Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
This commit is contained in:
parent
737278c2cc
commit
157a198d30
|
@ -0,0 +1,69 @@
|
||||||
|
From cc2e6770697e343f4af534114ab7e633d5beabec Mon Sep 17 00:00:00 2001
|
||||||
|
From: Paul Jakma <paul@jakma.org>
|
||||||
|
Date: Wed, 3 Jan 2018 23:57:33 +0000
|
||||||
|
Subject: [PATCH] bgpd/security: invalid attr length sends NOTIFY with data
|
||||||
|
overrun
|
||||||
|
|
||||||
|
Security issue: Quagga-2018-0543
|
||||||
|
|
||||||
|
See: https://www.quagga.net/security/Quagga-2018-0543.txt
|
||||||
|
|
||||||
|
* bgpd/bgp_attr.c: (bgp_attr_parse) An invalid attribute length is correctly
|
||||||
|
checked, and a NOTIFY prepared. The NOTIFY can include the incorrect
|
||||||
|
received data with the NOTIFY, for debug purposes. Commit
|
||||||
|
c69698704806a9ac5 modified the code to do that just, and also send the
|
||||||
|
malformed attr with the NOTIFY. However, the invalid attribute length was
|
||||||
|
used as the length of the data to send back.
|
||||||
|
|
||||||
|
The result is a read past the end of data, which is then written to the
|
||||||
|
NOTIFY message and sent to the peer.
|
||||||
|
|
||||||
|
A configured BGP peer can use this bug to read up to 64 KiB of memory from
|
||||||
|
the bgpd process, or crash the process if the invalid read is caught by
|
||||||
|
some means (unmapped page and SEGV, or other mechanism) resulting in a DoS.
|
||||||
|
|
||||||
|
This bug _ought_ /not/ be exploitable by anything other than the connected
|
||||||
|
BGP peer, assuming the underlying TCP transport is secure. For no BGP
|
||||||
|
peer should send on an UPDATE with this attribute. Quagga will not, as
|
||||||
|
Quagga always validates the attr header length, regardless of type.
|
||||||
|
|
||||||
|
However, it is possible that there are BGP implementations that do not
|
||||||
|
check lengths on some attributes (e.g. optional/transitive ones of a type
|
||||||
|
they do not recognise), and might pass such malformed attrs on. If such
|
||||||
|
implementations exists and are common, then this bug might be triggerable
|
||||||
|
by BGP speakers further hops away. Those peers will not receive the
|
||||||
|
NOTIFY (unless they sit on a shared medium), however they might then be
|
||||||
|
able to trigger a DoS.
|
||||||
|
|
||||||
|
Fix: use the valid bound to calculate the length.
|
||||||
|
|
||||||
|
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
|
||||||
|
---
|
||||||
|
bgpd/bgp_attr.c | 4 +++-
|
||||||
|
1 file changed, 3 insertions(+), 1 deletion(-)
|
||||||
|
|
||||||
|
diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
|
||||||
|
index ef58beb1..9564637e 100644
|
||||||
|
--- a/bgpd/bgp_attr.c
|
||||||
|
+++ b/bgpd/bgp_attr.c
|
||||||
|
@@ -2147,6 +2147,8 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
|
||||||
|
memset (seen, 0, BGP_ATTR_BITMAP_SIZE);
|
||||||
|
|
||||||
|
/* End pointer of BGP attribute. */
|
||||||
|
+ assert (size <= stream_get_size (BGP_INPUT (peer)));
|
||||||
|
+ assert (size <= stream_get_endp (BGP_INPUT (peer)));
|
||||||
|
endp = BGP_INPUT_PNT (peer) + size;
|
||||||
|
|
||||||
|
/* Get attributes to the end of attribute length. */
|
||||||
|
@@ -2228,7 +2230,7 @@ bgp_attr_parse (struct peer *peer, struct attr *attr, bgp_size_t size,
|
||||||
|
bgp_notify_send_with_data (peer,
|
||||||
|
BGP_NOTIFY_UPDATE_ERR,
|
||||||
|
BGP_NOTIFY_UPDATE_ATTR_LENG_ERR,
|
||||||
|
- startp, attr_endp - startp);
|
||||||
|
+ startp, endp - startp);
|
||||||
|
return BGP_ATTR_PARSE_ERROR;
|
||||||
|
}
|
||||||
|
|
||||||
|
--
|
||||||
|
2.11.0
|
||||||
|
|
|
@ -0,0 +1,112 @@
|
||||||
|
From e69b535f92eafb599329bf725d9b4c6fd5d7fded Mon Sep 17 00:00:00 2001
|
||||||
|
From: Paul Jakma <paul@jakma.org>
|
||||||
|
Date: Sat, 6 Jan 2018 19:52:10 +0000
|
||||||
|
Subject: [PATCH] bgpd/security: Fix double free of unknown attribute
|
||||||
|
|
||||||
|
Security issue: Quagga-2018-1114
|
||||||
|
See: https://www.quagga.net/security/Quagga-2018-1114.txt
|
||||||
|
|
||||||
|
It is possible for bgpd to double-free an unknown attribute. This can happen
|
||||||
|
via bgp_update_receive receiving an UPDATE with an invalid unknown attribute.
|
||||||
|
bgp_update_receive then will call bgp_attr_unintern_sub and bgp_attr_flush,
|
||||||
|
and the latter may try free an already freed unknown attr.
|
||||||
|
|
||||||
|
* bgpd/bgp_attr.c: (transit_unintern) Take a pointer to the caller's storage
|
||||||
|
for the (struct transit *), so that transit_unintern can NULL out the
|
||||||
|
caller's reference if the (struct transit) is freed.
|
||||||
|
(cluster_unintern) By inspection, appears to have a similar issue.
|
||||||
|
(bgp_attr_unintern_sub) adjust for above.
|
||||||
|
|
||||||
|
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
|
||||||
|
---
|
||||||
|
bgpd/bgp_attr.c | 33 +++++++++++++++++++--------------
|
||||||
|
bgpd/bgp_attr.h | 4 ++--
|
||||||
|
2 files changed, 21 insertions(+), 16 deletions(-)
|
||||||
|
|
||||||
|
diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
|
||||||
|
index 9564637e..0c2806b5 100644
|
||||||
|
--- a/bgpd/bgp_attr.c
|
||||||
|
+++ b/bgpd/bgp_attr.c
|
||||||
|
@@ -199,15 +199,17 @@ cluster_intern (struct cluster_list *cluster)
|
||||||
|
}
|
||||||
|
|
||||||
|
void
|
||||||
|
-cluster_unintern (struct cluster_list *cluster)
|
||||||
|
+cluster_unintern (struct cluster_list **cluster)
|
||||||
|
{
|
||||||
|
- if (cluster->refcnt)
|
||||||
|
- cluster->refcnt--;
|
||||||
|
+ struct cluster_list *c = *cluster;
|
||||||
|
+ if (c->refcnt)
|
||||||
|
+ c->refcnt--;
|
||||||
|
|
||||||
|
- if (cluster->refcnt == 0)
|
||||||
|
+ if (c->refcnt == 0)
|
||||||
|
{
|
||||||
|
- hash_release (cluster_hash, cluster);
|
||||||
|
- cluster_free (cluster);
|
||||||
|
+ hash_release (cluster_hash, c);
|
||||||
|
+ cluster_free (c);
|
||||||
|
+ *cluster = NULL;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
@@ -357,15 +359,18 @@ transit_intern (struct transit *transit)
|
||||||
|
}
|
||||||
|
|
||||||
|
void
|
||||||
|
-transit_unintern (struct transit *transit)
|
||||||
|
+transit_unintern (struct transit **transit)
|
||||||
|
{
|
||||||
|
- if (transit->refcnt)
|
||||||
|
- transit->refcnt--;
|
||||||
|
+ struct transit *t = *transit;
|
||||||
|
+
|
||||||
|
+ if (t->refcnt)
|
||||||
|
+ t->refcnt--;
|
||||||
|
|
||||||
|
- if (transit->refcnt == 0)
|
||||||
|
+ if (t->refcnt == 0)
|
||||||
|
{
|
||||||
|
- hash_release (transit_hash, transit);
|
||||||
|
- transit_free (transit);
|
||||||
|
+ hash_release (transit_hash, t);
|
||||||
|
+ transit_free (t);
|
||||||
|
+ *transit = NULL;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
@@ -820,11 +825,11 @@ bgp_attr_unintern_sub (struct attr *attr)
|
||||||
|
UNSET_FLAG(attr->flag, ATTR_FLAG_BIT (BGP_ATTR_LARGE_COMMUNITIES));
|
||||||
|
|
||||||
|
if (attr->extra->cluster)
|
||||||
|
- cluster_unintern (attr->extra->cluster);
|
||||||
|
+ cluster_unintern (&attr->extra->cluster);
|
||||||
|
UNSET_FLAG(attr->flag, ATTR_FLAG_BIT (BGP_ATTR_CLUSTER_LIST));
|
||||||
|
|
||||||
|
if (attr->extra->transit)
|
||||||
|
- transit_unintern (attr->extra->transit);
|
||||||
|
+ transit_unintern (&attr->extra->transit);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
|
||||||
|
index 9ff074b2..052acc7d 100644
|
||||||
|
--- a/bgpd/bgp_attr.h
|
||||||
|
+++ b/bgpd/bgp_attr.h
|
||||||
|
@@ -187,10 +187,10 @@ extern unsigned long int attr_unknown_count (void);
|
||||||
|
|
||||||
|
/* Cluster list prototypes. */
|
||||||
|
extern int cluster_loop_check (struct cluster_list *, struct in_addr);
|
||||||
|
-extern void cluster_unintern (struct cluster_list *);
|
||||||
|
+extern void cluster_unintern (struct cluster_list **);
|
||||||
|
|
||||||
|
/* Transit attribute prototypes. */
|
||||||
|
-void transit_unintern (struct transit *);
|
||||||
|
+void transit_unintern (struct transit **);
|
||||||
|
|
||||||
|
/* Below exported for unit-test purposes only */
|
||||||
|
struct bgp_attr_parser_args {
|
||||||
|
--
|
||||||
|
2.11.0
|
||||||
|
|
|
@ -0,0 +1,114 @@
|
||||||
|
From 9e5251151894aefdf8e9392a2371615222119ad8 Mon Sep 17 00:00:00 2001
|
||||||
|
From: Paul Jakma <paul@jakma.org>
|
||||||
|
Date: Sat, 6 Jan 2018 22:31:52 +0000
|
||||||
|
Subject: [PATCH] bgpd/security: debug print of received NOTIFY data can
|
||||||
|
over-read msg array
|
||||||
|
|
||||||
|
Security issue: Quagga-2018-1550
|
||||||
|
See: https://www.quagga.net/security/Quagga-2018-1550.txt
|
||||||
|
|
||||||
|
* bgpd/bgp_debug.c: (struct message) Nearly every one of the NOTIFY
|
||||||
|
code/subcode message arrays has their corresponding size variables off
|
||||||
|
by one, as most have 1 as first index.
|
||||||
|
|
||||||
|
This means (bgp_notify_print) can cause mes_lookup to overread the (struct
|
||||||
|
message) by 1 pointer value if given an unknown index.
|
||||||
|
|
||||||
|
Fix the bgp_notify_..._msg_max variables to use the compiler to calculate
|
||||||
|
the correct sizes.
|
||||||
|
|
||||||
|
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
|
||||||
|
---
|
||||||
|
bgpd/bgp_debug.c | 21 ++++++++++++---------
|
||||||
|
1 file changed, 12 insertions(+), 9 deletions(-)
|
||||||
|
|
||||||
|
diff --git a/bgpd/bgp_debug.c b/bgpd/bgp_debug.c
|
||||||
|
index ba797228..43faee7c 100644
|
||||||
|
--- a/bgpd/bgp_debug.c
|
||||||
|
+++ b/bgpd/bgp_debug.c
|
||||||
|
@@ -29,6 +29,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
|
||||||
|
#include "log.h"
|
||||||
|
#include "sockunion.h"
|
||||||
|
#include "filter.h"
|
||||||
|
+#include "memory.h"
|
||||||
|
|
||||||
|
#include "bgpd/bgpd.h"
|
||||||
|
#include "bgpd/bgp_aspath.h"
|
||||||
|
@@ -73,7 +74,8 @@ const struct message bgp_status_msg[] =
|
||||||
|
{ Clearing, "Clearing" },
|
||||||
|
{ Deleted, "Deleted" },
|
||||||
|
};
|
||||||
|
-const int bgp_status_msg_max = BGP_STATUS_MAX;
|
||||||
|
+#define BGP_DEBUG_MSG_MAX(msg) const int msg ## _max = array_size (msg)
|
||||||
|
+BGP_DEBUG_MSG_MAX (bgp_status_msg);
|
||||||
|
|
||||||
|
/* BGP message type string. */
|
||||||
|
const char *bgp_type_str[] =
|
||||||
|
@@ -84,7 +86,8 @@ const char *bgp_type_str[] =
|
||||||
|
"NOTIFICATION",
|
||||||
|
"KEEPALIVE",
|
||||||
|
"ROUTE-REFRESH",
|
||||||
|
- "CAPABILITY"
|
||||||
|
+ "CAPABILITY",
|
||||||
|
+ NULL,
|
||||||
|
};
|
||||||
|
|
||||||
|
/* message for BGP-4 Notify */
|
||||||
|
@@ -98,15 +101,15 @@ static const struct message bgp_notify_msg[] =
|
||||||
|
{ BGP_NOTIFY_CEASE, "Cease"},
|
||||||
|
{ BGP_NOTIFY_CAPABILITY_ERR, "CAPABILITY Message Error"},
|
||||||
|
};
|
||||||
|
-static const int bgp_notify_msg_max = BGP_NOTIFY_MAX;
|
||||||
|
+BGP_DEBUG_MSG_MAX (bgp_notify_msg);
|
||||||
|
|
||||||
|
static const struct message bgp_notify_head_msg[] =
|
||||||
|
{
|
||||||
|
{ BGP_NOTIFY_HEADER_NOT_SYNC, "/Connection Not Synchronized"},
|
||||||
|
{ BGP_NOTIFY_HEADER_BAD_MESLEN, "/Bad Message Length"},
|
||||||
|
- { BGP_NOTIFY_HEADER_BAD_MESTYPE, "/Bad Message Type"}
|
||||||
|
+ { BGP_NOTIFY_HEADER_BAD_MESTYPE, "/Bad Message Type"},
|
||||||
|
};
|
||||||
|
-static const int bgp_notify_head_msg_max = BGP_NOTIFY_HEADER_MAX;
|
||||||
|
+BGP_DEBUG_MSG_MAX (bgp_notify_head_msg);
|
||||||
|
|
||||||
|
static const struct message bgp_notify_open_msg[] =
|
||||||
|
{
|
||||||
|
@@ -119,7 +122,7 @@ static const struct message bgp_notify_open_msg[] =
|
||||||
|
{ BGP_NOTIFY_OPEN_UNACEP_HOLDTIME, "/Unacceptable Hold Time"},
|
||||||
|
{ BGP_NOTIFY_OPEN_UNSUP_CAPBL, "/Unsupported Capability"},
|
||||||
|
};
|
||||||
|
-static const int bgp_notify_open_msg_max = BGP_NOTIFY_OPEN_MAX;
|
||||||
|
+BGP_DEBUG_MSG_MAX (bgp_notify_open_msg);
|
||||||
|
|
||||||
|
static const struct message bgp_notify_update_msg[] =
|
||||||
|
{
|
||||||
|
@@ -136,7 +139,7 @@ static const struct message bgp_notify_update_msg[] =
|
||||||
|
{ BGP_NOTIFY_UPDATE_INVAL_NETWORK, "/Invalid Network Field"},
|
||||||
|
{ BGP_NOTIFY_UPDATE_MAL_AS_PATH, "/Malformed AS_PATH"},
|
||||||
|
};
|
||||||
|
-static const int bgp_notify_update_msg_max = BGP_NOTIFY_UPDATE_MAX;
|
||||||
|
+BGP_DEBUG_MSG_MAX (bgp_notify_update_msg);
|
||||||
|
|
||||||
|
static const struct message bgp_notify_cease_msg[] =
|
||||||
|
{
|
||||||
|
@@ -150,7 +153,7 @@ static const struct message bgp_notify_cease_msg[] =
|
||||||
|
{ BGP_NOTIFY_CEASE_COLLISION_RESOLUTION, "/Connection collision resolution"},
|
||||||
|
{ BGP_NOTIFY_CEASE_OUT_OF_RESOURCE, "/Out of Resource"},
|
||||||
|
};
|
||||||
|
-static const int bgp_notify_cease_msg_max = BGP_NOTIFY_CEASE_MAX;
|
||||||
|
+BGP_DEBUG_MSG_MAX (bgp_notify_cease_msg);
|
||||||
|
|
||||||
|
static const struct message bgp_notify_capability_msg[] =
|
||||||
|
{
|
||||||
|
@@ -159,7 +162,7 @@ static const struct message bgp_notify_capability_msg[] =
|
||||||
|
{ BGP_NOTIFY_CAPABILITY_INVALID_LENGTH, "/Invalid Capability Length"},
|
||||||
|
{ BGP_NOTIFY_CAPABILITY_MALFORMED_CODE, "/Malformed Capability Value"},
|
||||||
|
};
|
||||||
|
-static const int bgp_notify_capability_msg_max = BGP_NOTIFY_CAPABILITY_MAX;
|
||||||
|
+BGP_DEBUG_MSG_MAX (bgp_notify_capability_msg);
|
||||||
|
|
||||||
|
/* Origin strings. */
|
||||||
|
const char *bgp_origin_str[] = {"i","e","?"};
|
||||||
|
--
|
||||||
|
2.11.0
|
||||||
|
|
|
@ -0,0 +1,43 @@
|
||||||
|
From ce07207c50a3d1f05d6dd49b5294282e59749787 Mon Sep 17 00:00:00 2001
|
||||||
|
From: Paul Jakma <paul@jakma.org>
|
||||||
|
Date: Sat, 6 Jan 2018 21:20:51 +0000
|
||||||
|
Subject: [PATCH] bgpd/security: fix infinite loop on certain invalid OPEN
|
||||||
|
messages
|
||||||
|
|
||||||
|
Security issue: Quagga-2018-1975
|
||||||
|
See: https://www.quagga.net/security/Quagga-2018-1975.txt
|
||||||
|
|
||||||
|
* bgpd/bgp_packet.c: (bgp_capability_msg_parse) capability parser can infinite
|
||||||
|
loop due to checks that issue 'continue' without bumping the input
|
||||||
|
pointer.
|
||||||
|
|
||||||
|
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
|
||||||
|
---
|
||||||
|
bgpd/bgp_packet.c | 4 ++--
|
||||||
|
1 file changed, 2 insertions(+), 2 deletions(-)
|
||||||
|
|
||||||
|
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
|
||||||
|
index b3d601fc..f9338d8d 100644
|
||||||
|
--- a/bgpd/bgp_packet.c
|
||||||
|
+++ b/bgpd/bgp_packet.c
|
||||||
|
@@ -2328,7 +2328,8 @@ bgp_capability_msg_parse (struct peer *peer, u_char *pnt, bgp_size_t length)
|
||||||
|
|
||||||
|
end = pnt + length;
|
||||||
|
|
||||||
|
- while (pnt < end)
|
||||||
|
+ /* XXX: Streamify this */
|
||||||
|
+ for (; pnt < end; pnt += hdr->length + 3)
|
||||||
|
{
|
||||||
|
/* We need at least action, capability code and capability length. */
|
||||||
|
if (pnt + 3 > end)
|
||||||
|
@@ -2416,7 +2417,6 @@ bgp_capability_msg_parse (struct peer *peer, u_char *pnt, bgp_size_t length)
|
||||||
|
zlog_warn ("%s unrecognized capability code: %d - ignored",
|
||||||
|
peer->host, hdr->code);
|
||||||
|
}
|
||||||
|
- pnt += hdr->length + 3;
|
||||||
|
}
|
||||||
|
return 0;
|
||||||
|
}
|
||||||
|
--
|
||||||
|
2.11.0
|
||||||
|
|
Loading…
Reference in a new issue