From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: bgpd: handle 0-sized objects better
To: tech@openbsd.org
Date: Mon, 9 Sep 2024 16:39:12 +0200

memcpy(foo, NULL, 0) is sadly big no no. Also malloc(0) is not protable
and with that also calloc(0, foo) or reallocarray(foo, 0, bar).

This tries to fix some (most) of those. I know that some of the ASPA bits
need further work. Some of that will be in the next diff.

In aspath_prepend() I'm not sure if it is better to test for
asp->data == NULL instead of the uglier asp->len > shift
Now shift has to be 0 if asp->len == 0 so one could also check for
asp->len != 0.

In the pt_fill case the code in nlri_get_vpn4 ensures that labellen is
non-zero so I decided to fatal there.

-- 
:wq Claudio

Index: rde_attr.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
diff -u -p -r1.134 rde_attr.c
--- rde_attr.c	12 Jul 2023 14:45:43 -0000	1.134
+++ rde_attr.c	9 Sep 2024 13:00:28 -0000
@@ -357,7 +357,8 @@ aspath_get(void *data, uint16_t len)
 	aspath->len = len;
 	aspath->ascnt = aspath_count(data, len);
 	aspath->source_as = aspath_extract_origin(data, len);
-	memcpy(aspath->data, data, len);
+	if (len != 0)
+		memcpy(aspath->data, data, len);
 
 	return (aspath);
 }
@@ -396,7 +397,7 @@ aspath_put(struct aspath *aspath)
 u_char *
 aspath_deflate(u_char *data, uint16_t *len, int *flagnew)
 {
-	uint8_t	*seg, *nseg, *ndata;
+	uint8_t		*seg, *nseg, *ndata = NULL;
 	uint32_t	 as;
 	int		 i;
 	uint16_t	 seg_size, olen, nlen;
@@ -415,6 +416,9 @@ aspath_deflate(u_char *data, uint16_t *l
 			fatalx("%s: would overflow", __func__);
 	}
 
+	if (nlen == 0)
+		goto done;
+
 	if ((ndata = malloc(nlen)) == NULL)
 		fatal("%s", __func__);
 
@@ -437,6 +441,7 @@ aspath_deflate(u_char *data, uint16_t *l
 		}
 	}
 
+ done:
 	*len = nlen;
 	return (ndata);
 }
@@ -791,6 +796,10 @@ aspath_prepend(struct aspath *asp, uint3
 		fatalx("aspath_prepend: preposterous prepend");
 	if (quantum == 0) {
 		/* no change needed but return a copy */
+		if (asp->len == 0) {
+			*len = 0;
+			return (NULL);
+		}
 		p = malloc(asp->len);
 		if (p == NULL)
 			fatal("%s", __func__);
@@ -834,7 +843,8 @@ aspath_prepend(struct aspath *asp, uint3
 			wpos += sizeof(uint32_t);
 		}
 	}
-	memcpy(p + wpos, asp->data + shift, asp->len - shift);
+	if (asp->len > shift)
+		memcpy(p + wpos, asp->data + shift, asp->len - shift);
 
 	*len = l;
 	return (p);
@@ -851,6 +861,11 @@ aspath_override(struct aspath *asp, uint
 	uint32_t	 as;
 	uint16_t	 l, seg_size;
 	uint8_t		 i, seg_len, seg_type;
+
+	if (asp->len == 0) {
+		*len = 0;
+		return (NULL);
+	}
 
 	p = malloc(asp->len);
 	if (p == NULL)
Index: rde_community.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_community.c,v
diff -u -p -r1.15 rde_community.c
--- rde_community.c	24 Jan 2024 14:51:12 -0000	1.15
+++ rde_community.c	9 Sep 2024 13:45:00 -0000
@@ -715,18 +715,19 @@ communities_copy(struct rde_community *t
 	memset(to, 0, sizeof(*to));
 
 	/* ignore from->size and allocate the perfect amount */
-	to->size = from->size;
+	to->size = from->nentries;
 	to->nentries = from->nentries;
 	to->flags = from->flags;
 
+	if (to->nentries == 0)
+		return;
+
 	if ((to->communities = reallocarray(NULL, to->size,
 	    sizeof(struct community))) == NULL)
 		fatal(__func__);
 
 	memcpy(to->communities, from->communities,
 	    to->nentries * sizeof(struct community));
-	memset(to->communities + to->nentries, 0, sizeof(struct community) *
-	    (to->size - to->nentries));
 }
 
 /*
Index: rde_prefix.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_prefix.c,v
diff -u -p -r1.51 rde_prefix.c
--- rde_prefix.c	25 Jun 2024 13:21:18 -0000	1.51
+++ rde_prefix.c	9 Sep 2024 13:17:10 -0000
@@ -225,6 +225,8 @@ pt_fill(struct bgpd_addr *prefix, int pr
 		pte_vpn4.prefixlen = prefixlen;
 		pte_vpn4.rd = prefix->rd;
 		pte_vpn4.labellen = prefix->labellen;
+		if (prefix->labellen == 0)
+			fatalx("pt_fill: no MPLS label in VPN addr");
 		memcpy(pte_vpn4.labelstack, prefix->labelstack,
 		    prefix->labellen);
 		return ((struct pt_entry *)&pte_vpn4);
@@ -239,6 +241,8 @@ pt_fill(struct bgpd_addr *prefix, int pr
 		pte_vpn6.prefixlen = prefixlen;
 		pte_vpn6.rd = prefix->rd;
 		pte_vpn6.labellen = prefix->labellen;
+		if (prefix->labellen == 0)
+			fatalx("pt_fill: no MPLS label in VPN addr");
 		memcpy(pte_vpn6.labelstack, prefix->labelstack,
 		    prefix->labellen);
 		return ((struct pt_entry *)&pte_vpn6);
Index: rde_sets.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_sets.c,v
diff -u -p -r1.12 rde_sets.c
--- rde_sets.c	28 Jul 2022 13:11:51 -0000	1.12
+++ rde_sets.c	9 Sep 2024 13:18:49 -0000
@@ -149,6 +149,9 @@ set_free(struct set_table *set)
 int
 set_add(struct set_table *set, void *elms, size_t nelms)
 {
+	if (nelms == 0)		/* nothing todo */
+		return 0;
+
 	if (set->max < nelms || set->max - nelms < set->nmemb) {
 		uint32_t *s;
 		size_t new_size;