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
