pledge route(8) with '-n' flag
Hi,
I gave it another go to further reduce the pledge promises in route(8),
and this is what I could come up with:
Remove the initial pledge and join the 2 switch cases, then apply a
specific pledge depending on the codepath:
flush route, show and monitor use "stdio" if -n is used, otherwise it
uses "stdio rpath dns".
add/change/delete route on the other hand cannot be done with this
condition since nflag is not verified anywhere in the newroute()
function, and it doesn't matter if it's used or not. That being said it
starts with "stdio rpath dns" and after the loop to parse the arguments
and modifiers it can be reduced to "stdio".
While here I also changed 0 to SHUT_RD in order to use the symbolic name
instead of the hardcoded value.
As a side note I inspected route(8)'s source code on FreeBSD and NetBSD
and they also suffer from the same "problem" with nflag when changing
routes, maybe it's still there just for compatibility? As far as I can
remember I never used -n when changing routes because it just works. I
don't think it should be reported to bugs@ though since it's not an
issue per se.
Also adding mikeb@, bennob@ and claudio@ to the conversation as per
theo@'s advise.
Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.179
diff -u -p -u -r1.179 route.c
--- route.c 25 Oct 2015 09:37:08 - 1.179
+++ route.c 19 Nov 2015 14:46:32 -
@@ -224,17 +224,6 @@ main(int argc, char **argv)
case K_FLUSH:
exit(flushroutes(argc, argv));
break;
- }
-
- if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
- } else {
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
- }
-
- switch (kw) {
case K_GET:
uid = 0;
/* FALLTHROUGH */
@@ -330,7 +319,7 @@ flushroutes(int argc, char **argv)
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
@@ -445,12 +434,15 @@ newroute(int argc, char **argv)
int key;
uint8_t prio = 0;
struct hostent *hp = NULL;
+
+ if (pledge("stdio rpath dns", NULL) == -1)
+ err(1, "pledge");
if (uid)
errx(1, "must be root to alter routing table");
cmd = argv[0];
if (*cmd != 'g')
- shutdown(s, 0); /* Don't want to read back our messages */
+ shutdown(s, SHUT_RD); /* Don't want to read back our messages */
while (--argc > 0) {
if (**(++argv)== '-') {
switch (key = keyword(1 + *argv)) {
@@ -630,6 +622,10 @@ newroute(int argc, char **argv)
usage(NULL);
}
}
+
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+
if (forcehost)
ishost = 1;
if (forcenet)
@@ -1090,8 +1086,13 @@ monitor(int argc, char *argv[])
char msg[2048];
time_t now;
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
+ if (nflag) {
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath dns", NULL) == -1)
+ err(1, "pledge");
+ }
verbose = 1;
if (debugonly) {
Index: show.c
===
RCS file: /cvs/src/sbin/route/show.c,v
retrieving revision 1.102
diff -u -p -u -r1.102 show.c
--- show.c 23 Oct 2015 15:03:25 - 1.102
+++ show.c 19 Nov 2015 14:46:35 -
@@ -146,7 +146,7 @@ p_rttables(int af, u_int tableid, int ha
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
Re: pledge route(8) with '-n' flag
Hello again, With the latest patch I sent all code paths honor -n and won't abort due to pledge only to stdio, for now the only remaining it's newroute (add/change/delete) that aborts if -n is used and pledge only to stdio. Now, this one starts getting into netinet and I'm still not comfortable with it, sorry :( On 16/11/2015 16:06, Theo de Raadt wrote: >> On 16/11/15(Mon) 10:03, Ricardo Mestre wrote: >>> Hello! >>> >>> Like Benoit said, monitor still needs dns all the time, but since pledge >>> was being called there again with dns pledge then I thought it wouldn't >>> abort. Taking that into consideration and looking at it a little bit >>> more, how about this? >> >> IMHO this doesn't make sense if I explicitly say "route -n monitor" I'd >> prefer not to have any address resolved >> >> Otherwise I just say "route monitor". >> >> I'd add that on the list of bugs discovered by pledge(2). > > Yes... the monitor command should honour the -n flag. >
Re: pledge route(8) with '-n' flag
> On 16/11/15(Mon) 10:03, Ricardo Mestre wrote: > > Hello! > > > > Like Benoit said, monitor still needs dns all the time, but since pledge > > was being called there again with dns pledge then I thought it wouldn't > > abort. Taking that into consideration and looking at it a little bit > > more, how about this? > > IMHO this doesn't make sense if I explicitly say "route -n monitor" I'd > prefer not to have any address resolved > > Otherwise I just say "route monitor". > > I'd add that on the list of bugs discovered by pledge(2). Yes... the monitor command should honour the -n flag.
Re: pledge route(8) with '-n' flag
Hi Martin,
Using the same logic of my previous patch, now monitor works with
"stdio" when -n is used and "stdio rpath dns" otherwhise. Unfortunately
for newroute (add/delete/change) it still needs the 3 of them since when
it detects it's a host (instead of an IP address) it will always contact
the resolver, no matter -n is used or not:
Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.179
diff -u -p -u -r1.179 route.c
--- route.c 25 Oct 2015 09:37:08 - 1.179
+++ route.c 16 Nov 2015 11:07:24 -
@@ -224,17 +224,6 @@ main(int argc, char **argv)
case K_FLUSH:
exit(flushroutes(argc, argv));
break;
- }
-
- if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
- } else {
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
- }
-
- switch (kw) {
case K_GET:
uid = 0;
/* FALLTHROUGH */
@@ -330,7 +319,7 @@ flushroutes(int argc, char **argv)
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
@@ -446,6 +435,9 @@ newroute(int argc, char **argv)
uint8_t prio = 0;
struct hostent *hp = NULL;
+ if (pledge("stdio rpath dns", NULL) == -1)
+ err(1, "pledge");
+
if (uid)
errx(1, "must be root to alter routing table");
cmd = argv[0];
@@ -1090,8 +1082,13 @@ monitor(int argc, char *argv[])
char msg[2048];
time_t now;
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
+ if (nflag) {
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+ } else {
+ if (pledge("stdio rpath dns", NULL) == -1)
+ err(1, "pledge");
+ }
verbose = 1;
if (debugonly) {
Index: show.c
===
RCS file: /cvs/src/sbin/route/show.c,v
retrieving revision 1.102
diff -u -p -u -r1.102 show.c
--- show.c 23 Oct 2015 15:03:25 - 1.102
+++ show.c 16 Nov 2015 11:07:33 -
@@ -146,7 +146,7 @@ p_rttables(int af, u_int tableid, int ha
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
On 16/11/2015 10:22, Martin Pieuchot wrote:
> On 16/11/15(Mon) 10:03, Ricardo Mestre wrote:
>> Hello!
>>
>> Like Benoit said, monitor still needs dns all the time, but since pledge
>> was being called there again with dns pledge then I thought it wouldn't
>> abort. Taking that into consideration and looking at it a little bit
>> more, how about this?
>
> IMHO this doesn't make sense if I explicitly say "route -n monitor" I'd
> prefer not to have any address resolved
>
> Otherwise I just say "route monitor".
>
> I'd add that on the list of bugs discovered by pledge(2).
>
Re: pledge route(8) with '-n' flag
On 16/11/15(Mon) 10:03, Ricardo Mestre wrote: > Hello! > > Like Benoit said, monitor still needs dns all the time, but since pledge > was being called there again with dns pledge then I thought it wouldn't > abort. Taking that into consideration and looking at it a little bit > more, how about this? IMHO this doesn't make sense if I explicitly say "route -n monitor" I'd prefer not to have any address resolved Otherwise I just say "route monitor". I'd add that on the list of bugs discovered by pledge(2).
Re: pledge route(8) with '-n' flag
Hello!
Like Benoit said, monitor still needs dns all the time, but since pledge
was being called there again with dns pledge then I thought it wouldn't
abort. Taking that into consideration and looking at it a little bit
more, how about this?
I removed the first pledge in main() between the 2 switch cases and
reunified them, then considering the four codepaths below I applied the
pledges directly onto each one:
show -> "stdio" when -n, "stdio rpath dns" otherwise
flushroutes -> "stdio" when -n, "stdio rpath dns" otherwise
newroute (add/change/delete) -> always needs "stdio rpath dns"
monitor -> always needs "stdio rpath dns" (this pledge was already there
and I left it untouched)
I tried a few tests manually, and also the regress battery tests for
route and apart from a couple of "Network unreachable" none had aborted
due to incorrect pledging.
PS: I replied this directly only to Theo, but undoubtedly it needs to be
reviewed by a larger audience.
Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.179
diff -u -p -u -r1.179 route.c
--- route.c 25 Oct 2015 09:37:08 - 1.179
+++ route.c 15 Nov 2015 18:06:02 -
@@ -224,17 +224,6 @@ main(int argc, char **argv)
case K_FLUSH:
exit(flushroutes(argc, argv));
break;
- }
-
- if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
- } else {
- if (pledge("stdio rpath dns", NULL) == -1)
- err(1, "pledge");
- }
-
- switch (kw) {
case K_GET:
uid = 0;
/* FALLTHROUGH */
@@ -330,7 +319,7 @@ flushroutes(int argc, char **argv)
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
@@ -445,6 +434,9 @@ newroute(int argc, char **argv)
int key;
uint8_t prio = 0;
struct hostent *hp = NULL;
+
+ if (pledge("stdio rpath dns", NULL) == -1)
+ err(1, "pledge");
if (uid)
errx(1, "must be root to alter routing table");
Index: show.c
===
RCS file: /cvs/src/sbin/route/show.c,v
retrieving revision 1.102
diff -u -p -u -r1.102 show.c
--- show.c 23 Oct 2015 15:03:25 - 1.102
+++ show.c 15 Nov 2015 18:06:09 -
@@ -146,7 +146,7 @@ p_rttables(int af, u_int tableid, int ha
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
On 13/11/2015 19:08, Sebastian Benoit wrote:
> Ricardo Mestre(ser...@helheim.mooo.com) on 2015.11.13 18:00:11 +:
>> Hello,
>>
>> If '-n' argument is used on route(8) then nflag will be active and dns
>> transactions won't be needed, am I correct?
>
> please find out yourself.
>
> at least the pledge call in monitor will fail with -n and your diff,
> so some more work is required.
>
>> Index: route.c
>> ===
>> RCS file: /cvs/src/sbin/route/route.c,v
>> retrieving revision 1.179
>> diff -u -p -u -r1.179 route.c
>> --- route.c 25 Oct 2015 09:37:08 - 1.179
>> +++ route.c 13 Nov 2015 15:37:37 -
>> @@ -227,7 +227,7 @@ main(int argc, char **argv)
>> }
>>
>> if (nflag) {
>> -if (pledge("stdio rpath dns", NULL) == -1)
>> +if (pledge("stdio rpath", NULL) == -1)
>> err(1, "pledge");
>> } else {
>> if (pledge("stdio rpath dns", NULL) == -1)
>> @@ -330,7 +330,7 @@ flushroutes(int argc, char **argv)
>> }
>>
>> if (nflag) {
>> -if (pledge("stdio rpath dns", NULL) == -1)
>> +if (pledge("stdio rpath", NULL) == -1)
>> err(1, "pledge");
>> } else {
>> if (pledge("stdio rpath dns", NULL) == -1)
>> Index: show.c
>> ===
>> RCS file: /cvs/src/sbin/route/show.c,v
>> retrieving revision 1.102
>> diff -u -p -u -r1.102 show.c
>> --- show.c 23 Oct 2015 15:03:25 - 1.102
>> +++ show.c 13 Nov 2015 15:37:37 -
>> @@ -146,7 +146,7 @@ p_rttables(int af, u_int tableid, int ha
>> }
>>
>> if (nflag) {
>> -if (pledge("stdio rpath dns", NULL) == -1)
>> +if (pledge("stdio rpath", NULL) == -1)
>> err(1, "pledge");
>> } else {
>> if (pledge("stdio rpath dns", NULL) == -1)
>>
>> Best regards,
>> Ricardo Mestre
>>
>
Re: pledge route(8) with '-n' flag
Ricardo Mestre(ser...@helheim.mooo.com) on 2015.11.13 18:00:11 +:
> Hello,
>
> If '-n' argument is used on route(8) then nflag will be active and dns
> transactions won't be needed, am I correct?
please find out yourself.
at least the pledge call in monitor will fail with -n and your diff,
so some more work is required.
> Index: route.c
> ===
> RCS file: /cvs/src/sbin/route/route.c,v
> retrieving revision 1.179
> diff -u -p -u -r1.179 route.c
> --- route.c 25 Oct 2015 09:37:08 - 1.179
> +++ route.c 13 Nov 2015 15:37:37 -
> @@ -227,7 +227,7 @@ main(int argc, char **argv)
> }
>
> if (nflag) {
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
> err(1, "pledge");
> } else {
> if (pledge("stdio rpath dns", NULL) == -1)
> @@ -330,7 +330,7 @@ flushroutes(int argc, char **argv)
> }
>
> if (nflag) {
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
> err(1, "pledge");
> } else {
> if (pledge("stdio rpath dns", NULL) == -1)
> Index: show.c
> ===
> RCS file: /cvs/src/sbin/route/show.c,v
> retrieving revision 1.102
> diff -u -p -u -r1.102 show.c
> --- show.c23 Oct 2015 15:03:25 - 1.102
> +++ show.c13 Nov 2015 15:37:37 -
> @@ -146,7 +146,7 @@ p_rttables(int af, u_int tableid, int ha
> }
>
> if (nflag) {
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
> err(1, "pledge");
> } else {
> if (pledge("stdio rpath dns", NULL) == -1)
>
> Best regards,
> Ricardo Mestre
>
--
pledge route(8) with '-n' flag
Hello,
If '-n' argument is used on route(8) then nflag will be active and dns
transactions won't be needed, am I correct?
Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.179
diff -u -p -u -r1.179 route.c
--- route.c 25 Oct 2015 09:37:08 - 1.179
+++ route.c 13 Nov 2015 15:37:37 -
@@ -227,7 +227,7 @@ main(int argc, char **argv)
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
@@ -330,7 +330,7 @@ flushroutes(int argc, char **argv)
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
Index: show.c
===
RCS file: /cvs/src/sbin/route/show.c,v
retrieving revision 1.102
diff -u -p -u -r1.102 show.c
--- show.c 23 Oct 2015 15:03:25 - 1.102
+++ show.c 13 Nov 2015 15:37:37 -
@@ -146,7 +146,7 @@ p_rttables(int af, u_int tableid, int ha
}
if (nflag) {
- if (pledge("stdio rpath dns", NULL) == -1)
+ if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath dns", NULL) == -1)
Best regards,
Ricardo Mestre
