Re: unveil(2) sysctl(8)
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 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.c13 May 2019 20:47:19 - 1.242
> > +++ sysctl.c14 Jun 2019 19:04:01 -
> > @@ -94,13 +94,14 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > -#include
> > -#include
> > #include
> >
> > #include
> > @@ -162,6 +163,8 @@ struct list secondlevel[] = {
> >
> > intAflag, aflag, nflag, qflag;
> >
> > +time_t boottime;
> > +
> > /*
> > * Variables requiring special processing.
> > */
> > @@ -255,6 +258,15 @@ main(int argc, char *argv[])
> > argc -= optind;
> > argv += optind;
> >
> > + ctime(); /* 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());
> > >
> > > 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 - 1.242
> > > +++ sysctl.c 8 Jun 2019 16:33:07 -
> > > @@ -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(); /* 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 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
Re: unveil(2) sysctl(8)
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.c13 May 2019 20:47:19 - 1.242
+++ sysctl.c14 Jun 2019 19:04:01 -
@@ -94,13 +94,14 @@
#include
#include
+#include
#include
#include
+#include
+#include
#include
#include
#include
-#include
-#include
#include
#include
@@ -162,6 +163,8 @@ struct list secondlevel[] = {
intAflag, aflag, nflag, qflag;
+time_t boottime;
+
/*
* Variables requiring special processing.
*/
@@ -255,6 +258,15 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
+ ctime(); /* 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());
>
> 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 - 1.242
> +++ sysctl.c 8 Jun 2019 16:33:07 -
> @@ -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(); /* 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 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
Re: unveil(2) sysctl(8)
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());
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.c13 May 2019 20:47:19 - 1.242
+++ sysctl.c8 Jun 2019 16:33:07 -
@@ -162,6 +162,8 @@ struct list secondlevel[] = {
intAflag, aflag, nflag, qflag;
+time_t boottime;
+
/*
* Variables requiring special processing.
*/
@@ -255,6 +257,12 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
+ ctime(); /* 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 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.
>
Re: unveil(2) sysctl(8)
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@.
Re: unveil(2) sysctl(8)
On Sat, Jun 08 2019, Stuart Henderson 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
Re: unveil(2) sysctl(8)
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.
Re: unveil(2) sysctl(8)
On Sat, Jun 08, 2019 at 01:15:37PM +0200, Jeremie Courreges-Anglas wrote:
> On Sat, Jun 08 2019, Florian Obser 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,0x1)
> 3461 sysctl NAMI "/var/run/dev.db"
> 3461 sysctl RET open -1 errno 2 No such file or directory
> 3461 sysctl CALL
> open(0x7fad42c8f19,0x3)
> 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.
Re: unveil(2) sysctl(8)
On Sat, Jun 08 2019, Florian Obser 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,0x1)
3461 sysctl NAMI "/var/run/dev.db"
3461 sysctl RET open -1 errno 2 No such file or directory
3461 sysctl CALL
open(0x7fad42c8f19,0x3)
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
Re: unveil(2) sysctl(8)
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.
Re: unveil(2) sysctl(8)
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.c13 May 2019 20:47:19 - 1.242
> > +++ sysctl.c7 Jun 2019 17:01:23 -
> > @@ -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
>
Re: unveil(2) sysctl(8)
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 - 1.242
> +++ sysctl.c 7 Jun 2019 17:01:23 -
> @@ -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
unveil(2) sysctl(8)
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.c13 May 2019 20:47:19 - 1.242
+++ sysctl.c7 Jun 2019 17:01:23 -
@@ -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();
