after noticing some missing checks on the data received from the libexec
helpers in got patch I decided to do some further checks around every
got_privsep_recv_imsg in the rest of got.

(in doing so I've noticed that patch is the only got subsystem to
receive imsgs outside of privsep.c)

what follows are some misc tweaks, but sending three/four different
mails didn't seemed ideal so I'm bundling everything together.

there are a couple of purely stylistic changes (like not checking
datalen before recv_imsg_error or using strndup instead of a custom
malloc + memcpy), some paranoid bounds checks and a genuine fix
(got_privsep_recv_object_idlist/reused_deltas have a typo and are using
the pointer size instead of the object size, but we're lucky and those
are equal in this case.)

I'm pretty confident about most changes, but still not completely sure
about all (in particular the bounds check on the index progress and on
the pack offsets.)

One possible issue of my malloc+memcpy -> strndup change is that if an
author/committer name has a NUL byte in it the old code will preserve
it, the new don't.  Probably isn't an issue but thought of pointing it
out.

The outcome is a shorter privsep.c and hopefully not more bugs :)

diff 5b7126c502f8d046d0779dd2cb0558163c383819 /home/op/w/got
blob - a63073982fd405c8f986e41f79764f7b0f74ee7c
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -355,7 +355,8 @@ got_privsep_recv_raw_obj(uint8_t **outbuf, off_t *size
 			break;
 		}
 
