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 <qlyoung@cumulusnetworks.com>

diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index c412ea0710f290b6fbea6139b83b739d0b081ded..b3944e5f288aa0fd5a263de091e35075987b6a10 100644 (file)
--- 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 6e91957f6a12e6f92de8dda039fc4c73659dc25c..94531313ae587fc3be1f38a2c666f5dec3154020 100644 (file)
--- 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 {