Re: unveil xserver's priv proc
On Wed, Oct 24, 2018 at 10:36:58AM +0100, Ricardo Mestre wrote:
> Hello,
>
> semarie@ already gave positive feedback for unveiling xserver, did
> anyone tested it yet and comment on it or OK?
Sorry I almost forgot I was running with this patch for some days
now.
ok matthieu@
>
> 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 - 1.29
> +++ privsep.c 24 Oct 2018 09:35:01 -
> @@ -274,6 +274,10 @@ priv_init(uid_t uid, gid_t gid)
> setproctitle("[priv]");
> close(socks[1]);
>
> + for (dev = allowed_devices; dev->name != NULL; dev++) {
> + if (unveil(dev->name, "rw") == -1)
> + err(1, "unveil");
> + }
> if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
> err(1, "pledge");
>
--
Matthieu Herrb
Re: unveil xserver's priv proc
Hello,
semarie@ already gave positive feedback for unveiling xserver, did
anyone tested it yet and comment on it or OK?
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 - 1.29
+++ privsep.c 24 Oct 2018 09:35:01 -
@@ -274,6 +274,10 @@ priv_init(uid_t uid, gid_t gid)
setproctitle("[priv]");
close(socks[1]);
+ for (dev = allowed_devices; dev->name != NULL; dev++) {
+ if (unveil(dev->name, "rw") == -1)
+ err(1, "unveil");
+ }
if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
err(1, "pledge");
Re: unveil xserver's priv proc
Oops I missed the obvious kill(2) only a few lines later, silly me :\
On 13:56 Tue 16 Oct , Sebastien Marie wrote:
> 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], , 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 - 1.29
> > +++ privsep.c 16 Oct 2018 10:51:10 -
> > @@ -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
>
Re: unveil xserver's priv proc
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], , 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 - 1.29
> +++ privsep.c 16 Oct 2018 10:51:10 -
> @@ -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
unveil xserver's priv proc
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?
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 - 1.29
+++ privsep.c 16 Oct 2018 10:51:10 -
@@ -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) {
