From: Omar Polo <op@omarpolo.com>
Subject: Re: fix invalid imsg_free() in got_privsep_recv_printed_commits()
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 26 Feb 2024 17:30:59 +0100

On 2024/02/26 16:54:27 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Feb 24, 2024 at 06:46:59PM +0100, Omar Polo wrote:
> > p.s. as a follow-up now that I noticed i think we should swap the order
> > of the checks in got_privsep_recv_imsg(), so to have the GOT_IMSG_ERROR
> > case first, then the length.
> 
> This doesn't make sense to me since the GOT_IMSG_ERROR path uses
> imsg->hdr.len. So it requires a range-check, doesn't it?

This is what I had in mind.  It's not particularly important, but image
a caller of got_privsep_recv_imsg() that's expecting to receive a
struct.  Instead, the libexec process fails and sends a IMSG_GOT_ERROR.

Now, if the requested min_datalen is bigger than a struct
got_imsg_error, we return IMSG_ERR_PRIVSEP_LEN, hiding the real error.

Diff below should be safe since recv_imsg_error itself checks the
datalen before constructing the error.

diff /home/op/w/got
commit - fcece7180725bba9a781eaa892af379b1986208b
path + /home/op/w/got
blob - 208f38064847db1bdf2043d22f6be5691d0905c1
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -141,11 +141,6 @@ got_privsep_recv_imsg(struct imsg *imsg, struct imsgbu
 			return got_error_from_errno("imsg_get");
 	}
 
-	if (imsg->hdr.len < IMSG_HEADER_SIZE + min_datalen) {
-		imsg_free(imsg);
-		return got_error(GOT_ERR_PRIVSEP_LEN);
-	}
-
 	if (imsg->hdr.type == GOT_IMSG_ERROR) {
 		size_t datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
 		err = recv_imsg_error(imsg, datalen);
@@ -153,6 +148,11 @@ got_privsep_recv_imsg(struct imsg *imsg, struct imsgbu
 		return err;
 	}
 
+	if (imsg->hdr.len < IMSG_HEADER_SIZE + min_datalen) {
+		imsg_free(imsg);
+		return got_error(GOT_ERR_PRIVSEP_LEN);
+	}
+
 	return NULL;
 }