On Mon, Nov 11, 2024 at 10:37:21PM -0500, Chaz Kettleson wrote:
> On Wed, Sep 04, 2024 at 08:27:32AM -0400, Chaz Kettleson wrote:
> >
> > On Fri, Aug 30, 2024 at 08:44:12PM GMT, Theo de Raadt wrote:
> > > Chaz Kettleson <chaz@pyr3x.com> wrote:
> > >
> > > > My general thought here was since I only needed wpath/cpath for pid/log
> > > > files, and I was not going to patch for syslog (still need to write pid
> > > > anyway), I would at least unveil for only those files. The idea of
> > > > unveil("/", "") just seemed like a sane default from other domains where
> > > > a "block all, explicitly allow" makes sense.
> > >
> > > It is not sane. But also, it is not idiomatic. You can't find this in
> > > any other code. You made it up, it's an assumption that "everything
> > > possible should be used, it is all free". Try to explain what this does
> > > and why it is needed and why noone else uses it? You won't find a reason.
> > >
> >
> > Indeed. I think I got a bit carried away with excitement trying out
> > pledge/unveil. I've taken some time to study how pledge/unveil are used
> > in other ports, as well as in base. I've also took some time to study
> > additional code paths of the port based on options selected. I'll cook
> > some new diffs based on this analysis for review.
> >
> > Thanks again for all the feedback!
> >
> > --
> > Chaz
> >
>
> Hello,
>
> I took another stab at adding pledge/unveil to this port. I'd appreciate
> any feedback as I'm still learning. Since this is a port I didn't want
> to do any major surgery in moving things around from upstream.
>
> After reviewing the code I decided I would wait until all configuration
> has occurred before applying pledge/unveil. This includes reading any
> configuration files and opening any log/pid files. At this point the
> base set of promises are "stdio inet dns exec" before the main loop.
>
> exec is only needed to execute a new process for restarting. We enforce
> this with unveil(HOPM_BINPATH, "x")
>
> if configured for TLS, rpath is added. This is because certs need to be
> read. One thought was to pledge after the certs were loaded, however,
> this will not account for the client automatically reconnecting after a
> disconnect. As a result the promise is given and coupled with unveil for
> the specific files that need to be read.
>
> if configured for sending email, proc is added. This is because a call
> to popen(3) which creates the pipe, forks, and invokes the shell to
> 'sendmail -t' a crafted email to add an entry to a dns blocklist. We'll
> need to unveil /bin/sh. I tried to lock this down using execpromises but
> always got a permission denied from the mailwrapper. I've also added the
> "match.h" for the EmptyString checks to re-use the same logic the
> upstream code does.
>
> I wanted to ask a few questions:
>
> 1.) Is there any value in using unveil here at all for the uses I
> described above?
> 2.) Is my use of the #if defined correct, particularly for eventually
> upstreaming?
> 3.) Is the use of strlcat idiomatic for building the set of promises
> based on the configuration options that were set?
>
> Thanks again for reviewing and feedback -- I'm (hopefully) learning!
>
> --
> Chaz
>
> diff --git a/net/hopm/patches/patch-src_main_c b/net/hopm/patches/patch-src_main_c
> new file mode 100644
> index 00000000000..1b806587920
> --- /dev/null
> +++ b/net/hopm/patches/patch-src_main_c
> @@ -0,0 +1,87 @@
> +add pledge/unveil
> +
> +Index: src/main.c
> +--- src/main.c.orig
> ++++ src/main.c
> +@@ -30,6 +30,9 @@
> + #include <fcntl.h>
> + #include <stdlib.h>
> + #include <string.h>
> ++#if defined(__OpenBSD__)
> ++#include <err.h>
> ++
No comments:
Post a Comment