[patch] sandbox-fchown-fchmod-master.patch

sandbox-fchown-fchmod-master.patch (text/plain), 11.62 KB, created by Michael Orlitzky on 2023-06-20 22:13:23 UTC

From 5e2aa7dac69b44e9a9a52d1be653657fdec60655 Mon Sep 17 00:00:00 2001
From: Michael Orlitzky <[e-mail]>
Date: Tue, 20 Jun 2023 17:58:57 -0400
Subject: [PATCH 1/3] libsandbox: add support for fchown/fchmod on linux

The fchown/fchmod functions use a file descriptor obtained from
open(), and the sandbox relies on its open() wrapper for safety. But
it turns out that fchown/fchmod can operate on a descriptor opened
O_RDONLY, which the open() wrapper is happy to give you. Oops. This is
bug 599706.

There's no POSIX way to map the descriptor to a path once you've got
it, but on linux you can use the magic path "/proc/self/fd/%i" which
should be a symlink pointing to the path passed to open(). Once we
have that path, we can use the existing "is this path safe" machinery
in the sandbox. There is precedent for this approach in sandbox, and
the SANDBOX_PROC_SELF_FD macro already exists to indicate that the
feature is available.

Bug: https://bugs.gentoo.org/599706
Signed-off-by: Michael Orlitzky <[e-mail]>
---
 libsandbox/libsandbox.c           | 17 +++++++++++++++++
 libsandbox/libsandbox.h           |  7 +++++++
 libsandbox/symbols.h.in           |  2 ++
 libsandbox/trace.c                | 13 +++++++++++++
 libsandbox/wrapper-funcs/fchmod.c | 11 +++++++++++
 libsandbox/wrapper-funcs/fchown.c | 11 +++++++++++
 6 files changed, 61 insertions(+)
 create mode 100644 libsandbox/wrapper-funcs/fchmod.c
 create mode 100644 libsandbox/wrapper-funcs/fchown.c

