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 +0000: > > 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 -0000 1.45 > > +++ control.c 28 Oct 2018 17:24:46 -0000 > > @@ -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 -0000 1.7 > > +++ control.h 28 Oct 2018 17:24:46 -0000 > > @@ -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 -0000 1.100 > > +++ ospfd.c 28 Oct 2018 17:24:46 -0000 > > @@ -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 > > > <remi.loche...@relo.ch> wrote: > > > >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote: > > > >> Remi Locherer <remi.loche...@relo.ch> 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 -0000 1.100 > > > >+++ ospfd.c 27 Oct 2018 07:28:58 -0000 > > > >@@ -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"); > > >