-		if (*size + *hdrlen > GOT_PRIVSEP_INLINE_OBJECT_DATA_MAX) {
+		if (*size < 0 ||
+		    *size + *hdrlen > GOT_PRIVSEP_INLINE_OBJECT_DATA_MAX) {
 			err = got_error(GOT_ERR_PRIVSEP_LEN);
 			break;
 		}
@@ -702,10 +703,6 @@ got_privsep_recv_fetch_progress(int *done, struct got_
 	datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
 	switch (imsg.hdr.type) {
 	case GOT_IMSG_ERROR:
-		if (datalen < sizeof(struct got_imsg_error)) {
-			err = got_error(GOT_ERR_PRIVSEP_LEN);
-			break;
-		}
 		err = recv_imsg_error(&imsg, datalen);
 		break;
 	case GOT_IMSG_FETCH_SYMREFS:
@@ -930,10 +927,6 @@ got_privsep_recv_send_remote_refs(struct got_pathlist_
 		datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
 		switch (imsg.hdr.type) {
 		case GOT_IMSG_ERROR:
-			if (datalen < sizeof(struct got_imsg_error)) {
-				err = got_error(GOT_ERR_PRIVSEP_LEN);
-				goto done;
-			}
 			err = recv_imsg_error(&imsg, datalen);
 			goto done;
 		case GOT_IMSG_SEND_REMOTE_REF:
@@ -1017,10 +1010,6 @@ got_privsep_recv_send_progress(int *done, off_t *bytes
 	datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
 	switch (imsg.hdr.type) {
 	case GOT_IMSG_ERROR:
-		if (datalen < sizeof(struct got_imsg_error)) {
-			err = got_error(GOT_ERR_PRIVSEP_LEN);
-			break;
-		}
 		err = recv_imsg_error(&imsg, datalen);
 		break;
 	case GOT_IMSG_SEND_UPLOAD_PROGRESS:
@@ -1104,10 +1093,6 @@ got_privsep_recv_index_progress(int *done, int *nobj_t
 	datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
 	switch (imsg.hdr.type) {
 	case GOT_IMSG_ERROR:
-		if (datalen < sizeof(struct got_imsg_error)) {
-			err = got_error(GOT_ERR_PRIVSEP_LEN);
-			break;
-		}
 		err = recv_imsg_error(&imsg, datalen);
 		break;
 	case GOT_IMSG_IDXPACK_PROGRESS:
@@ -1116,6 +1101,11 @@ got_privsep_recv_index_progress(int *done, int *nobj_t
 			break;
 		}
 		iprogress = (struct got_imsg_index_pack_progress *)imsg.data;
+		if (iprogress->nobj_total < 0 || iprogress->nobj_indexed < 0 ||
+		    iprogress->nobj_loose < 0 || iprogress->nobj_resolved < 0) {
+			err = got_error(GOT_ERR_PRIVSEP_LEN);
+			break;
+		}
 		*nobj_total = iprogress->nobj_total;
 		*nobj_indexed = iprogress->nobj_indexed;
 		*nobj_loose = iprogress->nobj_loose;
@@ -1148,6 +1138,9 @@ got_privsep_get_imsg_obj(struct got_object **obj, stru
 		return got_error(GOT_ERR_PRIVSEP_LEN);
 	iobj = imsg->data;
 
+	if (iobj->pack_offset < 0)
+		return got_error(GOT_ERR_PRIVSEP_LEN);
+
 	*obj = calloc(1, sizeof(**obj));
 	if (*obj == NULL)
 		return got_error_from_errno("calloc");
@@ -1317,40 +1310,18 @@ get_commit_from_imsg(struct got_commit_object **commit
 	(*commit)->committer_time = icommit->committer_time;
 	(*commit)->committer_gmtoff = icommit->committer_gmtoff;
 
-	if (icommit->author_len == 0) {
-		(*commit)->author = strdup("");
-		if ((*commit)->author == NULL) {
-			err = got_error_from_errno("strdup");
-			goto done;
-		}
-	} else {
-		(*commit)->author = malloc(icommit->author_len + 1);
-		if ((*commit)->author == NULL) {
-			err = got_error_from_errno("malloc");
-			goto done;
-		}
-		memcpy((*commit)->author, imsg->data + len,
-		    icommit->author_len);
-		(*commit)->author[icommit->author_len] = '\0';
+	(*commit)->author = strndup(imsg->data + len, icommit->author_len);
+	if ((*commit)->author == NULL) {
+		err = got_error_from_errno("strndup");
+		goto done;
 	}
 	len += icommit->author_len;
 
-	if (icommit->committer_len == 0) {
-		(*commit)->committer = strdup("");
-		if ((*commit)->committer == NULL) {
-			err = got_error_from_errno("strdup");
-			goto done;
-		}
-	} else {
-		(*commit)->committer =
-		    malloc(icommit->committer_len + 1);
-		if ((*commit)->committer == NULL) {
-			err = got_error_from_errno("malloc");
-			goto done;
-		}
-		memcpy((*commit)->committer, imsg->data + len,
-		    icommit->committer_len);
-		(*commit)->committer[icommit->committer_len] = '\0';
+	(*commit)->committer = strndup(imsg->data + len,
+	    icommit->committer_len);
+	if ((*commit)->committer == NULL) {
+		err = got_error_from_errno("strndup");
+		goto done;
 	}
 	len += icommit->committer_len;
 
@@ -1776,7 +1747,8 @@ got_privsep_recv_blob(uint8_t **outbuf, size_t *size, 
 			break;
 		}
 
-		if (*size > GOT_PRIVSEP_INLINE_BLOB_DATA_MAX) {
+		if (*size > GOT_PRIVSEP_INLINE_BLOB_DATA_MAX ||
+		    *size > datalen + sizeof(*iblob)) {
 			err = got_error(GOT_ERR_PRIVSEP_LEN);
 			break;
 		}
@@ -1917,21 +1889,10 @@ got_privsep_recv_tag(struct got_tag_object **tag, stru
 
 		memcpy((*tag)->id.sha1, itag->id, SHA1_DIGEST_LENGTH);
 
-		if (itag->tag_len == 0) {
-			(*tag)->tag = strdup("");
-			if ((*tag)->tag == NULL) {
-				err = got_error_from_errno("strdup");
-				break;
-			}
-		} else {
-			(*tag)->tag = malloc(itag->tag_len + 1);
-			if ((*tag)->tag == NULL) {
-				err = got_error_from_errno("malloc");
-				break;
-			}
-			memcpy((*tag)->tag, imsg.data + len,
-			    itag->tag_len);
-			(*tag)->tag[itag->tag_len] = '\0';
+		(*tag)->tag = strndup(imsg.data + len, itag->tag_len);
+		if ((*tag)->tag == NULL) {
+			err = got_error_from_errno("strndup");
+			break;
 		}
 		len += itag->tag_len;
 
@@ -1939,21 +1900,10 @@ got_privsep_recv_tag(struct got_tag_object **tag, stru
 		(*tag)->tagger_time = itag->tagger_time;
 		(*tag)->tagger_gmtoff = itag->tagger_gmtoff;
 
-		if (itag->tagger_len == 0) {
-			(*tag)->tagger = strdup("");
-			if ((*tag)->tagger == NULL) {
-				err = got_error_from_errno("strdup");
-				break;
-			}
-		} else {
-			(*tag)->tagger = malloc(itag->tagger_len + 1);
-			if ((*tag)->tagger == NULL) {
-				err = got_error_from_errno("malloc");
-				break;
-			}
-			memcpy((*tag)->tagger, imsg.data + len,
-			    itag->tagger_len);
-			(*tag)->tagger[itag->tagger_len] = '\0';
+		(*tag)->tagger = strndup(imsg.data + len, itag->tagger_len);
+		if ((*tag)->tagger == NULL) {
+			err = got_error_from_errno("strndup");
+			break;
 		}
 		len += itag->tagger_len;
 
@@ -2169,11 +2119,10 @@ got_privsep_recv_gitconfig_str(char **str, struct imsg
 	const struct got_error *err = NULL;
 	struct imsg imsg;
 	size_t datalen;
-	const size_t min_datalen = 0;
 
 	*str = NULL;
 
-	err = got_privsep_recv_imsg(&imsg, ibuf, min_datalen);
+	err = got_privsep_recv_imsg(&imsg, ibuf, 0);
 	if (err)
 		return err;
 	datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
@@ -2415,21 +2364,16 @@ got_privsep_recv_gotconfig_str(char **str, struct imsg
 	const struct got_error *err = NULL;
 	struct imsg imsg;
 	size_t datalen;
-	const size_t min_datalen = 0;
 
 	*str = NULL;
 
-	err = got_privsep_recv_imsg(&imsg, ibuf, min_datalen);
+	err = got_privsep_recv_imsg(&imsg, ibuf, 0);
 	if (err)
 		return err;
 	datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
 
 	switch (imsg.hdr.type) {
 	case GOT_IMSG_ERROR:
-		if (datalen < sizeof(struct got_imsg_error)) {
-			err = got_error(GOT_ERR_PRIVSEP_LEN);
-			break;
-		}
 		err = recv_imsg_error(&imsg, datalen);
 		break;
 	case GOT_IMSG_GOTCONFIG_STR_VAL:
@@ -2476,10 +2420,6 @@ got_privsep_recv_gotconfig_remotes(struct got_remote_r
 
 	switch (imsg.hdr.type) {
 	case GOT_IMSG_ERROR:
-		if (datalen < sizeof(struct got_imsg_error)) {
-			err = got_error(GOT_ERR_PRIVSEP_LEN);
-			break;
-		}
 		err = recv_imsg_error(&imsg, datalen);
 		break;
 	case GOT_IMSG_GOTCONFIG_REMOTES:
@@ -2488,6 +2428,10 @@ got_privsep_recv_gotconfig_remotes(struct got_remote_r
 			break;
 		}
 		memcpy(&iremotes, imsg.data, sizeof(iremotes));
+		if (iremotes.nremotes < 0) {
+			err = got_error(GOT_ERR_PRIVSEP_LEN);
+			break;
+		}
 		if (iremotes.nremotes == 0) {
 			imsg_free(&imsg);
 			return NULL;
@@ -2517,10 +2461,6 @@ got_privsep_recv_gotconfig_remotes(struct got_remote_r
 
 		switch (imsg.hdr.type) {
 		case GOT_IMSG_ERROR:
-			if (datalen < sizeof(struct got_imsg_error)) {
-				err = got_error(GOT_ERR_PRIVSEP_LEN);
-				break;
-			}
 			err = recv_imsg_error(&imsg, datalen);
 			break;
 		case GOT_IMSG_GOTCONFIG_REMOTE:
@@ -2862,7 +2802,7 @@ got_privsep_recv_enumerated_objects(struct imsgbuf *ib
 
 		datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
 		switch (imsg.hdr.type) {
-		case GOT_IMSG_ENUMERATED_COMMIT: 
+		case GOT_IMSG_ENUMERATED_COMMIT:
 			if (have_commit && nentries != -1) {
 				err = got_error(GOT_ERR_PRIVSEP_MSG);
 				break;
@@ -3219,7 +3159,8 @@ got_privsep_recv_object_idlist(int *done, struct got_o
 			break;
 		}
 		idlist = imsg.data;
-		if (idlist->nids > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS) {
+		if (idlist->nids > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS ||
+		    idlist->nids * sizeof(**ids) > datalen - sizeof(*idlist)) {
 			err = got_error(GOT_ERR_PRIVSEP_LEN);
 			break;
 		}
@@ -3229,7 +3170,7 @@ got_privsep_recv_object_idlist(int *done, struct got_o
 			err = got_error_from_errno("calloc");
 			break;
 		}
-		memcpy(*ids, (uint8_t *)imsg.data + sizeof(idlist),
+		memcpy(*ids, (uint8_t *)imsg.data + sizeof(*idlist),
 		    *nids * sizeof(**ids));
 		break;
 	case GOT_IMSG_OBJ_ID_LIST_DONE:
@@ -3324,12 +3265,14 @@ got_privsep_recv_reused_deltas(int *done, struct got_i
 			break;
 		}
 		ideltas = imsg.data;
-		if (ideltas->ndeltas > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS) {
+		if (ideltas->ndeltas > GOT_IMSG_OBJ_ID_LIST_MAX_NIDS ||
+		    ideltas->ndeltas * sizeof(*deltas) >
+		    datalen - sizeof(*ideltas)) {
 			err = got_error(GOT_ERR_PRIVSEP_LEN);
 			break;
 		}
 		*ndeltas = ideltas->ndeltas;
-		memcpy(deltas, (uint8_t *)imsg.data + sizeof(ideltas),
+		memcpy(deltas, (uint8_t *)imsg.data + sizeof(*ideltas),
 		    *ndeltas * sizeof(*deltas));
 		break;
 	case GOT_IMSG_DELTA_REUSE_DONE: