On Thu, Dec 19, 2024 at 10:04:36AM -0500, Chaz Kettleson wrote:
> On Mon, Dec 09, 2024 at 06:21:06PM -0500, Chaz Kettleson wrote:
> > On Mon, Dec 02, 2024 at 08:29:54AM -0500, Chaz Kettleson wrote:
> > > 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