diff --git a/libsandbox/libsandbox.c b/libsandbox/libsandbox.c
index b9ef52e..98c2a07 100644
--- a/libsandbox/libsandbox.c
+++ b/libsandbox/libsandbox.c
@@ -766,7 +766,9 @@ static int check_access(sbcontext_t *sbcontext, int sb_nr, const char *func,
 	    sb_nr == SB_NR_CHOWN       ||
 	    sb_nr == SB_NR_CREAT       ||
 	    sb_nr == SB_NR_CREAT64     ||
+    	    sb_nr == SB_NR_FCHMOD      ||
 	    sb_nr == SB_NR_FCHMODAT    ||
+    	    sb_nr == SB_NR_FCHOWN      ||
 	    sb_nr == SB_NR_FCHOWNAT    ||
 	  /*sb_nr == SB_NR_FTRUNCATE   ||
 	    sb_nr == SB_NR_FTRUNCATE64 ||*/
@@ -1102,6 +1104,21 @@ bool before_syscall_open_int(int dirfd, int sb_nr, const char *func, const char
 	return before_syscall(dirfd, sb_nr, ext_func, file, flags);
 }
 
+bool before_syscall_fd(int sb_nr, const char *func, int fd) {
+#ifdef SANDBOX_PROC_SELF_FD
+	/* We only know how to handle e.g. fchmod() and fchown() on
+	 * linux, where it's possible to (eventually) get a path out
+	 * of the given file descriptor. The "64" below accounts for
+	 * the length of an integer string, and is probably
+	 * overkill. */
+	char path[sizeof("/proc/self/fd/") + 64];
+	snprintf(path, sizeof("/proc/self/fd/") + 64, "/proc/self/fd/%i", fd);
+	return before_syscall(AT_FDCWD, sb_nr, func, path, 0);
+#else
+	return true;
+#endif
+}
+
 bool before_syscall_open_char(int dirfd, int sb_nr, const char *func, const char *file, const char *mode)
 {
 	if (NULL == mode)
diff --git a/libsandbox/libsandbox.h b/libsandbox/libsandbox.h
index 206c506..01a4c6c 100644
--- a/libsandbox/libsandbox.h
+++ b/libsandbox/libsandbox.h
@@ -46,6 +46,11 @@
 #define  SB_SAFE_OPEN_CHAR(_path, _mode) \
          SB_SAFE_OPEN_CHAR_AT(AT_FDCWD, _path, _mode)
 
+#define _SB_SAFE_FD(_nr, _name, _fd) \
+        __SB_SAFE(before_syscall_fd(_nr, _name, fd))
+#define  SB_SAFE_FD(_fd) \
+         _SB_SAFE_FD(WRAPPER_NR, STRING_NAME, _fd)
+
 /* Symbols that don't exist in the C library will be <= this value. */
 #define SB_NR_UNDEF -99999
 #define SB_NR_IS_DEFINED(nr) (nr > SB_NR_UNDEF)
@@ -55,6 +60,8 @@ bool before_syscall(int, int, const char *, const char *, int);
 bool before_syscall_access(int, int, const char *, const char *, int);
 bool before_syscall_open_int(int, int, const char *, const char *, int);
 bool before_syscall_open_char(int, int, const char *, const char *, const char *);
+bool before_syscall_fd(int, const char *, int);
+
 enum sandbox_method_t get_sandbox_method(void);
 
 void *get_dlsym(const char *symname, const char *symver);
diff --git a/libsandbox/symbols.h.in b/libsandbox/symbols.h.in
index ecf141c..297c13a 100644
--- a/libsandbox/symbols.h.in
+++ b/libsandbox/symbols.h.in
@@ -7,8 +7,10 @@
 #     before 'creat()' as 'creat()' uses 'open()' ...
 
 chmod
+fchmod
 fchmodat
 chown
+fchown
 fchownat
 open
 __open_2
diff --git a/libsandbox/trace.c b/libsandbox/trace.c
index d70f3bc..75a749e 100644
--- a/libsandbox/trace.c
+++ b/libsandbox/trace.c
@@ -455,8 +455,21 @@ static bool trace_check_syscall(const struct syscall_entry *se, void *regs)
 		}
 		__sb_debug("})");
 		return 1;
+	} else if (nr == SB_NR_FCHMOD) {
+		int fd = trace_arg(regs, 1);
+		mode_t mode = trace_arg(regs, 2);
+		__sb_debug("(%i, %o)", fd, mode);
+		return _SB_SAFE_FD(nr, name, fd);
+
+	} else if (nr == SB_NR_FCHOWN) {
+		int fd = trace_arg(regs, 1);
+		uid_t uid = trace_arg(regs, 2);
+		gid_t gid = trace_arg(regs, 3);
+		__sb_debug("(%i, %i, %i)", fd, uid, gid);
+		return _SB_SAFE_FD(nr, name, fd);
 	}
 
+
  done:
 	__sb_debug("(...)");
 	return ret;
diff --git a/libsandbox/wrapper-funcs/fchmod.c b/libsandbox/wrapper-funcs/fchmod.c
new file mode 100644
index 0000000..04bfcea
--- /dev/null
+++ b/libsandbox/wrapper-funcs/fchmod.c
@@ -0,0 +1,11 @@
+/*
+ * fchmod() wrapper.
+ *
+ * Copyright 1999-2018 Gentoo Foundation
+ * Licensed under the GPL-2
+ */
+
+#define WRAPPER_ARGS_PROTO int fd, mode_t mode
+#define WRAPPER_ARGS fd, mode
+#define WRAPPER_SAFE() SB_SAFE_FD(fd)
+#include "__wrapper_simple.c"
diff --git a/libsandbox/wrapper-funcs/fchown.c b/libsandbox/wrapper-funcs/fchown.c
new file mode 100644
index 0000000..ab79d5c
--- /dev/null
+++ b/libsandbox/wrapper-funcs/fchown.c
@@ -0,0 +1,11 @@
+/*
+ * fchown() wrapper.
+ *
+ * Copyright 1999-2018 Gentoo Foundation
+ * Licensed under the GPL-2
+ */
+
+#define WRAPPER_ARGS_PROTO int fd, uid_t owner, gid_t group
+#define WRAPPER_ARGS fd, owner, group
+#define WRAPPER_SAFE() SB_SAFE_FD(fd)
+#include "__wrapper_simple.c"
-- 
2.39.3

