From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got_privsep_recv_tree() fixes
To: gameoftrees@openbsd.org
Date: Sun, 15 May 2022 15:00:04 +0200

On Sun, May 15, 2022 at 02:47:42PM +0200, Stefan Sperling wrote:
> A couple of fixes for got_privsep_recv_tree().
> 
> The most important fix is at the bottom: We failed to check whether
> the number of tree entries sent by the libexec helper exceeds the
> number of tree entries advertised in the first got_imsg_tree_object
> message. This makes it trivial for the libexec process to make the
> main process write beyond the end of its entries array (heap overflow).
> 
> Besides this, rework the logic to avoid an ugly 'goto', check whether
> the number of entries is >= 0 (this is an int, which gets converted
> to size_t when passed to calloc, where it would become a very large
> number if negative and likely trigger ENOMEM), and free *tree if we
> fail to allocate (*tree)->entries. Freeing *tree via got_object_tree_close()
> below depends on (*tree)->nentries being set up properly, which hasn't
> happened at this point.
> 
> I should probably make separate commits for each fix but don't want
> to bother the mailing list with 4 tiny patches.
> 
> ok?

My previous patch broke tests in cherrypick.sh.

I forgot that a tree with zero entries is a valid case.
There may be an issue for -portable here because we rely on OpenBSD calloc()
behaviour where a unique non-NULL pointer is returned for zero-size
allocations, instead of a NULL pointer. But this can be fixed separately,
and it is not a new problem.

Fixed diff (all tests are passing this time):

diff e45f7eba7c3fe929b6bd5852f390301aeace98aa /home/stsp/src/got
blob - b8e60e5a73e4d935fbc621706294df4e664a14d3
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -1566,7 +1566,7 @@ got_privsep_recv_tree(struct got_tree_object **tree, s
 	int nentries = 0;
 
 	*tree = NULL;
-get_more:
+
 	err = read_imsg(ibuf);
 	if (err)
 		goto done;
@@ -1580,9 +1580,18 @@ get_more:
 
 		n = imsg_get(ibuf, &imsg);
 		if (n == 0) {
-			if (*tree && (*tree)->nentries != nentries)
-				goto get_more;
-			break;
+			if ((*tree)) {
+				if ((*tree)->nentries < nentries) {
+					err = read_imsg(ibuf);
+					if (err)
+						break;
+					continue;
+				} else
+					break;
+			} else {
+				err = got_error(GOT_ERR_PRIVSEP_MSG);
+				break;
+			}
 		}
 
 		if (imsg.hdr.len < IMSG_HEADER_SIZE + min_datalen) {
@@ -1608,6 +1617,10 @@ get_more:
 				break;
 			}
 			itree = imsg.data;
+			if (itree->nentries < 0) {
+				err = got_error(GOT_ERR_PRIVSEP_LEN);
+				break;
+			}
 			*tree = malloc(sizeof(**tree));
 			if (*tree == NULL) {
 				err = got_error_from_errno("malloc");
@@ -1617,6 +1630,8 @@ get_more:
 			    sizeof(struct got_tree_entry));
 			if ((*tree)->entries == NULL) {
 				err = got_error_from_errno("malloc");
+				free(*tree);
+				*tree = NULL;
 				break;
 			}
 			(*tree)->nentries = itree->nentries;
@@ -1645,6 +1660,10 @@ get_more:
 				err = got_error(GOT_ERR_NO_SPACE);
 				break;
 			}
+			if (nentries >= (*tree)->nentries) {
+				err = got_error(GOT_ERR_PRIVSEP_LEN);
+				break;
+			}
 			te = &(*tree)->entries[nentries];
 			memcpy(te->name, imsg.data + sizeof(*ite), datalen);
 			te->name[datalen] = '\0';