unveil xserver's priv proc

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

unveil xserver's priv proc

Ricardo Mestre-2
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 -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) {

Reply | Threaded
Open this post in threaded view
|

Re: unveil xserver's priv proc

Sebastien Marie-3
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

Reply | Threaded
Open this post in threaded view
|

Re: unveil xserver's priv proc

Ricardo Mestre-2
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], &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
>

Reply | Threaded
Open this post in threaded view
|

Re: unveil xserver's priv proc

Ricardo Mestre-2
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 -0000 1.29
+++ privsep.c 24 Oct 2018 09:35:01 -0000
@@ -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");
 

Reply | Threaded
Open this post in threaded view
|

Re: unveil xserver's priv proc

Matthieu Herrb-7
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 -0000 1.29
> +++ privsep.c 24 Oct 2018 09:35:01 -0000
> @@ -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