From c4e90900974521c2078588648732eef8bed39fd6 Mon Sep 17 00:00:00 2001
From: Michael Orlitzky <[e-mail]>
Date: Sat, 27 Jan 2018 20:05:02 -0500
Subject: [PATCH 2/3] tests: add test case for fchown/fchmod with O_RDONLY.

Bug: https://bugs.gentoo.org/599706
Signed-off-by: Michael Orlitzky <[e-mail]>
---
 tests/fchmod-0.c  | 35 +++++++++++++++++++++++++++++++++++
 tests/fchmod-1.sh | 14 ++++++++++++++
 tests/fchmod.at   |  1 +
 tests/fchown-0.c  | 34 ++++++++++++++++++++++++++++++++++
 tests/fchown-1.sh | 14 ++++++++++++++
 tests/fchown.at   |  1 +
 tests/local.mk    |  2 ++
 7 files changed, 101 insertions(+)
 create mode 100644 tests/fchmod-0.c
 create mode 100755 tests/fchmod-1.sh
 create mode 100644 tests/fchmod.at
 create mode 100644 tests/fchown-0.c
 create mode 100755 tests/fchown-1.sh
 create mode 100644 tests/fchown.at

diff --git a/tests/fchmod-0.c b/tests/fchmod-0.c
new file mode 100644
index 0000000..de0c237
--- /dev/null
+++ b/tests/fchmod-0.c
@@ -0,0 +1,35 @@
+/*
+ * https://bugs.gentoo.org/599706
+ *
+ */
+
+#include "headers.h"
+
+int main(int argc, char *argv[])
+{
+	if (argc < 2)
+		return -2;
+
+	int mode = 0;
+	sscanf(argv[1], "%i", &mode);
+	/* The sandbox catches this:
+	 *
+	 *   int fd = open(argv[2], O_RDWR);
+	 *
+	 * And it /should/ catch this:
+	 *
+	 *    int fd = open(argv[2], O_RDONLY);
+	 *
+	 * ...but the latter only works when /proc/self/fd/%i
+	 * is available.
+	 *
+	 */
+#ifdef SANDBOX_PROC_SELF_FD
+	int fd = open(argv[2], O_RDONLY);
+#else
+	int fd = open(argv[2], O_RDWR);
+#endif
+	int fchmod_result = fchmod(fd, (mode_t)mode);
+	close(fd);
+	return fchmod_result;
+}
diff --git a/tests/fchmod-1.sh b/tests/fchmod-1.sh
new file mode 100755
index 0000000..db404ba
--- /dev/null
+++ b/tests/fchmod-1.sh
@@ -0,0 +1,14 @@
+#!/bin/sh
+#
+# https://bugs.gentoo.org/599706
+#
+
+addwrite $PWD
+
+# The sandbox doesn't log anything when it returns a junk file
+# descriptor? It doesn't look like we can test the contents of
+# sandbox.log here... instead, we just have to count on fchmod
+# failing, which it does if you use O_RDWR, and it *should* if you use
+# O_RDONLY (because that won't stop the change of permissions).
+fchmod-0 $(stat --format='%#04a' ../..) ../.. && exit 1
+exit 0
diff --git a/tests/fchmod.at b/tests/fchmod.at
new file mode 100644
index 0000000..081d7d2
--- /dev/null
+++ b/tests/fchmod.at
@@ -0,0 +1 @@
+SB_CHECK(1)
diff --git a/tests/fchown-0.c b/tests/fchown-0.c
new file mode 100644
index 0000000..7fdca73
--- /dev/null
+++ b/tests/fchown-0.c
@@ -0,0 +1,34 @@
+/*
+ * https://bugs.gentoo.org/599706
+ *
+ */
+
+#include "headers.h"
+
+int main(int argc, char *argv[])
+{
+	if (argc < 3)
+		return -2;
+
+	uid_t uid = atoi(argv[1]);
+	gid_t gid = atoi(argv[2]);
+	/* The sandbox catches this:
+	 *
+	 *   int fd = open(argv[3], O_RDWR);
+	 *
+	 * And it /should/ catch this:
+	 *
+	 *    int fd = open(argv[3], O_RDONLY);
+	 *
+	 * ...but the latter only works when /proc/self/fd/%i
+	 * is available.
+	 */
+#ifdef SANDBOX_PROC_SELF_FD
+	int fd = open(argv[3], O_RDONLY);
+#else
+	int fd = open(argv[3], O_RDWR);
+#endif
+	int fchown_result = fchown(fd, uid, gid);
+	close(fd);
+	return fchown_result;
+}
diff --git a/tests/fchown-1.sh b/tests/fchown-1.sh
new file mode 100755
index 0000000..1b4a173
--- /dev/null
+++ b/tests/fchown-1.sh
@@ -0,0 +1,14 @@
+#!/bin/sh
+#
+# https://bugs.gentoo.org/599706
+#
+
+addwrite $PWD
+
+# The sandbox doesn't log anything when it returns a junk file
+# descriptor? It doesn't look like we can test the contents of
+# sandbox.log here... instead, we just have to count on fchown
+# failing, which it does if you use O_RDWR, and it *should* if you use
+# O_RDONLY (because that won't stop the change of ownership).
+fchown-0 ${SB_UID} ${SB_GID} ../.. && exit 1
+exit 0
diff --git a/tests/fchown.at b/tests/fchown.at
new file mode 100644
index 0000000..081d7d2
--- /dev/null
+++ b/tests/fchown.at
@@ -0,0 +1 @@
+SB_CHECK(1)
diff --git a/tests/local.mk b/tests/local.mk
index 046cf6f..f1f4ac0 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -29,7 +29,9 @@ check_PROGRAMS += \
 	%D%/execv-0 \
 	%D%/execvp-0 \
 	%D%/faccessat-0 \
