Re: disable fs access on snmpd

2018-10-30 Thread Theo de Raadt
Looks right.


Ricardo Mestre  wrote:

> snmpd(8)'s main process needs to open the config file and /dev/pf both with
> read permissions, but once it reaches pledge(2) just before the main loop both
> were already opened. Since snmpd(8) doesn't have a way to load or reload the
> config file, not even through SIGHUP, then rpath promise is not needed.
> 
> The snmpe process cannot yet be pledged, but it doesn't need fs access so we
> can disable the access through unveil("/", ""); unveil(NULL, NULL);
> 
> The traphandler is already pledged to not access the fs at all.
> 
> With both modifications the regress tests still pass. Comments? OK?
> 
> Index: snmpd.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmpd.c,v
> retrieving revision 1.39
> diff -u -p -u -r1.39 snmpd.c
> --- snmpd.c   5 Aug 2018 09:33:13 -   1.39
> +++ snmpd.c   30 Oct 2018 14:03:38 -
> @@ -255,7 +255,7 @@ main(int argc, char *argv[])
>  
>   proc_connect(ps);
>  
> - if (pledge("stdio rpath dns sendfd proc exec id", NULL) == -1)
> + if (pledge("stdio dns sendfd proc exec id", NULL) == -1)
>   fatal("pledge");
>  
>   event_dispatch();
> Index: snmpe.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
> retrieving revision 1.54
> diff -u -p -u -r1.54 snmpe.c
> --- snmpe.c   31 Jul 2018 11:01:29 -  1.54
> +++ snmpe.c   30 Oct 2018 14:03:38 -
> @@ -120,6 +120,10 @@ snmpe_init(struct privsep *ps, struct pr
>   event_add(&so->s_ev, NULL);
>   }
>  
> + if (unveil("/", "") == -1)
> + fatal("unveil");
> + if (unveil(NULL, NULL) == -1)
> + fatal("unveil");
>  #if 0
>   /*
>* XXX Refactoring required to move illegal ioctls and sysctls.



disable fs access on snmpd

2018-10-30 Thread Ricardo Mestre
Hi,

snmpd(8)'s main process needs to open the config file and /dev/pf both with
read permissions, but once it reaches pledge(2) just before the main loop both
were already opened. Since snmpd(8) doesn't have a way to load or reload the
config file, not even through SIGHUP, then rpath promise is not needed.

The snmpe process cannot yet be pledged, but it doesn't need fs access so we
can disable the access through unveil("/", ""); unveil(NULL, NULL);

The traphandler is already pledged to not access the fs at all.

With both modifications the regress tests still pass. Comments? OK?

Index: snmpd.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpd.c,v
retrieving revision 1.39
diff -u -p -u -r1.39 snmpd.c
--- snmpd.c 5 Aug 2018 09:33:13 -   1.39
+++ snmpd.c 30 Oct 2018 14:03:38 -
@@ -255,7 +255,7 @@ main(int argc, char *argv[])
 
proc_connect(ps);
 
-   if (pledge("stdio rpath dns sendfd proc exec id", NULL) == -1)
+   if (pledge("stdio dns sendfd proc exec id", NULL) == -1)
fatal("pledge");
 
event_dispatch();
Index: snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.54
diff -u -p -u -r1.54 snmpe.c
--- snmpe.c 31 Jul 2018 11:01:29 -  1.54
+++ snmpe.c 30 Oct 2018 14:03:38 -
@@ -120,6 +120,10 @@ snmpe_init(struct privsep *ps, struct pr
event_add(&so->s_ev, NULL);
}
 
+   if (unveil("/", "") == -1)
+   fatal("unveil");
+   if (unveil(NULL, NULL) == -1)
+   fatal("unveil");
 #if 0
/*
 * XXX Refactoring required to move illegal ioctls and sysctls.