From 628565c73d5289e8004abddb6c36b4384083f0f3 Mon Sep 17 00:00:00 2001 From: Quentin Young Date: Sun, 5 Apr 2020 17:11:25 -0400 Subject: [PATCH] bgpd: fix multiple bugs with cluster_list attrs Multiple different issues causing mostly UAFs but maybe other more subtle things. - Cluster lists were the only attributes whose pointers were not being NULL'd when freed, resulting in heap UAF - When performing an insert into the cluster hash, our temporary struct used for hash_get() was inconsistent with our hash keying and comparison functions. In the case of a zero length cluster list, the ->length field is 0 and the ->list field is NULL. When performing an insert, we set the ->list field regardless of whether the length is 0. This resulted in the two cluster lists hashing equal but not comparing equal. Later, when removing one of them from the hash before freeing it, because the key matched and the comparison succeeded (because it was set to NULL *after* the search but *before* inserting into the hash) we would sometimes release the duplicated copy of the struct, and then free the one that remained in the hash table. Later accesses constitute UAF. This is fixed by making sure the fields used for the existence check match what is actually inserted into the hash when that check fails. This patch also makes cluster_unintern static, because it should be. Signed-off-by: Quentin Young --- bgpd/bgp_attr.c | 20 +++++++++++--------- bgpd/bgp_attr.h | 1 - 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index c412ea0710..b3944e5f28 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -119,11 +119,11 @@ static void *cluster_hash_alloc(void *p) /* Cluster list related functions. */ static struct cluster_list *cluster_parse(struct in_addr *pnt, int length) { - struct cluster_list tmp; + struct cluster_list tmp = {}; struct cluster_list *cluster; tmp.length = length; - tmp.list = pnt; + tmp.list = length == 0 ? NULL : pnt; cluster = hash_get(cluster_hash, &tmp, cluster_hash_alloc); cluster->refcnt++; @@ -180,14 +180,16 @@ static struct cluster_list *cluster_intern(struct cluster_list *cluster) return find; } -void cluster_unintern(struct cluster_list *cluster) +static void cluster_unintern(struct cluster_list **cluster) { - if (cluster->refcnt) - cluster->refcnt--; + if ((*cluster)->refcnt) + (*cluster)->refcnt--; - if (cluster->refcnt == 0) { - hash_release(cluster_hash, cluster); - cluster_free(cluster); + if ((*cluster)->refcnt == 0) { + void *p = hash_release(cluster_hash, *cluster); + assert(p == *cluster); + cluster_free(*cluster); + *cluster = NULL; } } @@ -1035,7 +1037,7 @@ void bgp_attr_unintern_sub(struct attr *attr) UNSET_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_LARGE_COMMUNITIES)); if (attr->cluster) - cluster_unintern(attr->cluster); + cluster_unintern(&attr->cluster); UNSET_FLAG(attr->flag, ATTR_FLAG_BIT(BGP_ATTR_CLUSTER_LIST)); if (attr->transit) diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index 6e91957f6a..94531313ae 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -335,7 +335,6 @@ extern unsigned long int attr_unknown_count(void); /* Cluster list prototypes. */ extern bool cluster_loop_check(struct cluster_list *, struct in_addr); -extern void cluster_unintern(struct cluster_list *); /* Below exported for unit-test purposes only */ struct bgp_attr_parser_args { -- 2.39.5