On Sun, Oct 28, 2018 at 05:26:24PM +0000, 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 -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");
>
