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 <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
>
ping
--
Chaz
