From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got_privsep_recv_tree() fixes
To: gameoftrees@openbsd.org
Date: Mon, 16 May 2022 15:51:35 +0200

On Sun, May 15, 2022 at 03:00:04PM +0200, Stefan Sperling wrote:
> 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):

I found another bug in this patch, where got tree -R failed on a large
repository because of inversed logic in an if-statement. Fixed here.

diff 50fa49718233a5cfbfc5160b8dd184d7b203ad3a c3d22e03793d3ce16ab6954d65ae211ca8953b4d
blob - b8e60e5a73e4d935fbc621706294df4e664a14d3
blob + e7450fd7c2dce123856dba8a632d60b237b54e91
--- 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 (nentries < (*tree)->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';