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;
