[PATCH 1/2] OpenBSD: Always unveil

[PATCH 1/2] OpenBSD: Always unveil

From: Klemens Nanni
To: list+catgirl
Cc: Klemens Nanni
Even though catgirl never ends up with any write/create permissions on
the filesystem outside of the logdir, explicitly unveiling `/' (root)
executable-only guards against any programming mistakes while also
documenting the behaviour through code.

This also decouples `log' and `restrict' semantics further, simplifing
logic.

Note that `logdir' must be unveiled first, otherwise unveiling root
hides the data directory and creating `logdir' prior to unveiling it
fails (obvious in hindsight but worth mentioning nonetheless).
---
 chat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/chat.c b/chat.c
index 9934fc3..0bdb69c 100644
--- a/chat.c
+++ b/chat.c
@@ -282,12 +282,17 @@ int main(int argc, char *argv[]) {
 	}
 
 #ifdef __OpenBSD__
-	if (self.restricted && log) {
+	if (log) {
 		const char *logdir = dataMkdir("log");
 		int error = unveil(logdir, "wc");
 		if (error) err(EX_OSERR, "unveil");
 	}
 
+	if (!self.restricted) {
+		int error = unveil("/", "x");
+		if (error) err(EX_OSERR, "unveil");
+	}
+
 	char promises[64] = "stdio tty";
 	char *ptr = &promises[strlen(promises)], *end = &promises[sizeof(promises)];
 	if (log) ptr = seprintf(ptr, end, " wpath cpath");
-- 
2.32.0
1 reply

[PATCH 2/2] OpenBSD: merge unveil and pledge logic a bit

From: Klemens Nanni
To: list+catgirl
Cc: Klemens Nanni
This reads somewhat clearer as code is grouped by features instead of
security mechanisms by simply merging identical tests/conditions.

No functional change.
---
 chat.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/chat.c b/chat.c
index 0bdb69c..ab0678a 100644
--- a/chat.c
+++ b/chat.c
@@ -282,24 +282,23 @@ int main(int argc, char *argv[]) {
 	}
 
 #ifdef __OpenBSD__
+	char promises[64] = "stdio tty";
+	char *ptr = &promises[strlen(promises)], *end = &promises[sizeof(promises)];
+
 	if (log) {
 		const char *logdir = dataMkdir("log");
 		int error = unveil(logdir, "wc");
 		if (error) err(EX_OSERR, "unveil");
+		ptr = seprintf(ptr, end, " wpath cpath");
 	}
 
 	if (!self.restricted) {
 		int error = unveil("/", "x");
 		if (error) err(EX_OSERR, "unveil");
+		ptr = seprintf(ptr, end, " proc exec");
 	}
 
-	char promises[64] = "stdio tty";
-	char *ptr = &promises[strlen(promises)], *end = &promises[sizeof(promises)];
-	if (log) ptr = seprintf(ptr, end, " wpath cpath");
-	if (!self.restricted) ptr = seprintf(ptr, end, " proc exec");
-
 	char *promisesInitial = ptr;
-
 	ptr = seprintf(ptr, end, " inet dns");
 	int error = pledge(promises, NULL);
 	if (error) err(EX_OSERR, "pledge");
-- 
2.32.0
2 replies

Re: [PATCH 1/2] OpenBSD: Always unveil

From: Klemens Nanni
To: list+catgirl
On Tue, Jun 29, 2021 at 12:02:59AM +0000, Klemens Nanni wrote:
> ...

Here is a more accurate commit message:  obviously unveil() is called
under conditions so "always" is wrong, I meant the "no restrict mode"
case really.

Same code diff as before.

-- >8 --

From 2546dc41059c40211e423ac9889aa6200e09fc32 Mon Sep 17 00:00:00 2001
From: Klemens Nanni <klemens@posteo.de>
Date: Tue, 29 Jun 2021 03:21:42 +0200
Subject: [PATCH 1/2] OpenBSD: unveil logs regardless of restrict mode

Simplify logic and decouple the two features such that the code gets
even more self-ducumenting.

Previously `catgirl -R -l' would never unveil and therefore "proc exec"
could execute arbitrary paths without "rpath" as is usual unveil/pledge
semantic.

Now that `catgirl -l' alone triggers unveil(2), previous "proc exec"
alone is not enough since the first unveil() hides everything else from
filesystem;  unveil all of root executable-only in order to restore
non-restrict mode's visibility.

This leaves yields distinct cases wrt. filesystem visibility
(hoisted save file functionality excluded):

1. restrict on,  log off:  no access
2. restrict on,  log on :  logdir write/create
3. restrict off, log off:  all exec-only
4. restrict off, log on :  logdir write/create, all else exec-only

In the first case `unveil("/", "")' could be used but with no benefit as
the later lack of "rpath wpath cpath", i.e. filesystem access is revoked
entirely by pledge alone already.

Practically, this does not change functionality but improves correctness
and readability.
---
 chat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/chat.c b/chat.c
index 9934fc3..0bdb69c 100644
--- a/chat.c
+++ b/chat.c
@@ -282,12 +282,17 @@ int main(int argc, char *argv[]) {
 	}
 
 #ifdef __OpenBSD__
-	if (self.restricted && log) {
+	if (log) {
 		const char *logdir = dataMkdir("log");
 		int error = unveil(logdir, "wc");
 		if (error) err(EX_OSERR, "unveil");
 	}
 
+	if (!self.restricted) {
+		int error = unveil("/", "x");
+		if (error) err(EX_OSERR, "unveil");
+	}
+
 	char promises[64] = "stdio tty";
 	char *ptr = &promises[strlen(promises)], *end = &promises[sizeof(promises)];
 	if (log) ptr = seprintf(ptr, end, " wpath cpath");
-- 
2.32.0