unveil(2) sysctl(8)

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

unveil(2) sysctl(8)

Ricardo Mestre-2
Hi,

My eyes may be cheating me in plain sight, but sysctl(8) doesn't seem to
require fs access at all.

Comments? OK?

Index: sysctl.c
===================================================================
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.242
diff -u -p -u -r1.242 sysctl.c
--- sysctl.c 13 May 2019 20:47:19 -0000 1.242
+++ sysctl.c 7 Jun 2019 17:01:23 -0000
@@ -255,6 +255,11 @@ main(int argc, char *argv[])
  argc -= optind;
  argv += optind;
 
+ if (unveil("/", "") == -1)
+    err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+    err(1, "unveil");
+
  if (argc == 0 || (Aflag || aflag)) {
  debuginit();
  vfsinit();

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Consus-2
On 18:14 Fri 07 Jun, Ricardo Mestre wrote:

> Hi,
>
> My eyes may be cheating me in plain sight, but sysctl(8) doesn't seem to
> require fs access at all.
>
> Comments? OK?
>
> Index: sysctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.242
> diff -u -p -u -r1.242 sysctl.c
> --- sysctl.c 13 May 2019 20:47:19 -0000 1.242
> +++ sysctl.c 7 Jun 2019 17:01:23 -0000
> @@ -255,6 +255,11 @@ main(int argc, char *argv[])
>   argc -= optind;
>   argv += optind;
>  
> + if (unveil("/", "") == -1)
> +    err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> +    err(1, "unveil");
> +
>   if (argc == 0 || (Aflag || aflag)) {
>   debuginit();
>   vfsinit();
>

$ ktrace sysctl >/dev/null; kdump 2>&1 | grep -c 'CALL  open'
3

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Ricardo Mestre-2
i did that and for some for reason i didn't get it! it tries to open
timezone so it kinda looks like a red flag right there...

apart from /dev do we need to look into TZ on this one as well? if TZ
var needs to be looked at then all bets are off :/

On 01:01 Sat 08 Jun     , Consus wrote:

> On 18:14 Fri 07 Jun, Ricardo Mestre wrote:
> > Hi,
> >
> > My eyes may be cheating me in plain sight, but sysctl(8) doesn't seem to
> > require fs access at all.
> >
> > Comments? OK?
> >
> > Index: sysctl.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> > retrieving revision 1.242
> > diff -u -p -u -r1.242 sysctl.c
> > --- sysctl.c 13 May 2019 20:47:19 -0000 1.242
> > +++ sysctl.c 7 Jun 2019 17:01:23 -0000
> > @@ -255,6 +255,11 @@ main(int argc, char *argv[])
> >   argc -= optind;
> >   argv += optind;
> >  
> > + if (unveil("/", "") == -1)
> > +    err(1, "unveil");
> > + if (unveil(NULL, NULL) == -1)
> > +    err(1, "unveil");
> > +
> >   if (argc == 0 || (Aflag || aflag)) {
> >   debuginit();
> >   vfsinit();
> >
>
> $ ktrace sysctl >/dev/null; kdump 2>&1 | grep -c 'CALL  open'
> 3
>

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Florian Obser-2
On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> i did that and for some for reason i didn't get it! it tries to open
> timezone so it kinda looks like a red flag right there...
>
> apart from /dev do we need to look into TZ on this one as well? if TZ
> var needs to be looked at then all bets are off :/

this seems to do the right thing:

diff --git sysctl.c sysctl.c
index dc6abc16670..c74e706942a 100644
--- sysctl.c
+++ sysctl.c
@@ -255,6 +255,13 @@ main(int argc, char *argv[])
  argc -= optind;
  argv += optind;
 
+ tzset(); /* for kern.boottime in localtime */
+
+ if (unveil("/dev", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
  if (argc == 0 || (Aflag || aflag)) {
  debuginit();
  vfsinit();


--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Jeremie Courreges-Anglas-2
On Sat, Jun 08 2019, Florian Obser <[hidden email]> wrote:

> On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
>> i did that and for some for reason i didn't get it! it tries to open
>> timezone so it kinda looks like a red flag right there...
>>
>> apart from /dev do we need to look into TZ on this one as well? if TZ
>> var needs to be looked at then all bets are off :/
>
> this seems to do the right thing:
>
> diff --git sysctl.c sysctl.c
> index dc6abc16670..c74e706942a 100644
> --- sysctl.c
> +++ sysctl.c
> @@ -255,6 +255,13 @@ main(int argc, char *argv[])
>   argc -= optind;
>   argv += optind;
>  
> + tzset(); /* for kern.boottime in localtime */
> +
> + if (unveil("/dev", "r") == -1)
> + err(1, "unveil");

sysctl(8) uses devname(3); the latter wants to use /var/run/dev.db and
then falls back on listing /dev.  Both /var/run/dev.db and /dev should
be whitelisted I think.

--8<--
  3461 sysctl   CALL  open(0x7fad42c9eeb,0x10000<O_RDONLY|O_CLOEXEC>)
  3461 sysctl   NAMI  "/var/run/dev.db"
  3461 sysctl   RET   open -1 errno 2 No such file or directory
  3461 sysctl   CALL  open(0x7fad42c8f19,0x30000<O_RDONLY|O_CLOEXEC|O_DIRECTORY>)
  3461 sysctl   NAMI  "/dev/"
  3461 sysctl   RET   open 3

-->8--

Naive question: should sysctl(8) be pledged?  I'm not objecting but
I wonder what we are trying to protect us from, here.

> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
> +
>   if (argc == 0 || (Aflag || aflag)) {
>   debuginit();
>   vfsinit();

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Florian Obser-2
On Sat, Jun 08, 2019 at 01:15:37PM +0200, Jeremie Courreges-Anglas wrote:

> On Sat, Jun 08 2019, Florian Obser <[hidden email]> wrote:
> > On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> >> i did that and for some for reason i didn't get it! it tries to open
> >> timezone so it kinda looks like a red flag right there...
> >>
> >> apart from /dev do we need to look into TZ on this one as well? if TZ
> >> var needs to be looked at then all bets are off :/
> >
> > this seems to do the right thing:
> >
> > diff --git sysctl.c sysctl.c
> > index dc6abc16670..c74e706942a 100644
> > --- sysctl.c
> > +++ sysctl.c
> > @@ -255,6 +255,13 @@ main(int argc, char *argv[])
> >   argc -= optind;
> >   argv += optind;
> >  
> > + tzset(); /* for kern.boottime in localtime */
> > +
> > + if (unveil("/dev", "r") == -1)
> > + err(1, "unveil");
>
> sysctl(8) uses devname(3); the latter wants to use /var/run/dev.db and
> then falls back on listing /dev.  Both /var/run/dev.db and /dev should
> be whitelisted I think.

nice catch. sure.

>
> --8<--
>   3461 sysctl   CALL  open(0x7fad42c9eeb,0x10000<O_RDONLY|O_CLOEXEC>)
>   3461 sysctl   NAMI  "/var/run/dev.db"
>   3461 sysctl   RET   open -1 errno 2 No such file or directory
>   3461 sysctl   CALL  open(0x7fad42c8f19,0x30000<O_RDONLY|O_CLOEXEC|O_DIRECTORY>)
>   3461 sysctl   NAMI  "/dev/"
>   3461 sysctl   RET   open 3
>
> -->8--
>
> Naive question: should sysctl(8) be pledged?  I'm not objecting but
> I wonder what we are trying to protect us from, here.

shrug, I was just passive aggressively reacting to some unhelpful
asshattery.

In more general terms when working with pledge / unveil I'm not
interested in listing (exhaustively) things we need to protect
against. I try to come up with the minimal set of privileges.
That seems to be easier, at least for me.

Why is cat(1) pledged? It has 197 loc, surely a careful audit will
find all bugs... Maybe... Who knows...

So I don't have an answer to your question, but I also don't find it
interesting :)

>
> > + if (unveil(NULL, NULL) == -1)
> > + err(1, "unveil");
> > +
> >   if (argc == 0 || (Aflag || aflag)) {
> >   debuginit();
> >   vfsinit();
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Stuart Henderson
In reply to this post by Jeremie Courreges-Anglas-2
On 2019/06/08 13:15, Jeremie Courreges-Anglas wrote:
> Naive question: should sysctl(8) be pledged?  I'm not objecting but
> I wonder what we are trying to protect us from, here.

AFAIK it can't be done because pledge always restricts which sysctl nodes
may be accessed.

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Jeremie Courreges-Anglas-2
On Sat, Jun 08 2019, Stuart Henderson <[hidden email]> wrote:
> On 2019/06/08 13:15, Jeremie Courreges-Anglas wrote:
>> Naive question: should sysctl(8) be pledged?  I'm not objecting but
>> I wonder what we are trying to protect us from, here.
>
> AFAIK it can't be done because pledge always restricts which sysctl nodes
> may be accessed.

Duh, I'll blame it on coffee deprivation...  What I had in mind was:

  "Naive question: should sysctl(8) use unveil(2)?"

Sorry about that.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Florian Obser-2
In reply to this post by Florian Obser-2
On Sat, Jun 08, 2019 at 01:42:04PM +0200, Florian Obser wrote:
> shrug, I was just passive aggressively reacting to some unhelpful
> asshattery.

To clarify: This was not aimed at mestre@.

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Theo de Raadt-2
In reply to this post by Florian Obser-2
When userland was massaged for pledge(), I hesitated using the
"manually call tzset()" approach for handling things.  It felt
too low-level to call tzset(), an API almost noone knows the
existance of.

Arriving in the same situation to satisfy unveil().  Again calling
tzset() feels too unfamiliar and low level.

Regarding the comment in your diff, it says "localtime", but what is
actually called is ctime(), which calls localtime() (which calls
tzset(), which is where the unveil-files-missing or pledge-whatver
issues would show up in some programs).  Probably should adjust
the comment

Here's the later troublesome chunk:

        if (special & BOOTTIME) {
                struct timeval *btp = (struct timeval *)buf;
                time_t boottime;

                if (!nflag) {
                        boottime = btp->tv_sec;
                        (void)printf("%s%s%s", string, equ, ctime(&boottime));

That makes me wonder, can we be less obtuse up front, and
prime the subsystem before unveil by calling the same function which
will be called later?

Something like this.  It feels slightly better to me.

Index: sysctl.c
===================================================================
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.242
diff -u -p -u -r1.242 sysctl.c
--- sysctl.c 13 May 2019 20:47:19 -0000 1.242
+++ sysctl.c 8 Jun 2019 16:33:07 -0000
@@ -162,6 +162,8 @@ struct list secondlevel[] = {
 
 int Aflag, aflag, nflag, qflag;
 
+time_t boottime;
+
 /*
  * Variables requiring special processing.
  */
@@ -255,6 +257,12 @@ main(int argc, char *argv[])
  argc -= optind;
  argv += optind;
 
+ ctime(&boottime); /* satisfy potential $TZ expansion before unveil() */
+
+ if (unveil("/dev", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
  if (argc == 0 || (Aflag || aflag)) {
  debuginit();
  vfsinit();
@@ -893,7 +901,6 @@ parse(char *string, int flags)
  }
  if (special & BOOTTIME) {
  struct timeval *btp = (struct timeval *)buf;
- time_t boottime;
 
  if (!nflag) {
  boottime = btp->tv_sec;





Florian Obser <[hidden email]> wrote:

> On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> > i did that and for some for reason i didn't get it! it tries to open
> > timezone so it kinda looks like a red flag right there...
> >
> > apart from /dev do we need to look into TZ on this one as well? if TZ
> > var needs to be looked at then all bets are off :/
>
> this seems to do the right thing:
>
> diff --git sysctl.c sysctl.c
> index dc6abc16670..c74e706942a 100644
> --- sysctl.c
> +++ sysctl.c
> @@ -255,6 +255,13 @@ main(int argc, char *argv[])
>   argc -= optind;
>   argv += optind;
>  
> + tzset(); /* for kern.boottime in localtime */
> +
> + if (unveil("/dev", "r") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
> +
>   if (argc == 0 || (Aflag || aflag)) {
>   debuginit();
>   vfsinit();
>
>
> --
> I'm not entirely sure you are real.
>

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Ricardo Mestre-2
Hi,

Sorry to be late in the game, but as jca@ pointed out sysctl(8) tries to
open _PATH_DEVDB first and then /dev if it cannot open the former, so
both should be unveil(2)ed. Scramble the includes while at it.

Index: sysctl.c
===================================================================
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.242
diff -u -p -u -r1.242 sysctl.c
--- sysctl.c 13 May 2019 20:47:19 -0000 1.242
+++ sysctl.c 14 Jun 2019 19:04:01 -0000
@@ -94,13 +94,14 @@
 #include <ddb/db_var.h>
 #include <dev/rndvar.h>
 
+#include <ctype.h>
 #include <err.h>
 #include <errno.h>
+#include <limits.h>
+#include <paths.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <ctype.h>
-#include <limits.h>
 #include <unistd.h>
 
 #include <machine/cpu.h>
@@ -162,6 +163,8 @@ struct list secondlevel[] = {
 
 int Aflag, aflag, nflag, qflag;
 
+time_t boottime;
+
 /*
  * Variables requiring special processing.
  */
@@ -255,6 +258,15 @@ main(int argc, char *argv[])
  argc -= optind;
  argv += optind;
 
+ ctime(&boottime); /* satisfy potential $TZ expansion before unveil() */
+
+ if (unveil(_PATH_DEVDB, "r") == -1)
+ err(1,"unveil");
+ if (unveil("/dev", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
  if (argc == 0 || (Aflag || aflag)) {
  debuginit();
  vfsinit();
@@ -893,7 +905,6 @@ parse(char *string, int flags)
  }
  if (special & BOOTTIME) {
  struct timeval *btp = (struct timeval *)buf;
- time_t boottime;
 
  if (!nflag) {
  boottime = btp->tv_sec;

On 10:35 Sat 08 Jun     , Theo de Raadt wrote:

> When userland was massaged for pledge(), I hesitated using the
> "manually call tzset()" approach for handling things.  It felt
> too low-level to call tzset(), an API almost noone knows the
> existance of.
>
> Arriving in the same situation to satisfy unveil().  Again calling
> tzset() feels too unfamiliar and low level.
>
> Regarding the comment in your diff, it says "localtime", but what is
> actually called is ctime(), which calls localtime() (which calls
> tzset(), which is where the unveil-files-missing or pledge-whatver
> issues would show up in some programs).  Probably should adjust
> the comment
>
> Here's the later troublesome chunk:
>
>         if (special & BOOTTIME) {
>                 struct timeval *btp = (struct timeval *)buf;
>                 time_t boottime;
>
>                 if (!nflag) {
>                         boottime = btp->tv_sec;
>                         (void)printf("%s%s%s", string, equ, ctime(&boottime));
>
> That makes me wonder, can we be less obtuse up front, and
> prime the subsystem before unveil by calling the same function which
> will be called later?
>
> Something like this.  It feels slightly better to me.
>
> Index: sysctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.242
> diff -u -p -u -r1.242 sysctl.c
> --- sysctl.c 13 May 2019 20:47:19 -0000 1.242
> +++ sysctl.c 8 Jun 2019 16:33:07 -0000
> @@ -162,6 +162,8 @@ struct list secondlevel[] = {
>  
>  int Aflag, aflag, nflag, qflag;
>  
> +time_t boottime;
> +
>  /*
>   * Variables requiring special processing.
>   */
> @@ -255,6 +257,12 @@ main(int argc, char *argv[])
>   argc -= optind;
>   argv += optind;
>  
> + ctime(&boottime); /* satisfy potential $TZ expansion before unveil() */
> +
> + if (unveil("/dev", "r") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
>   if (argc == 0 || (Aflag || aflag)) {
>   debuginit();
>   vfsinit();
> @@ -893,7 +901,6 @@ parse(char *string, int flags)
>   }
>   if (special & BOOTTIME) {
>   struct timeval *btp = (struct timeval *)buf;
> - time_t boottime;
>  
>   if (!nflag) {
>   boottime = btp->tv_sec;
>
>
>
>
>
> Florian Obser <[hidden email]> wrote:
>
> > On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> > > i did that and for some for reason i didn't get it! it tries to open
> > > timezone so it kinda looks like a red flag right there...
> > >
> > > apart from /dev do we need to look into TZ on this one as well? if TZ
> > > var needs to be looked at then all bets are off :/
> >
> > this seems to do the right thing:
> >
> > diff --git sysctl.c sysctl.c
> > index dc6abc16670..c74e706942a 100644
> > --- sysctl.c
> > +++ sysctl.c
> > @@ -255,6 +255,13 @@ main(int argc, char *argv[])
> >   argc -= optind;
> >   argv += optind;
> >  
> > + tzset(); /* for kern.boottime in localtime */
> > +
> > + if (unveil("/dev", "r") == -1)
> > + err(1, "unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + err(1, "unveil");
> > +
> >   if (argc == 0 || (Aflag || aflag)) {
> >   debuginit();
> >   vfsinit();
> >
> >
> > --
> > I'm not entirely sure you are real.
> >

On 10:35 Sat 08 Jun     , Theo de Raadt wrote:

> When userland was massaged for pledge(), I hesitated using the
> "manually call tzset()" approach for handling things.  It felt
> too low-level to call tzset(), an API almost noone knows the
> existance of.
>
> Arriving in the same situation to satisfy unveil().  Again calling
> tzset() feels too unfamiliar and low level.
>
> Regarding the comment in your diff, it says "localtime", but what is
> actually called is ctime(), which calls localtime() (which calls
> tzset(), which is where the unveil-files-missing or pledge-whatver
> issues would show up in some programs).  Probably should adjust
> the comment
>
> Here's the later troublesome chunk:
>
>         if (special & BOOTTIME) {
>                 struct timeval *btp = (struct timeval *)buf;
>                 time_t boottime;
>
>                 if (!nflag) {
>                         boottime = btp->tv_sec;
>                         (void)printf("%s%s%s", string, equ, ctime(&boottime));
>
> That makes me wonder, can we be less obtuse up front, and
> prime the subsystem before unveil by calling the same function which
> will be called later?
>
> Something like this.  It feels slightly better to me.
>
> Index: sysctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.242
> diff -u -p -u -r1.242 sysctl.c
> --- sysctl.c 13 May 2019 20:47:19 -0000 1.242
> +++ sysctl.c 8 Jun 2019 16:33:07 -0000
> @@ -162,6 +162,8 @@ struct list secondlevel[] = {
>  
>  int Aflag, aflag, nflag, qflag;
>  
> +time_t boottime;
> +
>  /*
>   * Variables requiring special processing.
>   */
> @@ -255,6 +257,12 @@ main(int argc, char *argv[])
>   argc -= optind;
>   argv += optind;
>  
> + ctime(&boottime); /* satisfy potential $TZ expansion before unveil() */
> +
> + if (unveil("/dev", "r") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
>   if (argc == 0 || (Aflag || aflag)) {
>   debuginit();
>   vfsinit();
> @@ -893,7 +901,6 @@ parse(char *string, int flags)
>   }
>   if (special & BOOTTIME) {
>   struct timeval *btp = (struct timeval *)buf;
> - time_t boottime;
>  
>   if (!nflag) {
>   boottime = btp->tv_sec;
>
>
>
>
>
> Florian Obser <[hidden email]> wrote:
>
> > On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> > > i did that and for some for reason i didn't get it! it tries to open
> > > timezone so it kinda looks like a red flag right there...
> > >
> > > apart from /dev do we need to look into TZ on this one as well? if TZ
> > > var needs to be looked at then all bets are off :/
> >
> > this seems to do the right thing:
> >
> > diff --git sysctl.c sysctl.c
> > index dc6abc16670..c74e706942a 100644
> > --- sysctl.c
> > +++ sysctl.c
> > @@ -255,6 +255,13 @@ main(int argc, char *argv[])
> >   argc -= optind;
> >   argv += optind;
> >  
> > + tzset(); /* for kern.boottime in localtime */
> > +
> > + if (unveil("/dev", "r") == -1)
> > + err(1, "unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + err(1, "unveil");
> > +
> >   if (argc == 0 || (Aflag || aflag)) {
> >   debuginit();
> >   vfsinit();
> >
> >
> > --
> > I'm not entirely sure you are real.
> >

Reply | Threaded
Open this post in threaded view
|

Re: unveil(2) sysctl(8)

Ricardo Mestre-2
sure, it wasn't mine! it was just missing one unveil so it's ok mestre@
on this one

On 11:42 Sat 15 Jun     , Theo de Raadt wrote:

> yeah that was my idea..
>
> Ricardo Mestre <[hidden email]> wrote:
>
> > Hi,
> >
> > Sorry to be late in the game, but as jca@ pointed out sysctl(8) tries to
> > open _PATH_DEVDB first and then /dev if it cannot open the former, so
> > both should be unveil(2)ed. Scramble the includes while at it.
> >
> > Index: sysctl.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> > retrieving revision 1.242
> > diff -u -p -u -r1.242 sysctl.c
> > --- sysctl.c 13 May 2019 20:47:19 -0000 1.242
> > +++ sysctl.c 14 Jun 2019 19:04:01 -0000
> > @@ -94,13 +94,14 @@
> >  #include <ddb/db_var.h>
> >  #include <dev/rndvar.h>
> >  
> > +#include <ctype.h>
> >  #include <err.h>
> >  #include <errno.h>
> > +#include <limits.h>
> > +#include <paths.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > -#include <ctype.h>
> > -#include <limits.h>
> >  #include <unistd.h>
> >  
> >  #include <machine/cpu.h>
> > @@ -162,6 +163,8 @@ struct list secondlevel[] = {
> >  
> >  int Aflag, aflag, nflag, qflag;
> >  
> > +time_t boottime;
> > +
> >  /*
> >   * Variables requiring special processing.
> >   */
> > @@ -255,6 +258,15 @@ main(int argc, char *argv[])
> >   argc -= optind;
> >   argv += optind;
> >  
> > + ctime(&boottime); /* satisfy potential $TZ expansion before unveil() */
> > +
> > + if (unveil(_PATH_DEVDB, "r") == -1)
> > + err(1,"unveil");
> > + if (unveil("/dev", "r") == -1)
> > + err(1, "unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + err(1, "unveil");
> > +
> >   if (argc == 0 || (Aflag || aflag)) {
> >   debuginit();
> >   vfsinit();
> > @@ -893,7 +905,6 @@ parse(char *string, int flags)
> >   }
> >   if (special & BOOTTIME) {
> >   struct timeval *btp = (struct timeval *)buf;
> > - time_t boottime;
> >  
> >   if (!nflag) {
> >   boottime = btp->tv_sec;
> >
> > On 10:35 Sat 08 Jun     , Theo de Raadt wrote:
> > > When userland was massaged for pledge(), I hesitated using the
> > > "manually call tzset()" approach for handling things.  It felt
> > > too low-level to call tzset(), an API almost noone knows the
> > > existance of.
> > >
> > > Arriving in the same situation to satisfy unveil().  Again calling
> > > tzset() feels too unfamiliar and low level.
> > >
> > > Regarding the comment in your diff, it says "localtime", but what is
> > > actually called is ctime(), which calls localtime() (which calls
> > > tzset(), which is where the unveil-files-missing or pledge-whatver
> > > issues would show up in some programs).  Probably should adjust
> > > the comment
> > >
> > > Here's the later troublesome chunk:
> > >
> > >         if (special & BOOTTIME) {
> > >                 struct timeval *btp = (struct timeval *)buf;
> > >                 time_t boottime;
> > >
> > >                 if (!nflag) {
> > >                         boottime = btp->tv_sec;
> > >                         (void)printf("%s%s%s", string, equ, ctime(&boottime));
> > >
> > > That makes me wonder, can we be less obtuse up front, and
> > > prime the subsystem before unveil by calling the same function which
> > > will be called later?
> > >
> > > Something like this.  It feels slightly better to me.
> > >
> > > Index: sysctl.c
> > > ===================================================================
> > > RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> > > retrieving revision 1.242
> > > diff -u -p -u -r1.242 sysctl.c
> > > --- sysctl.c 13 May 2019 20:47:19 -0000 1.242
> > > +++ sysctl.c 8 Jun 2019 16:33:07 -0000
> > > @@ -162,6 +162,8 @@ struct list secondlevel[] = {
> > >  
> > >  int Aflag, aflag, nflag, qflag;
> > >  
> > > +time_t boottime;
> > > +
> > >  /*
> > >   * Variables requiring special processing.
> > >   */
> > > @@ -255,6 +257,12 @@ main(int argc, char *argv[])
> > >   argc -= optind;
> > >   argv += optind;
> > >  
> > > + ctime(&boottime); /* satisfy potential $TZ expansion before unveil() */
> > > +
> > > + if (unveil("/dev", "r") == -1)
> > > + err(1, "unveil");
> > > + if (unveil(NULL, NULL) == -1)
> > > + err(1, "unveil");
> > >   if (argc == 0 || (Aflag || aflag)) {
> > >   debuginit();
> > >   vfsinit();
> > > @@ -893,7 +901,6 @@ parse(char *string, int flags)
> > >   }
> > >   if (special & BOOTTIME) {
> > >   struct timeval *btp = (struct timeval *)buf;
> > > - time_t boottime;
> > >  
> > >   if (!nflag) {
> > >   boottime = btp->tv_sec;
> > >
> > >
> > >
> > >
> > >
> > > Florian Obser <[hidden email]> wrote:
> > >
> > > > On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> > > > > i did that and for some for reason i didn't get it! it tries to open
> > > > > timezone so it kinda looks like a red flag right there...
> > > > >
> > > > > apart from /dev do we need to look into TZ on this one as well? if TZ
> > > > > var needs to be looked at then all bets are off :/
> > > >
> > > > this seems to do the right thing:
> > > >
> > > > diff --git sysctl.c sysctl.c
> > > > index dc6abc16670..c74e706942a 100644
> > > > --- sysctl.c
> > > > +++ sysctl.c
> > > > @@ -255,6 +255,13 @@ main(int argc, char *argv[])
> > > >   argc -= optind;
> > > >   argv += optind;
> > > >  
> > > > + tzset(); /* for kern.boottime in localtime */
> > > > +
> > > > + if (unveil("/dev", "r") == -1)
> > > > + err(1, "unveil");
> > > > + if (unveil(NULL, NULL) == -1)
> > > > + err(1, "unveil");
> > > > +
> > > >   if (argc == 0 || (Aflag || aflag)) {
> > > >   debuginit();
> > > >   vfsinit();
> > > >
> > > >
> > > > --
> > > > I'm not entirely sure you are real.
> > > >
> >
> > On 10:35 Sat 08 Jun     , Theo de Raadt wrote:
> > > When userland was massaged for pledge(), I hesitated using the
> > > "manually call tzset()" approach for handling things.  It felt
> > > too low-level to call tzset(), an API almost noone knows the
> > > existance of.
> > >
> > > Arriving in the same situation to satisfy unveil().  Again calling
> > > tzset() feels too unfamiliar and low level.
> > >
> > > Regarding the comment in your diff, it says "localtime", but what is
> > > actually called is ctime(), which calls localtime() (which calls
> > > tzset(), which is where the unveil-files-missing or pledge-whatver
> > > issues would show up in some programs).  Probably should adjust
> > > the comment
> > >
> > > Here's the later troublesome chunk:
> > >
> > >         if (special & BOOTTIME) {
> > >                 struct timeval *btp = (struct timeval *)buf;
> > >                 time_t boottime;
> > >
> > >                 if (!nflag) {
> > >                         boottime = btp->tv_sec;
> > >                         (void)printf("%s%s%s", string, equ, ctime(&boottime));
> > >
> > > That makes me wonder, can we be less obtuse up front, and
> > > prime the subsystem before unveil by calling the same function which
> > > will be called later?
> > >
> > > Something like this.  It feels slightly better to me.
> > >
> > > Index: sysctl.c
> > > ===================================================================
> > > RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> > > retrieving revision 1.242
> > > diff -u -p -u -r1.242 sysctl.c
> > > --- sysctl.c 13 May 2019 20:47:19 -0000 1.242
> > > +++ sysctl.c 8 Jun 2019 16:33:07 -0000
> > > @@ -162,6 +162,8 @@ struct list secondlevel[] = {
> > >  
> > >  int Aflag, aflag, nflag, qflag;
> > >  
> > > +time_t boottime;
> > > +
> > >  /*
> > >   * Variables requiring special processing.
> > >   */
> > > @@ -255,6 +257,12 @@ main(int argc, char *argv[])
> > >   argc -= optind;
> > >   argv += optind;
> > >  
> > > + ctime(&boottime); /* satisfy potential $TZ expansion before unveil() */
> > > +
> > > + if (unveil("/dev", "r") == -1)
> > > + err(1, "unveil");
> > > + if (unveil(NULL, NULL) == -1)
> > > + err(1, "unveil");
> > >   if (argc == 0 || (Aflag || aflag)) {
> > >   debuginit();
> > >   vfsinit();
> > > @@ -893,7 +901,6 @@ parse(char *string, int flags)
> > >   }
> > >   if (special & BOOTTIME) {
> > >   struct timeval *btp = (struct timeval *)buf;
> > > - time_t boottime;
> > >  
> > >   if (!nflag) {
> > >   boottime = btp->tv_sec;
> > >
> > >
> > >
> > >
> > >
> > > Florian Obser <[hidden email]> wrote:
> > >
> > > > On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> > > > > i did that and for some for reason i didn't get it! it tries to open
> > > > > timezone so it kinda looks like a red flag right there...
> > > > >
> > > > > apart from /dev do we need to look into TZ on this one as well? if TZ
> > > > > var needs to be looked at then all bets are off :/
> > > >
> > > > this seems to do the right thing:
> > > >
> > > > diff --git sysctl.c sysctl.c
> > > > index dc6abc16670..c74e706942a 100644
> > > > --- sysctl.c
> > > > +++ sysctl.c
> > > > @@ -255,6 +255,13 @@ main(int argc, char *argv[])
> > > >   argc -= optind;
> > > >   argv += optind;
> > > >  
> > > > + tzset(); /* for kern.boottime in localtime */
> > > > +
> > > > + if (unveil("/dev", "r") == -1)
> > > > + err(1, "unveil");
> > > > + if (unveil(NULL, NULL) == -1)
> > > > + err(1, "unveil");
> > > > +
> > > >   if (argc == 0 || (Aflag || aflag)) {
> > > >   debuginit();
> > > >   vfsinit();
> > > >
> > > >
> > > > --
> > > > I'm not entirely sure you are real.
> > > >