+	%D%/fchmod-0 \
 	%D%/fchmodat-0 \
+	%D%/fchown-0 \
 	%D%/fchownat-0 \
 	%D%/fopen-0 \
 	%D%/fopen64-0 \
-- 
2.39.3

From a209d96e920a61b0a74dc8dfd8095d623591cf57 Mon Sep 17 00:00:00 2001
From: Michael Orlitzky <[e-mail]>
Date: Sat, 27 Jan 2018 22:38:26 -0500
Subject: [PATCH 3/3] tests: add more tests to make sure fchown/fchmod are
 handled correctly.

Closes: https://bugs.gentoo.org/599706
Signed-off-by: Michael Orlitzky <[e-mail]>
---
 tests/fchmod-2.sh | 11 +++++++++++
 tests/fchmod.at   |  1 +
 tests/fchown-2.sh | 11 +++++++++++
 tests/fchown.at   |  1 +
 4 files changed, 24 insertions(+)
 create mode 100755 tests/fchmod-2.sh
 create mode 100755 tests/fchown-2.sh

diff --git a/tests/fchmod-2.sh b/tests/fchmod-2.sh
new file mode 100755
index 0000000..96d7cc9
--- /dev/null
+++ b/tests/fchmod-2.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+#
+# Ensure that fchmod() doesn't trigger spurious violations in the most
+# basic of cases.
+#
+addwrite $PWD
+
+# This should not trigger a violation.
+rm -f file
+touch file
+fchmod-0 0644 file || exit 1
diff --git a/tests/fchmod.at b/tests/fchmod.at
index 081d7d2..d364b4b 100644
--- a/tests/fchmod.at
+++ b/tests/fchmod.at
@@ -1 +1,2 @@
 SB_CHECK(1)
+SB_CHECK(2)
diff --git a/tests/fchown-2.sh b/tests/fchown-2.sh
new file mode 100755
index 0000000..dedfbe4
--- /dev/null
+++ b/tests/fchown-2.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+#
+# Ensure that fchown() doesn't trigger spurious violations in the most
+# basic of cases.
+#
+addwrite $PWD
+
+# This should not trigger a violation.
+rm -f file
+touch file
+fchown-0 ${SB_UID} ${SB_GID} file || exit 1
diff --git a/tests/fchown.at b/tests/fchown.at
index 081d7d2..d364b4b 100644
--- a/tests/fchown.at
+++ b/tests/fchown.at
@@ -1 +1,2 @@
 SB_CHECK(1)
+SB_CHECK(2)
-- 
2.39.3