"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

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

Download raw body.

Thread
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?

diff 50fa49718233a5cfbfc5160b8dd184d7b203ad3a 8e3ba862ea151c560f30f80551b19608c85138f7
blob - b8e60e5a73e4d935fbc621706294df4e664a14d3
blob + 95397e68aa496b96e22f4b0f1f95a15718b2744d
--- 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';