Re: unveil ospfd's parent proc
On Sun, Oct 28, 2018 at 03:58:53PM +0100, Florian Obser wrote:
> Sorry, I'm on a phone. The diff context looks like the control FD is already
> open at this point. Does ospfd later re-open it?
>
No it does not. It deletes the control socket during shutdown.
> On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> wrote:
> >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> >> Remi Locherer wrote:
> >>
> >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> >> > > This breaks usage of the "include" keyword. Something that all
> >the parse.y daemons support.
> >> > >
> >> >
> >> > Oh, of course!
> >> >
> >> > I guess this is similar to unveil files based on a list of command
> >line args.
> >>
> >> correct.
> >>
> >> Now that unveil is used in the tree, there are 3 types of programs
> >>
> >> 1) they use unveil
> >> 2) they use pledge, heading close towards "stdio" without a "*path"
> >> 3) they access arbitrary files based upon argv
> >>
> >> this is (3), except not argv, it nested inside the config file
> >>
> >> Well there are maybe 20 programs beyond that which aren't converted
> >yet,
> >> but things are looking pretty good.
> >>
> >
> >Since ospfd is not suppose to write or execute files we could make the
> >file system read only (with the exception of the control socket).
> >
> >(Once we can add pledge to ospfd's parent proc this will probably not
> >make
> >sense anymore.)
> >
> >
> >
> >cvs diff: Diffing .
> >Index: ospfd.c
> >===
> >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> >retrieving revision 1.100
> >diff -u -p -r1.100 ospfd.c
> >--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> >+++ ospfd.c 27 Oct 2018 07:28:58 -
> >@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> > fatalx("control socket setup failed");
> > main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
> >
> >+if (unveil("/", "r") == -1)
> >+fatal("unveil");
> >+if (unveil(ospfd_conf->csock, "c") == -1)
> >+fatal("unveil");
> >+if (unveil(NULL, NULL) == -1)
> >+fatal("unveil");
> >+
> > if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
> > ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
> > fatalx("kr_init failed");
Re: unveil ospfd's parent proc
On Sun, Oct 28, 2018 at 07:24:23PM +0100, Sebastian Benoit wrote:
> Ricardo Mestre(ser...@helheim.mooo.com) on 2018.10.28 17:26:24 +:
> > Correct, and I'd go even further by not unveiling the socket at all. A few
> > weeks ago I removed the logic of unlinking the socket when the program
> > stops,
> > for a few daemons, but left untouched the ones that don't have the main
> > process
> > pledged since it wouldn't make much difference.
> >
> > If we want to go this way then I think we also should remove the logic for
> > these daemons and adding unveil just like the below.
> >
> > Comments? OK?
>
> in ospfd, ospf6d (and hopefully soon bgpd) we make sure the daemon does not
> run twice by checking the socket.
Removing control_cleanup would not affect this check. It's more a cosmetic
thing to remove /var/run/ospfd.sock.0 on exit.
>
> Since it really is bad if you run two ospfd, please keep this.
>
> /Benno
>
>
> >
> > Index: control.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v
> > retrieving revision 1.45
> > diff -u -p -u -r1.45 control.c
> > --- control.c 29 Aug 2018 08:43:16 - 1.45
> > +++ control.c 28 Oct 2018 17:24:46 -
> > @@ -124,16 +124,6 @@ control_listen(void)
> > return (0);
> > }
> >
> > -void
> > -control_cleanup(char *path)
> > -{
> > - if (path == NULL)
> > - return;
> > - event_del(&control_state.ev);
> > - event_del(&control_state.evt);
> > - unlink(path);
> > -}
> > -
> > /* ARGSUSED */
> > void
> > control_accept(int listenfd, short event, void *bula)
> > Index: control.h
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/control.h,v
> > retrieving revision 1.7
> > diff -u -p -u -r1.7 control.h
> > --- control.h 29 Aug 2018 08:43:16 - 1.7
> > +++ control.h 28 Oct 2018 17:24:46 -
> > @@ -40,6 +40,5 @@ int control_listen(void);
> > void control_accept(int, short, void *);
> > void control_dispatch_imsg(int, short, void *);
> > intcontrol_imsg_relay(struct imsg *);
> > -void control_cleanup(char *);
> >
> > #endif /* _CONTROL_H_ */
> > Index: ospfd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > retrieving revision 1.100
> > diff -u -p -u -r1.100 ospfd.c
> > --- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> > +++ ospfd.c 28 Oct 2018 17:24:46 -
> > @@ -288,6 +288,11 @@ main(int argc, char *argv[])
> > area_del(a);
> > }
> >
> > + if (unveil("/", "r") == -1)
> > + fatal("unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + fatal("unveil");
> > +
> > event_dispatch();
> >
> > ospfd_shutdown();
> > @@ -308,7 +313,6 @@ ospfd_shutdown(void)
> > msgbuf_clear(&iev_rde->ibuf.w);
> > close(iev_rde->ibuf.fd);
> >
> > - control_cleanup(ospfd_conf->csock);
> > while ((r = SIMPLEQ_FIRST(&ospfd_conf->redist_list)) != NULL) {
> > SIMPLEQ_REMOVE_HEAD(&ospfd_conf->redist_list, entry);
> > free(r);
> >
> > On 15:58 Sun 28 Oct , Florian Obser wrote:
> > > Sorry, I'm on a phone. The diff context looks like the control FD is
> > > already open at this point. Does ospfd later re-open it?
> > >
> > > On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> > > wrote:
> > > >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> > > >> Remi Locherer wrote:
> > > >>
> > > >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> > > >> > > This breaks usage of the "include" keyword. Something that all
> > > >the parse.y daemons support.
> > > >> > >
> > > >> >
> > > >> > Oh, of course!
> > > >> >
> > > >> > I guess this is similar to unveil files based on a list of command
> > > >line args.
> > > >>
> > > >> correct.
> > > >>
> > > >> Now that unveil is used in the tree, there are 3 types of programs
> > > >>
> > > >> 1) they use unveil
> > > >> 2) they use pledge, heading close towards "stdio" without a "*path"
> > > >> 3) they access arbitrary files based upon argv
> > > >>
> > > >> this is (3), except not argv, it nested inside the config file
> > > >>
> > > >> Well there are maybe 20 programs beyond that which aren't converted
> > > >yet,
> > > >> but things are looking pretty good.
> > > >>
> > > >
> > > >Since ospfd is not suppose to write or execute files we could make the
> > > >file system read only (with the exception of the control socket).
> > > >
> > > >(Once we can add pledge to ospfd's parent proc this will probably not
> > > >make
> > > >sense anymore.)
> > > >
> > > >
> > > >
> > > >cvs diff: Diffing .
> > > >Index: ospfd.c
> > > >===
> > > >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > > >retrieving revision 1.100
> > > >diff -u -p -r1.100 ospfd.
Re: unveil ospfd's parent proc
Not all daemons have the same behaviour, so if this is still used then
Remi's diff of course makes more sense.
On 19:24 Sun 28 Oct , Sebastian Benoit wrote:
> Ricardo Mestre(ser...@helheim.mooo.com) on 2018.10.28 17:26:24 +:
> in ospfd, ospf6d (and hopefully soon bgpd) we make sure the daemon does not
> run twice by checking the socket.
>
> Since it really is bad if you run two ospfd, please keep this.
>
> /Benno
>
>
> >
> > Index: control.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v
> > retrieving revision 1.45
> > diff -u -p -u -r1.45 control.c
> > --- control.c 29 Aug 2018 08:43:16 - 1.45
> > +++ control.c 28 Oct 2018 17:24:46 -
> > @@ -124,16 +124,6 @@ control_listen(void)
> > return (0);
> > }
> >
> > -void
> > -control_cleanup(char *path)
> > -{
> > - if (path == NULL)
> > - return;
> > - event_del(&control_state.ev);
> > - event_del(&control_state.evt);
> > - unlink(path);
> > -}
> > -
> > /* ARGSUSED */
> > void
> > control_accept(int listenfd, short event, void *bula)
> > Index: control.h
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/control.h,v
> > retrieving revision 1.7
> > diff -u -p -u -r1.7 control.h
> > --- control.h 29 Aug 2018 08:43:16 - 1.7
> > +++ control.h 28 Oct 2018 17:24:46 -
> > @@ -40,6 +40,5 @@ int control_listen(void);
> > void control_accept(int, short, void *);
> > void control_dispatch_imsg(int, short, void *);
> > intcontrol_imsg_relay(struct imsg *);
> > -void control_cleanup(char *);
> >
> > #endif /* _CONTROL_H_ */
> > Index: ospfd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > retrieving revision 1.100
> > diff -u -p -u -r1.100 ospfd.c
> > --- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> > +++ ospfd.c 28 Oct 2018 17:24:46 -
> > @@ -288,6 +288,11 @@ main(int argc, char *argv[])
> > area_del(a);
> > }
> >
> > + if (unveil("/", "r") == -1)
> > + fatal("unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + fatal("unveil");
> > +
> > event_dispatch();
> >
> > ospfd_shutdown();
> > @@ -308,7 +313,6 @@ ospfd_shutdown(void)
> > msgbuf_clear(&iev_rde->ibuf.w);
> > close(iev_rde->ibuf.fd);
> >
> > - control_cleanup(ospfd_conf->csock);
> > while ((r = SIMPLEQ_FIRST(&ospfd_conf->redist_list)) != NULL) {
> > SIMPLEQ_REMOVE_HEAD(&ospfd_conf->redist_list, entry);
> > free(r);
> >
> > On 15:58 Sun 28 Oct , Florian Obser wrote:
> > > Sorry, I'm on a phone. The diff context looks like the control FD is
> > > already open at this point. Does ospfd later re-open it?
> > >
> > > On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> > > wrote:
> > > >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> > > >> Remi Locherer wrote:
> > > >>
> > > >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> > > >> > > This breaks usage of the "include" keyword. Something that all
> > > >the parse.y daemons support.
> > > >> > >
> > > >> >
> > > >> > Oh, of course!
> > > >> >
> > > >> > I guess this is similar to unveil files based on a list of command
> > > >line args.
> > > >>
> > > >> correct.
> > > >>
> > > >> Now that unveil is used in the tree, there are 3 types of programs
> > > >>
> > > >> 1) they use unveil
> > > >> 2) they use pledge, heading close towards "stdio" without a "*path"
> > > >> 3) they access arbitrary files based upon argv
> > > >>
> > > >> this is (3), except not argv, it nested inside the config file
> > > >>
> > > >> Well there are maybe 20 programs beyond that which aren't converted
> > > >yet,
> > > >> but things are looking pretty good.
> > > >>
> > > >
> > > >Since ospfd is not suppose to write or execute files we could make the
> > > >file system read only (with the exception of the control socket).
> > > >
> > > >(Once we can add pledge to ospfd's parent proc this will probably not
> > > >make
> > > >sense anymore.)
> > > >
> > > >
> > > >
> > > >cvs diff: Diffing .
> > > >Index: ospfd.c
> > > >===
> > > >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > > >retrieving revision 1.100
> > > >diff -u -p -r1.100 ospfd.c
> > > >--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> > > >+++ ospfd.c 27 Oct 2018 07:28:58 -
> > > >@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> > > > fatalx("control socket setup failed");
> > > > main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
> > > >
> > > >+if (unveil("/", "r") == -1)
> > > >+fatal("unveil");
> > > >+if (unveil(ospfd_conf->csock, "c") == -1)
> > > >+fatal("unveil");
Re: unveil ospfd's parent proc
On Sun, Oct 28, 2018 at 05:26:24PM +, Ricardo Mestre wrote:
> Correct, and I'd go even further by not unveiling the socket at all. A few
> weeks ago I removed the logic of unlinking the socket when the program stops,
> for a few daemons, but left untouched the ones that don't have the main
> process
> pledged since it wouldn't make much difference.
>
> If we want to go this way then I think we also should remove the logic for
> these daemons and adding unveil just like the below.
>
> Comments? OK?
I also thought about this. Once we can pledge the master process it makes
sense to me to remove control_cleanup and use the rpath promise.
But was is the gain now?
>
> Index: control.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/control.c,v
> retrieving revision 1.45
> diff -u -p -u -r1.45 control.c
> --- control.c 29 Aug 2018 08:43:16 - 1.45
> +++ control.c 28 Oct 2018 17:24:46 -
> @@ -124,16 +124,6 @@ control_listen(void)
> return (0);
> }
>
> -void
> -control_cleanup(char *path)
> -{
> - if (path == NULL)
> - return;
> - event_del(&control_state.ev);
> - event_del(&control_state.evt);
> - unlink(path);
> -}
> -
> /* ARGSUSED */
> void
> control_accept(int listenfd, short event, void *bula)
> Index: control.h
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/control.h,v
> retrieving revision 1.7
> diff -u -p -u -r1.7 control.h
> --- control.h 29 Aug 2018 08:43:16 - 1.7
> +++ control.h 28 Oct 2018 17:24:46 -
> @@ -40,6 +40,5 @@ int control_listen(void);
> void control_accept(int, short, void *);
> void control_dispatch_imsg(int, short, void *);
> int control_imsg_relay(struct imsg *);
> -void control_cleanup(char *);
>
> #endif /* _CONTROL_H_ */
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.100
> diff -u -p -u -r1.100 ospfd.c
> --- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> +++ ospfd.c 28 Oct 2018 17:24:46 -
> @@ -288,6 +288,11 @@ main(int argc, char *argv[])
> area_del(a);
> }
>
> + if (unveil("/", "r") == -1)
> + fatal("unveil");
> + if (unveil(NULL, NULL) == -1)
> + fatal("unveil");
> +
> event_dispatch();
>
> ospfd_shutdown();
> @@ -308,7 +313,6 @@ ospfd_shutdown(void)
> msgbuf_clear(&iev_rde->ibuf.w);
> close(iev_rde->ibuf.fd);
>
> - control_cleanup(ospfd_conf->csock);
> while ((r = SIMPLEQ_FIRST(&ospfd_conf->redist_list)) != NULL) {
> SIMPLEQ_REMOVE_HEAD(&ospfd_conf->redist_list, entry);
> free(r);
>
> On 15:58 Sun 28 Oct , Florian Obser wrote:
> > Sorry, I'm on a phone. The diff context looks like the control FD is
> > already open at this point. Does ospfd later re-open it?
> >
> > On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> > wrote:
> > >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> > >> Remi Locherer wrote:
> > >>
> > >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> > >> > > This breaks usage of the "include" keyword. Something that all
> > >the parse.y daemons support.
> > >> > >
> > >> >
> > >> > Oh, of course!
> > >> >
> > >> > I guess this is similar to unveil files based on a list of command
> > >line args.
> > >>
> > >> correct.
> > >>
> > >> Now that unveil is used in the tree, there are 3 types of programs
> > >>
> > >> 1) they use unveil
> > >> 2) they use pledge, heading close towards "stdio" without a "*path"
> > >> 3) they access arbitrary files based upon argv
> > >>
> > >> this is (3), except not argv, it nested inside the config file
> > >>
> > >> Well there are maybe 20 programs beyond that which aren't converted
> > >yet,
> > >> but things are looking pretty good.
> > >>
> > >
> > >Since ospfd is not suppose to write or execute files we could make the
> > >file system read only (with the exception of the control socket).
> > >
> > >(Once we can add pledge to ospfd's parent proc this will probably not
> > >make
> > >sense anymore.)
> > >
> > >
> > >
> > >cvs diff: Diffing .
> > >Index: ospfd.c
> > >===
> > >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > >retrieving revision 1.100
> > >diff -u -p -r1.100 ospfd.c
> > >--- ospfd.c29 Aug 2018 08:43:17 - 1.100
> > >+++ ospfd.c27 Oct 2018 07:28:58 -
> > >@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> > > fatalx("control socket setup failed");
> > > main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
> > >
> > >+ if (unveil("/", "r") == -1)
> > >+ fatal("unveil");
> > >+ if (unveil(ospfd_conf->csock, "c") == -1)
> > >+ fatal("unveil");
> > >+ if (unveil(NULL, NULL) == -1)
> > >+
Re: unveil ospfd's parent proc
Ricardo Mestre(ser...@helheim.mooo.com) on 2018.10.28 17:26:24 +:
> Correct, and I'd go even further by not unveiling the socket at all. A few
> weeks ago I removed the logic of unlinking the socket when the program stops,
> for a few daemons, but left untouched the ones that don't have the main
> process
> pledged since it wouldn't make much difference.
>
> If we want to go this way then I think we also should remove the logic for
> these daemons and adding unveil just like the below.
>
> Comments? OK?
in ospfd, ospf6d (and hopefully soon bgpd) we make sure the daemon does not
run twice by checking the socket.
Since it really is bad if you run two ospfd, please keep this.
/Benno
>
> Index: control.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/control.c,v
> retrieving revision 1.45
> diff -u -p -u -r1.45 control.c
> --- control.c 29 Aug 2018 08:43:16 - 1.45
> +++ control.c 28 Oct 2018 17:24:46 -
> @@ -124,16 +124,6 @@ control_listen(void)
> return (0);
> }
>
> -void
> -control_cleanup(char *path)
> -{
> - if (path == NULL)
> - return;
> - event_del(&control_state.ev);
> - event_del(&control_state.evt);
> - unlink(path);
> -}
> -
> /* ARGSUSED */
> void
> control_accept(int listenfd, short event, void *bula)
> Index: control.h
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/control.h,v
> retrieving revision 1.7
> diff -u -p -u -r1.7 control.h
> --- control.h 29 Aug 2018 08:43:16 - 1.7
> +++ control.h 28 Oct 2018 17:24:46 -
> @@ -40,6 +40,5 @@ int control_listen(void);
> void control_accept(int, short, void *);
> void control_dispatch_imsg(int, short, void *);
> int control_imsg_relay(struct imsg *);
> -void control_cleanup(char *);
>
> #endif /* _CONTROL_H_ */
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.100
> diff -u -p -u -r1.100 ospfd.c
> --- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> +++ ospfd.c 28 Oct 2018 17:24:46 -
> @@ -288,6 +288,11 @@ main(int argc, char *argv[])
> area_del(a);
> }
>
> + if (unveil("/", "r") == -1)
> + fatal("unveil");
> + if (unveil(NULL, NULL) == -1)
> + fatal("unveil");
> +
> event_dispatch();
>
> ospfd_shutdown();
> @@ -308,7 +313,6 @@ ospfd_shutdown(void)
> msgbuf_clear(&iev_rde->ibuf.w);
> close(iev_rde->ibuf.fd);
>
> - control_cleanup(ospfd_conf->csock);
> while ((r = SIMPLEQ_FIRST(&ospfd_conf->redist_list)) != NULL) {
> SIMPLEQ_REMOVE_HEAD(&ospfd_conf->redist_list, entry);
> free(r);
>
> On 15:58 Sun 28 Oct , Florian Obser wrote:
> > Sorry, I'm on a phone. The diff context looks like the control FD is
> > already open at this point. Does ospfd later re-open it?
> >
> > On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> > wrote:
> > >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> > >> Remi Locherer wrote:
> > >>
> > >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> > >> > > This breaks usage of the "include" keyword. Something that all
> > >the parse.y daemons support.
> > >> > >
> > >> >
> > >> > Oh, of course!
> > >> >
> > >> > I guess this is similar to unveil files based on a list of command
> > >line args.
> > >>
> > >> correct.
> > >>
> > >> Now that unveil is used in the tree, there are 3 types of programs
> > >>
> > >> 1) they use unveil
> > >> 2) they use pledge, heading close towards "stdio" without a "*path"
> > >> 3) they access arbitrary files based upon argv
> > >>
> > >> this is (3), except not argv, it nested inside the config file
> > >>
> > >> Well there are maybe 20 programs beyond that which aren't converted
> > >yet,
> > >> but things are looking pretty good.
> > >>
> > >
> > >Since ospfd is not suppose to write or execute files we could make the
> > >file system read only (with the exception of the control socket).
> > >
> > >(Once we can add pledge to ospfd's parent proc this will probably not
> > >make
> > >sense anymore.)
> > >
> > >
> > >
> > >cvs diff: Diffing .
> > >Index: ospfd.c
> > >===
> > >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > >retrieving revision 1.100
> > >diff -u -p -r1.100 ospfd.c
> > >--- ospfd.c29 Aug 2018 08:43:17 - 1.100
> > >+++ ospfd.c27 Oct 2018 07:28:58 -
> > >@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> > > fatalx("control socket setup failed");
> > > main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
> > >
> > >+ if (unveil("/", "r") == -1)
> > >+ fatal("unveil");
> > >+ if (unveil(ospfd_conf->csock, "c") == -1)
> > >+ fatal("unveil");
> > >+ if (unveil(NULL,
Re: unveil ospfd's parent proc
Correct, and I'd go even further by not unveiling the socket at all. A few
weeks ago I removed the logic of unlinking the socket when the program stops,
for a few daemons, but left untouched the ones that don't have the main process
pledged since it wouldn't make much difference.
If we want to go this way then I think we also should remove the logic for
these daemons and adding unveil just like the below.
Comments? OK?
Index: control.c
===
RCS file: /cvs/src/usr.sbin/ospfd/control.c,v
retrieving revision 1.45
diff -u -p -u -r1.45 control.c
--- control.c 29 Aug 2018 08:43:16 - 1.45
+++ control.c 28 Oct 2018 17:24:46 -
@@ -124,16 +124,6 @@ control_listen(void)
return (0);
}
-void
-control_cleanup(char *path)
-{
- if (path == NULL)
- return;
- event_del(&control_state.ev);
- event_del(&control_state.evt);
- unlink(path);
-}
-
/* ARGSUSED */
void
control_accept(int listenfd, short event, void *bula)
Index: control.h
===
RCS file: /cvs/src/usr.sbin/ospfd/control.h,v
retrieving revision 1.7
diff -u -p -u -r1.7 control.h
--- control.h 29 Aug 2018 08:43:16 - 1.7
+++ control.h 28 Oct 2018 17:24:46 -
@@ -40,6 +40,5 @@ int control_listen(void);
void control_accept(int, short, void *);
void control_dispatch_imsg(int, short, void *);
intcontrol_imsg_relay(struct imsg *);
-void control_cleanup(char *);
#endif /* _CONTROL_H_ */
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.100
diff -u -p -u -r1.100 ospfd.c
--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
+++ ospfd.c 28 Oct 2018 17:24:46 -
@@ -288,6 +288,11 @@ main(int argc, char *argv[])
area_del(a);
}
+ if (unveil("/", "r") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
ospfd_shutdown();
@@ -308,7 +313,6 @@ ospfd_shutdown(void)
msgbuf_clear(&iev_rde->ibuf.w);
close(iev_rde->ibuf.fd);
- control_cleanup(ospfd_conf->csock);
while ((r = SIMPLEQ_FIRST(&ospfd_conf->redist_list)) != NULL) {
SIMPLEQ_REMOVE_HEAD(&ospfd_conf->redist_list, entry);
free(r);
On 15:58 Sun 28 Oct , Florian Obser wrote:
> Sorry, I'm on a phone. The diff context looks like the control FD is already
> open at this point. Does ospfd later re-open it?
>
> On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> wrote:
> >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> >> Remi Locherer wrote:
> >>
> >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> >> > > This breaks usage of the "include" keyword. Something that all
> >the parse.y daemons support.
> >> > >
> >> >
> >> > Oh, of course!
> >> >
> >> > I guess this is similar to unveil files based on a list of command
> >line args.
> >>
> >> correct.
> >>
> >> Now that unveil is used in the tree, there are 3 types of programs
> >>
> >> 1) they use unveil
> >> 2) they use pledge, heading close towards "stdio" without a "*path"
> >> 3) they access arbitrary files based upon argv
> >>
> >> this is (3), except not argv, it nested inside the config file
> >>
> >> Well there are maybe 20 programs beyond that which aren't converted
> >yet,
> >> but things are looking pretty good.
> >>
> >
> >Since ospfd is not suppose to write or execute files we could make the
> >file system read only (with the exception of the control socket).
> >
> >(Once we can add pledge to ospfd's parent proc this will probably not
> >make
> >sense anymore.)
> >
> >
> >
> >cvs diff: Diffing .
> >Index: ospfd.c
> >===
> >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> >retrieving revision 1.100
> >diff -u -p -r1.100 ospfd.c
> >--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> >+++ ospfd.c 27 Oct 2018 07:28:58 -
> >@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> > fatalx("control socket setup failed");
> > main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
> >
> >+if (unveil("/", "r") == -1)
> >+fatal("unveil");
> >+if (unveil(ospfd_conf->csock, "c") == -1)
> >+fatal("unveil");
> >+if (unveil(NULL, NULL) == -1)
> >+fatal("unveil");
> >+
> > if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
> > ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
> > fatalx("kr_init failed");
Re: unveil ospfd's parent proc
Sorry, I'm on a phone. The diff context looks like the control FD is already
open at this point. Does ospfd later re-open it?
On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
wrote:
>On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
>> Remi Locherer wrote:
>>
>> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
>> > > This breaks usage of the "include" keyword. Something that all
>the parse.y daemons support.
>> > >
>> >
>> > Oh, of course!
>> >
>> > I guess this is similar to unveil files based on a list of command
>line args.
>>
>> correct.
>>
>> Now that unveil is used in the tree, there are 3 types of programs
>>
>> 1) they use unveil
>> 2) they use pledge, heading close towards "stdio" without a "*path"
>> 3) they access arbitrary files based upon argv
>>
>> this is (3), except not argv, it nested inside the config file
>>
>> Well there are maybe 20 programs beyond that which aren't converted
>yet,
>> but things are looking pretty good.
>>
>
>Since ospfd is not suppose to write or execute files we could make the
>file system read only (with the exception of the control socket).
>
>(Once we can add pledge to ospfd's parent proc this will probably not
>make
>sense anymore.)
>
>
>
>cvs diff: Diffing .
>Index: ospfd.c
>===
>RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
>retrieving revision 1.100
>diff -u -p -r1.100 ospfd.c
>--- ospfd.c29 Aug 2018 08:43:17 - 1.100
>+++ ospfd.c27 Oct 2018 07:28:58 -
>@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> fatalx("control socket setup failed");
> main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
>
>+ if (unveil("/", "r") == -1)
>+ fatal("unveil");
>+ if (unveil(ospfd_conf->csock, "c") == -1)
>+ fatal("unveil");
>+ if (unveil(NULL, NULL) == -1)
>+ fatal("unveil");
>+
> if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
> ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
> fatalx("kr_init failed");
Re: unveil ospfd's parent proc
Sebastian Benoit wrote: > Remi Locherer(remi.loche...@relo.ch) on 2018.10.27 23:25:58 +0200: > > On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote: > > > Remi Locherer wrote: > > > > > > > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote: > > > > > This breaks usage of the "include" keyword. Something that all the > > > > > parse.y daemons support. > > > > > > > > > > > > > Oh, of course! > > > > > > > > I guess this is similar to unveil files based on a list of command line > > > > args. > > > > > > correct. > > > > > > Now that unveil is used in the tree, there are 3 types of programs > > > > > > 1) they use unveil > > > 2) they use pledge, heading close towards "stdio" without a "*path" > > > 3) they access arbitrary files based upon argv > > > > > > this is (3), except not argv, it nested inside the config file > > > > > > Well there are maybe 20 programs beyond that which aren't converted yet, > > > but things are looking pretty good. > > > > > > > Since ospfd is not suppose to write or execute files we could make the > > file system read only (with the exception of the control socket). > > > > (Once we can add pledge to ospfd's parent proc this will probably not make > > sense anymore.) > > I first wanted to say that this makes it impossible to change the socket... > until i realized that ospfd does not have that option in ospfd.conf (only > on the command line). > > Which means i'm fine with this. ok benno@ It is such a nice containment, we should perhaps look at other daemons which do re-parsing, and see if they should lose the ability to create files.
Re: unveil ospfd's parent proc
Remi Locherer(remi.loche...@relo.ch) on 2018.10.27 23:25:58 +0200:
> On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> > Remi Locherer wrote:
> >
> > > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> > > > This breaks usage of the "include" keyword. Something that all the
> > > > parse.y daemons support.
> > > >
> > >
> > > Oh, of course!
> > >
> > > I guess this is similar to unveil files based on a list of command line
> > > args.
> >
> > correct.
> >
> > Now that unveil is used in the tree, there are 3 types of programs
> >
> > 1) they use unveil
> > 2) they use pledge, heading close towards "stdio" without a "*path"
> > 3) they access arbitrary files based upon argv
> >
> > this is (3), except not argv, it nested inside the config file
> >
> > Well there are maybe 20 programs beyond that which aren't converted yet,
> > but things are looking pretty good.
> >
>
> Since ospfd is not suppose to write or execute files we could make the
> file system read only (with the exception of the control socket).
>
> (Once we can add pledge to ospfd's parent proc this will probably not make
> sense anymore.)
I first wanted to say that this makes it impossible to change the socket...
until i realized that ospfd does not have that option in ospfd.conf (only
on the command line).
Which means i'm fine with this. ok benno@
> cvs diff: Diffing .
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.100
> diff -u -p -r1.100 ospfd.c
> --- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> +++ ospfd.c 27 Oct 2018 07:28:58 -
> @@ -278,6 +278,13 @@ main(int argc, char *argv[])
> fatalx("control socket setup failed");
> main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
>
> + if (unveil("/", "r") == -1)
> + fatal("unveil");
> + if (unveil(ospfd_conf->csock, "c") == -1)
> + fatal("unveil");
> + if (unveil(NULL, NULL) == -1)
> + fatal("unveil");
> +
> if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
> ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
> fatalx("kr_init failed");
>
Re: unveil ospfd's parent proc
Remi Locherer wrote: > On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote: > > Remi Locherer wrote: > > > > > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote: > > > > This breaks usage of the "include" keyword. Something that all the > > > > parse.y daemons support. > > > > > > > > > > Oh, of course! > > > > > > I guess this is similar to unveil files based on a list of command line > > > args. > > > > correct. > > > > Now that unveil is used in the tree, there are 3 types of programs > > > > 1) they use unveil > > 2) they use pledge, heading close towards "stdio" without a "*path" > > 3) they access arbitrary files based upon argv > > > > this is (3), except not argv, it nested inside the config file > > > > Well there are maybe 20 programs beyond that which aren't converted yet, > > but things are looking pretty good. > > > > Since ospfd is not suppose to write or execute files we could make the > file system read only (with the exception of the control socket). Indeed -- isn't a unveil a funny system?
Re: unveil ospfd's parent proc
On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> Remi Locherer wrote:
>
> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> > > This breaks usage of the "include" keyword. Something that all the
> > > parse.y daemons support.
> > >
> >
> > Oh, of course!
> >
> > I guess this is similar to unveil files based on a list of command line
> > args.
>
> correct.
>
> Now that unveil is used in the tree, there are 3 types of programs
>
> 1) they use unveil
> 2) they use pledge, heading close towards "stdio" without a "*path"
> 3) they access arbitrary files based upon argv
>
> this is (3), except not argv, it nested inside the config file
>
> Well there are maybe 20 programs beyond that which aren't converted yet,
> but things are looking pretty good.
>
Since ospfd is not suppose to write or execute files we could make the
file system read only (with the exception of the control socket).
(Once we can add pledge to ospfd's parent proc this will probably not make
sense anymore.)
cvs diff: Diffing .
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.100
diff -u -p -r1.100 ospfd.c
--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
+++ ospfd.c 27 Oct 2018 07:28:58 -
@@ -278,6 +278,13 @@ main(int argc, char *argv[])
fatalx("control socket setup failed");
main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
+ if (unveil("/", "r") == -1)
+ fatal("unveil");
+ if (unveil(ospfd_conf->csock, "c") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
fatalx("kr_init failed");
Re: unveil ospfd's parent proc
On 2018/10/26 18:15, Remi Locherer wrote: > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote: > > This breaks usage of the "include" keyword. Something that all the parse.y > > daemons support. > > > > Oh, of course! > > I guess this is similar to unveil files based on a list of command line args. Similar, but with parse.y daemons, it can change at config reload time too.
Re: unveil ospfd's parent proc
Remi Locherer wrote: > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote: > > This breaks usage of the "include" keyword. Something that all the parse.y > > daemons support. > > > > Oh, of course! > > I guess this is similar to unveil files based on a list of command line args. correct. Now that unveil is used in the tree, there are 3 types of programs 1) they use unveil 2) they use pledge, heading close towards "stdio" without a "*path" 3) they access arbitrary files based upon argv this is (3), except not argv, it nested inside the config file Well there are maybe 20 programs beyond that which aren't converted yet, but things are looking pretty good.
Re: unveil ospfd's parent proc
On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> This breaks usage of the "include" keyword. Something that all the parse.y
> daemons support.
>
Oh, of course!
I guess this is similar to unveil files based on a list of command line args.
> On October 26, 2018 5:26:06 PM GMT+02:00, Remi Locherer
> wrote:
> >Hi,
> >
> >this restricts ospfd's parent process to only read it's config file
> >(reload)
> >and unlink the control socket on exit. I added unveil after the setup
> >of
> >the control socket is done since chmod is used in control_init.
> >
> >OK?
> >
> >Remi
> >
> >
> >Index: ospfd.c
> >===
> >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> >retrieving revision 1.100
> >diff -u -p -r1.100 ospfd.c
> >--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> >+++ ospfd.c 26 Oct 2018 15:10:08 -
> >@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> > fatalx("control socket setup failed");
> > main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
> >
> >+if (unveil(conffile, "r") == -1)
> >+fatal("unveil");
> >+if (unveil(ospfd_conf->csock, "c") == -1)
> >+fatal("unveil");
> >+if (unveil(NULL, NULL) == -1)
> >+fatal("unveil");
> >+
> > if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
> > ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
> > fatalx("kr_init failed");
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: unveil ospfd's parent proc
This breaks usage of the "include" keyword. Something that all the parse.y
daemons support.
On October 26, 2018 5:26:06 PM GMT+02:00, Remi Locherer
wrote:
>Hi,
>
>this restricts ospfd's parent process to only read it's config file
>(reload)
>and unlink the control socket on exit. I added unveil after the setup
>of
>the control socket is done since chmod is used in control_init.
>
>OK?
>
>Remi
>
>
>Index: ospfd.c
>===
>RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
>retrieving revision 1.100
>diff -u -p -r1.100 ospfd.c
>--- ospfd.c29 Aug 2018 08:43:17 - 1.100
>+++ ospfd.c26 Oct 2018 15:10:08 -
>@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> fatalx("control socket setup failed");
> main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
>
>+ if (unveil(conffile, "r") == -1)
>+ fatal("unveil");
>+ if (unveil(ospfd_conf->csock, "c") == -1)
>+ fatal("unveil");
>+ if (unveil(NULL, NULL) == -1)
>+ fatal("unveil");
>+
> if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
> ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
> fatalx("kr_init failed");
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
unveil ospfd's parent proc
Hi,
this restricts ospfd's parent process to only read it's config file (reload)
and unlink the control socket on exit. I added unveil after the setup of
the control socket is done since chmod is used in control_init.
OK?
Remi
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.100
diff -u -p -r1.100 ospfd.c
--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
+++ ospfd.c 26 Oct 2018 15:10:08 -
@@ -278,6 +278,13 @@ main(int argc, char *argv[])
fatalx("control socket setup failed");
main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
+ if (unveil(conffile, "r") == -1)
+ fatal("unveil");
+ if (unveil(ospfd_conf->csock, "c") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
fatalx("kr_init failed");
