29.12.2022 23:58, Stefan Sperling пишет:
> On Thu, Dec 29, 2022 at 07:27:00PM +0100, Stefan Sperling wrote:
>> Remove filesystem access via bind(2) from gotd's auth process.
>> See the added comment for rationale, and keep in mind that
>> AF_UNIX bind(2) requires unveil "w".
>>
>> ok?
> 
> Omar pointed out on IRC that we need to add an unveil rule
> before locking unveil.
> 
> diff 365cf0f34d08316d433e730a8663283029f729b3 fdc9c76e0b23708faf82df8e055ddf5895344150
> commit - 365cf0f34d08316d433e730a8663283029f729b3
> commit + fdc9c76e0b23708faf82df8e055ddf5895344150
> blob - 05f659daea632d0e305556351e4d6a5e97519fa0
> blob + b79d7d9818993976319266976df74331b6ba4d71
> --- gotd/gotd.c
> +++ gotd/gotd.c
> @@ -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?

> -		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.

>  		auth_main(title, &gotd.repos, repo_path);
>  		/* NOTREACHED */
>  		break;
>