From: Omar Polo <op@omarpolo.com>
Subject: Re: use unveil in gotwebd sockets proc
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 14 Nov 2023 11:48:27 +0100

On 2023/11/13 22:23:28 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> The gotwebd sockets processes don't use unveil at present.
> The patch below unveils what I believe is needed, and it seems to work
> for me in some manual testing.
> 
> The call to got_privsep_unveil_exec_helpers() will unveil some programs
> that do not exist in the chroot environment, such as got-send-pack.
> Not a big deal, but perhaps we should add a gotwebd-specific version
> of this function at some point?
> 
> ok?

two nits below, ok op@ otherwise

> diff /home/stsp/src/got
> commit - b1c090542f4ecaf993fc81468338839febcb8e37
> path + /home/stsp/src/got
> blob - 341d3774c799acfb106876120fa0e5ae0b9131c0
> file + gotwebd/sockets.c
> --- gotwebd/sockets.c
> +++ gotwebd/sockets.c
> @@ -53,6 +53,7 @@
>  #include "got_opentemp.h"
>  #include "got_reference.h"
>  #include "got_repository.h"
> +#include "got_privsep.h"
>  
>  #include "proc.h"
>  #include "gotwebd.h"
> @@ -112,8 +113,8 @@ sockets_run(struct privsep *ps, struct privsep_proc *p
>  	signal_add(&ps->ps_evsigchld, NULL);
>  
>  #ifndef PROFILE
> -	if (pledge("stdio rpath wpath cpath inet recvfd proc exec sendfd",
> -	    NULL) == -1)
> +	if (pledge("stdio rpath wpath cpath inet recvfd proc exec sendfd "
> +	    "unveil", NULL) == -1)
>  		fatal("pledge");
>  #endif
>  }
> @@ -246,6 +247,8 @@ static void
>  sockets_launch(void)
>  {
>  	struct socket *sock;
> +	struct server *srv;
> +	const struct got_error *error;
>  
>  	TAILQ_FOREACH(sock, &gotwebd_env->sockets, entry) {
>  		log_debug("%s: configuring socket %d (%d)", __func__,
> @@ -262,6 +265,21 @@ sockets_launch(void)
>  		log_debug("%s: running socket listener %d", __func__,
>  		    sock->conf.id);
>  	}
> +
> +	TAILQ_FOREACH(srv, &gotwebd_env->servers, entry) {
> +		if (unveil(srv->repos_path, "r") == -1)
> +			fatal("unveil %s", srv->repos_path);
> +	}
> +
> +	if (unveil(GOT_TMPDIR_STR, "rwc") == -1)
> +		fatal("unveil " GOT_TMPDIR_STR);

don't need GOT_TMPDIR_STR here neither.  the sockets process is the one
serving the request, which will receive the tempfile via imsg.

> +	error = got_privsep_unveil_exec_helpers();
> +	if (error)
> +		fatal("%s", error->msg);
> +
> +	if (unveil(NULL, NULL) != 0)

nit: i'd prefer to keep the unveil(...) == -1 idiom instead of != 0.

> +		fatal("unveil");
>  }
>  
>  static void