On Sun, Oct 28, 2018 at 07:24:23PM +0100, Sebastian Benoit wrote:
> Ricardo Mestre([email protected]) 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
> > > <[email protected]> wrote:
> > > >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> > > >> Remi Locherer <[email protected]> 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");
> >
>
