Re: sysutils/openbsdisks2: unveil to prevent execution

On Wed, Nov 08, 2023 at 11:04:01AM +0000, Klemens Nanni wrote:
> This service seems like a common dependency for desktop environments
> and runs as root speaking D-Bus without any activesecurity mechanisms.
>
> ioctl(2) for cd(4) and sysctl(2) hw.disknames usage currently prevents
> using pledge(2).
>
> Use unveil("/", "rwc") for starters to strip x bits as, by design, this
> daemon is not executing anything (it spawns a thread, though).

Unlike me, semarie can actually read code and pointed out the obvious
QProcess usage to run mount_*(8) and umount(8).

Updated diff to account for that.
# pkg_add kf5-dolphin
$ dolphin
shows disks, but fails to mount/unmount USB sticks even without my diff.
The code looks wrong, rsadowski reports it used to work and will check.

> Perhaps "c" could be dropped as well, but I haven't looked that far into
> its Qt and D-Bus tentacles to check whether it does indeed never tries
> to create any files.

It creates/deletes /media/ and subdirs at runtime, so as per unveil(2)
unveil("/", "rw") + unveil("/media", "rwc") won't work, iiuc:

Directories are remembered at the time of a
call to unveil(). This means that a directory that is removed and
recreated after a call to unveil() will appear to not exist.


Index: Makefile
===================================================================
RCS file: /cvs/ports/sysutils/openbsdisks2/Makefile,v
diff -u -p -r1.8 Makefile
--- Makefile 27 Sep 2023 17:16:32 -0000 1.8
+++ Makefile 9 Nov 2023 09:14:21 -0000
@@ -2,6 +2,7 @@ COMMENT = UDisks2 service implementation

V = 0.3.1
DISTNAME = openbsdisks2-${V}
+REVISION = 0

CATEGORIES = sysutils

@@ -15,6 +16,7 @@ PERMIT_PACKAGE = Yes
# C++
COMPILER = base-clang ports-gcc

+# uses unveil()
WANTLIB += ${COMPILER_LIBCXX} Qt5Core Qt5DBus c m util

SITES = https://github.com/sizeofvoid/openbsdisks2/releases/download/v${V}/
Index: patches/patch-src_main_cpp
===================================================================
RCS file: patches/patch-src_main_cpp
diff -N patches/patch-src_main_cpp
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_main_cpp 9 Nov 2023 09:51:20 -0000
@@ -0,0 +1,42 @@
+Uncovered hw.disknames sysctl(2) and cd(4) ioctl(2) prevents pledge(2) usage.
+unveil(2) all files to
+- limit execution except to /sbin/mount_* and /sbin/umount
+
+Index: src/main.cpp
+--- src/main.cpp.orig
++++ src/main.cpp
+@@ -34,8 +34,10 @@
+ #include "manageradaptor.h"
+ #include "objectmanager.h"
+
++#include <err.h>
+ #include <iostream>
+ #include <syslog.h>
++#include <unistd.h>
+
+ #include <QSet>
+
+@@ -84,6 +86,23 @@ static void msg_handler(QtMsgType type, const QMessage
+
+ int main(int argc, char** argv)
+ {
++ if (unveil("/", "rwc") == -1)
++ err(1, "unveil /");
++ if (unveil("/sbin/umount", "rx") == -1)
++ err(1, "unveil /sbin/umount");
++ if (unveil("/sbin/mount_ffs", "rx") == -1)
++ err(1, "unveil /sbin/mount_ffs");
++ if (unveil("/sbin/mount_ext2fs", "rx") == -1)
++ err(1, "unveil /sbin/mount_ext2fs");
++ if (unveil("/sbin/mount_ntfs", "rx") == -1)
++ err(1, "unveil /sbin/mount_ntfs");
++ if (unveil("/sbin/mount_msdos", "rx") == -1)
++ err(1, "unveil /sbin/mount_msdos");
++ if (unveil("/sbin/mount_cd9660", "rx") == -1)
++ err(1, "unveil /sbin/mount_cd9660");
++ if (unveil(NULL, NULL) == -1)
++ err(1, "unveil NULL");
++
+ qInstallMessageHandler(msg_handler);
+
+ qRegisterMetaType<Configuration>();