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 <c...@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>
> > ++#endif
> > +
> > + #include "config.h"
> > + #include "irc.h"
> > +@@ -39,6 +42,9 @@
> > + #include "options.h"
> > + #include "memory.h"
> > + #include "main.h"
> > ++#if defined(__OpenBSD__)
> > ++#include "match.h"
> > ++#endif
> > +
> > +
> > + static int RESTART; /* Flagged to restart on next cycle */
> > +@@ -70,6 +76,50 @@ setup_corelimit(void)
> > + }
> > + }
> > +
> > ++#if defined(__OpenBSD__)
> > ++static void
> > ++setup_pledge(void) {
> > ++ char promises[256] = "stdio inet dns exec";
> > ++
> > ++ if (unveil(HOPM_BINPATH, "x") == -1) {
> > ++ err(1, "unveil");
> > ++ }
> > ++
> > ++ if (IRCItem.tls) {
> > ++ strlcat(promises, " rpath", sizeof(promises));
> > ++
> > ++ if (unveil("/etc/ssl/cert.pem", "r") == -1) {
> > ++ err(1, "unveil");
> > ++ }
> > ++
> > ++ if (!EmptyString(IRCItem.rsa_private_key_file) &&
> > ++ !EmptyString(IRCItem.tls_certificate_file)) {
> > ++ if (unveil("IRCItem.rsa_private_key", "r") == -1) {
> > ++ err(1, "unveil");
> > ++ }
> > ++
> > ++ if (unveil("IRCItem.tls_certificate_file", "r") == -1) {
> > ++ err(1, "unveil");
> > ++ }
> > ++ }
> > ++ }
> > ++
> > ++ if (!EmptyString(OpmItem.dnsbl_to) &&
> > ++ !EmptyString(OpmItem.dnsbl_from) &&
> > ++ !EmptyString(OpmItem.sendmail)) {
> > ++ strlcat(promises, " proc", sizeof(promises));
> > ++
> > ++ if (unveil("/bin/sh", "x") == -1) {
> > ++ err(1, "unveil");
> > ++ }
> > ++ }
> > ++
> > ++ if (pledge(promises, NULL) == -1) {
> > ++ err(1, "pledge");
> > ++ }
> > ++}
> > ++#endif
> > ++
> > + static void
> > + do_signal(int signum)
> > + {
> > +@@ -199,6 +249,10 @@ main(int argc, char *argv[])
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > ++#if defined(__OpenBSD__)
> > ++ setup_pledge();
> > ++#endif
> > ++
> > + /* Setup alarm & int handlers. */
> > + ALARMACTION.sa_handler = &do_signal;
> > + ALARMACTION.sa_flags = SA_RESTART;
> >
>
> ping
> --
> Chaz
ping
--
Chaz
