On Tue, Oct 16, 2018 at 12:27:33PM +0100, Ricardo Mestre wrote:
> Hi,
>
> xserver's priv proc is responsible for opening devices in O_RDWR mode and send
> their fds over to the parent proc. Knowing this then we already have a list of
> all possible devices that might be opened in the future and we can unveil(2)
> them by traversing allowed_devices and yes it's a long list, but we won't hit
> the limit defined by UNVEIL_MAX_VNODES (currently set to 128). But yes, this
> change might be disputable due to a limitation of vnodes available.
>
> Additionally, by this point we already fork(2)ed so we can drop "proc" promise
> from pledge(2) and I didn't run into any troubles with both these changes.
>
> Comments on either unveil or pledge? OK?
about unveil: it seems fine. open_ok() functions checks if
cmd.arg.open.path is in allowed_devices. so having it locked to only
that seems correct.
about pledge: "proc" isn't only used for fork(2). but also for using
kill(2) for others pids than itself.
in the main loop, the process could have to send USR1 signal to
parent_pid. if it doesn't have "proc" it will be killed.
277 if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
278 err(1, "pledge");
279
280 while (1) {
281 if (read(socks[0], &cmd, sizeof(cmd)) == 0) {
282 exit(0);
283 }
284 switch (cmd.cmd) {
285
286 case PRIV_OPEN_DEVICE:
287 if ((dev = open_ok(cmd.arg.open.path)) != NULL)
{
288 fd = open(cmd.arg.open.path,
dev->flags);
289 } else {
290 fd = -1;
291 errno = EPERM;
292 }
293 send_fd(socks[0], fd);
294 if (fd >= 0)
295 close(fd);
296 break;
297 case PRIV_SIG_PARENT:
298 if (parent_pid > 1)
299 kill(parent_pid, SIGUSR1);
300 break;
301 default:
302 errx(1, "%s: unknown command %d", __func__,
cmd.cmd);
303 break;
304 }
305 }
> Index: privsep.c
> ===================================================================
> RCS file: /cvs/xenocara/xserver/os/privsep.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 privsep.c
> --- privsep.c 6 Aug 2018 20:11:34 -0000 1.29
> +++ privsep.c 16 Oct 2018 10:51:10 -0000
> @@ -274,7 +274,11 @@ priv_init(uid_t uid, gid_t gid)
> setproctitle("[priv]");
> close(socks[1]);
>
> - if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
> + for (dev = allowed_devices; dev->name != NULL; dev++) {
> + if (unveil(dev->name, "rw") == -1)
> + err(1, "unveil");
> + }
> + if (pledge("stdio rpath wpath sendfd", NULL) == -1)
> err(1, "pledge");
>
> while (1) {
>
--
Sebastien Marie
