From: Omar Polo <op@omarpolo.com>
Subject: Re: gotd auth unveil
To: Klemens Nanni <kn@openbsd.org>
Cc: gameoftrees@openbsd.org
Date: Mon, 02 Jan 2023 18:59:21 +0100

On 2022/12/30 19:18:55 +0000, Klemens Nanni <kn@openbsd.org> wrote:
> 29.12.2022 23:58, Stefan Sperling пишет:
> > @@ -2371,6 +2371,16 @@ apply_unveil(void)
> >  }
> >  
> >  static void
> > +apply_unveil_none(void)
> > +{
> > +	if (unveil("/", "") == -1)
> > +		fatal("unveil");
> > +
> > +	if (unveil(NULL, NULL) == -1)
> > +		fatal("unveil");
> > +}
> > +
> > +static void
> >  apply_unveil(void)
> >  {
> >  	struct gotd_repo *repo;
> > @@ -2582,9 +2592,17 @@ main(int argc, char **argv)
> >  		break;
> >  	case PROC_AUTH:
> >  #ifndef PROFILE
> 
> Why not simply do the stub unveil here:
> 
> 		unveil("/", "");
> 
> Then you can leave the promises unchanged, since pleding without
> "unveil" immediately after your last unveil is equivalent to
> unveil(NULL, NULL), no?

(sorry for the late reply)

Yes, it is, and I'd agree it's cleaner.

However, in other parts of got you'd find this "pattern" of doing
pledge("... unveil") and then apply_unveil() to lock it.  (see
got/got.c and tog/tog.c for example), so this is consistent with the
rest of the codebase.

> > -		if (pledge("stdio getpw recvfd unix", NULL) == -1)
> > +		if (pledge("stdio getpw recvfd unix unveil", NULL) == -1)
> >  			err(1, "pledge");
> >  #endif
> > +		/*
> > +		 * We need the "unix" pledge promise for getpeername(2) only.
> > +		 * Ensure that AF_UNIX bind(2) cannot be used by revoking all
> > +		 * filesystem access via unveil(2). Access to password database
> > +		 * files will still work since "getpw" bypasses unveil(2).
> > +		 */
> > +		apply_unveil_none();
> > +
> 
> The commit would then just go above the first and only unveil call.
> 
> Since with that diff you are locking unveil immediately, there is no way
> for later code after/inside auth_main() to make unveil further anyway,
> so it should be safe do the pledge-without-unveil-instead-of-NULL-NULL
> idiom here.