On Tue, Jun 14, 2022 at 11:59:22AM +0200, Omar Polo wrote:
> diff 5b7126c502f8d046d0779dd2cb0558163c383819 /home/op/w/got

I am ok with with the diff, just some small nits:

> blob - a63073982fd405c8f986e41f79764f7b0f74ee7c
> file + lib/privsep.c
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -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;
> +		}

Could this be using GOT_ERR_RANGE instead?

PRIVSEP_LEN refers to the length of the entire imsg, not values
reported within the message. It's fine to use PRIVPSEN_LEN where
a value describes an amount of records that should be present
in the message, but this is not the case here.

> @@ -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);

As above, except GOT_ERR_PACK_OFFSET might also be a valid choice here.

> @@ -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;
> +		}

So in this case, PRIVPSEN_LEN is fine.

> @@ -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);

And it is fine here, too.

> @@ -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),

> -		memcpy(deltas, (uint8_t *)imsg.data + sizeof(ideltas),
> +		memcpy(deltas, (uint8_t *)imsg.data + sizeof(*ideltas),

Nice catch!