[gentoo-commits] proj/sandbox:master commit in: libsandbox/, tests/

[gentoo-commits] proj/sandbox:master commit in: libsandbox/, tests/

[Mike Frysinger]

commit:     93d401570d4e54f732c0f821cdbb5ba2e1dee0f3
Author:     Mike Frysinger <vapier <AT> gentoo <DOT> org>
AuthorDate: Sun Sep 20 07:23:37 2015 +0000
Commit:     Mike Frysinger <vapier <AT> gentoo <DOT> org>
CommitDate: Sun Sep 20 07:23:37 2015 +0000
URL:        https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=93d40157

libsandbox: fix handling of dangling symlinks

Make sure we properly check the target of symlinks even when the target
does not exist.  This caused problems in two ways:
(1) It allowed code to bypass checks by writing through a symlink that
was in a good location but pointed to a bad (non-existent) location.
(2) It caused code to be wrongly rejected when it tried writing to a
symlink in a bad location but pointed to a good location.

In order to get this behavior, we need to use the new gnulib helpers
added in the previous commit.  They include functions which can look
up the targets of symlinks even when the final path doesn't exist.

URL: https://bugs.gentoo.org/540828
Reported-by: Rick Farina <zerochaos <AT> gentoo.org>
Signed-off-by: Mike Frysinger <vapier <AT> gentoo.org>

 libsandbox/libsandbox.c | 23 ++++++++++++++++++-----
 tests/script-11.sh      | 19 +++++++++++++++++++
 tests/script-12.sh      | 25 +++++++++++++++++++++++++
 tests/script.at         |  2 ++
 4 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/libsandbox/libsandbox.c b/libsandbox/libsandbox.c
index 3bd3794..57be731 100644
--- a/libsandbox/libsandbox.c
+++ b/libsandbox/libsandbox.c
@@ -240,7 +240,12 @@ static char *resolve_path(const char *path, int follow_link)
 		else
 			ret = realpath(path, filtered_path);
 
-		/* Maybe we failed because of funky anonymous fd symlinks.
+		/* Handle broken symlinks.  This can come up for a variety of reasons,
+		 * but we need to make sure that we resolve the path all the way to the
+		 * final target, and not just where the current link happens to start.
+		 * Latest discussion is in #540828.
+		 *
+		 * Maybe we failed because of funky anonymous fd symlinks.
 		 * You can see this by doing something like:
 		 *		$ echo | ls -l /proc/self/fd/
 		 *		.......	0 -> pipe:[9422999]
@@ -248,11 +253,19 @@ static char *resolve_path(const char *path, int follow_link)
 		 * actual file paths for us to check against. #288863
 		 * Don't look for any particular string as these are dynamic
 		 * according to the kernel.  You can see pipe:, socket:, etc...
+		 *
+		 * Maybe we failed because it's a symlink to a path in /proc/ that
+		 * is a symlink to a path that longer exists -- readlink will set
+		 * ENOENT even in that case and the file ends in (deleted).  This
+		 * can come up in cases like:
+		 * /dev/stderr -> fd/2 -> /proc/self/fd/2 -> /removed/file (deleted)
 		 */
-		if (!ret && !strncmp(filtered_path, "/proc/", 6)) {
-			char *base = strrchr(filtered_path, '/');
-			if (base && strchr(base, ':'))
-				ret = filtered_path;
+		if (!ret && errno == ENOENT) {
+			ret = canonicalize_filename_mode(path, CAN_ALL_BUT_LAST);
+			if (ret) {
+				free(filtered_path);
+				filtered_path = ret;
+			}
 		}
 
 		if (!ret) {

diff --git a/tests/script-11.sh b/tests/script-11.sh
new file mode 100755
index 0000000..da9bbbf
--- /dev/null
+++ b/tests/script-11.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+# handle targets of dangling symlinks correctly #540828
+[ "${at_xfail}" = "yes" ] && exit 77 # see script-0
+
+# this should fail
+mkdir subdir
+ln -s subdir/target symlink
+
+adddeny "${PWD}/subdir"
+
+echo blah >symlink
+# we should not be able to write through the symlink
+if [ $? -eq 0 ] ; then
+	exit 1
+fi
+
+test -s "${SANDBOX_LOG}"
+
+exit $?

diff --git a/tests/script-12.sh b/tests/script-12.sh
new file mode 100755
index 0000000..a80108b
--- /dev/null
+++ b/tests/script-12.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+# handle targets of dangling symlinks correctly #540828
+[ "${at_xfail}" = "yes" ] && exit 77 # see script-0
+
+# this should pass
+mkdir subdir
+ln -s subdir/target symlink
+
+# make sure the log is in a writable location
+SANDBOX_LOG="${PWD}/subdir/log"
+
+(
+# This clobbers all existing writable paths for this one write.
+SANDBOX_WRITE="${PWD}/subdir"
+echo pass >symlink
+)
+# we should be able to write through the symlink
+if [ $? -ne 0 ] ; then
+	exit 1
+fi
+
+# and not gotten a sandbox violation
+test ! -s "${SANDBOX_LOG}"
+
+exit $?

diff --git a/tests/script.at b/tests/script.at
index 93e370a..f07a8f1 100644
--- a/tests/script.at
+++ b/tests/script.at
@@ -8,3 +8,5 @@ SB_CHECK(7)
 SB_CHECK(8)
 SB_CHECK(9, [wait errpipe... done OK!])
 SB_CHECK(10)
+SB_CHECK(11)
+SB_CHECK(12)

[gentoo-commits] proj/sandbox:master commit in: libsandbox/, tests/

[Mike Frysinger]

commit:     4c47cfa22802fd8201586bef233d8161df4ff61b
Author:     Mike Frysinger <vapier <AT> gentoo <DOT> org>
AuthorDate: Fri Mar 10 18:15:50 2017 +0000
Commit:     Mike Frysinger <vapier <AT> gentoo <DOT> org>
CommitDate: Fri Mar 10 18:15:50 2017 +0000
URL:        https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=4c47cfa2

libsandbox: whitelist renameat/symlinkat as symlink funcs

These funcs don't deref their path args, so flag them as such.

URL: https://bugs.gentoo.org/612202
Signed-off-by: Mike Frysinger <vapier <AT> gentoo.org>

 libsandbox/libsandbox.c |  4 +++-
 tests/renameat-2.sh     | 12 ++++++++++++
 tests/renameat-3.sh     | 11 +++++++++++
 tests/renameat.at       |  2 ++
 tests/symlinkat-2.sh    | 10 ++++++++++
 tests/symlinkat-3.sh    |  9 +++++++++
 tests/symlinkat.at      |  2 ++
 7 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/libsandbox/libsandbox.c b/libsandbox/libsandbox.c
index e809308..de48bd7 100644
--- a/libsandbox/libsandbox.c
+++ b/libsandbox/libsandbox.c
@@ -650,8 +650,10 @@ static bool symlink_func(int sb_nr, int flags, const char *abs_path)
 	      sb_nr == SB_NR_LCHOWN   ||
 	      sb_nr == SB_NR_REMOVE   ||
 	      sb_nr == SB_NR_RENAME   ||
+	      sb_nr == SB_NR_RENAMEAT ||
 	      sb_nr == SB_NR_RMDIR    ||
-	      sb_nr == SB_NR_SYMLINK))
+	      sb_nr == SB_NR_SYMLINK  ||
+	      sb_nr == SB_NR_SYMLINKAT))
 	{
 		/* These funcs sometimes operate on symlinks */
 		if (!((sb_nr == SB_NR_FCHOWNAT ||

diff --git a/tests/renameat-2.sh b/tests/renameat-2.sh
new file mode 100755
index 0000000..d0fbe8a
--- /dev/null
+++ b/tests/renameat-2.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+# make sure we can clobber symlinks #612202
+
+addwrite $PWD
+
+ln -s /asdf sym || exit 1
+touch file
+renameat-0 0 AT_FDCWD file AT_FDCWD sym || exit 1
+[ ! -e file ]
+[ ! -L sym ]
+[ -e sym ]
+test ! -s "${SANDBOX_LOG}"

diff --git a/tests/renameat-3.sh b/tests/renameat-3.sh
new file mode 100755
index 0000000..9ae5c9a
--- /dev/null
+++ b/tests/renameat-3.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+# make sure we reject bad renames #612202
+
+addwrite $PWD
+mkdir deny
+adddeny $PWD/deny
+
+touch file
+renameat-0 -1,EACCES AT_FDCWD file AT_FDCWD deny/file || exit 1
+[ -e file ]
+test -s "${SANDBOX_LOG}"

diff --git a/tests/renameat.at b/tests/renameat.at
index 081d7d2..eec4638 100644
--- a/tests/renameat.at
+++ b/tests/renameat.at
@@ -1 +1,3 @@
 SB_CHECK(1)
+SB_CHECK(2)
+SB_CHECK(3)

diff --git a/tests/symlinkat-2.sh b/tests/symlinkat-2.sh
new file mode 100755
index 0000000..168362e
--- /dev/null
+++ b/tests/symlinkat-2.sh
@@ -0,0 +1,10 @@
+#!/bin/sh
+# make sure we can clobber symlinks #612202
+
+addwrite $PWD
+
+symlinkat-0 0 /asdf AT_FDCWD ./sym || exit 1
+[ -L sym ]
+symlinkat-0 -1,EEXIST /asdf AT_FDCWD ./sym || exit 1
+[ -L sym ]
+test ! -s "${SANDBOX_LOG}"

diff --git a/tests/symlinkat-3.sh b/tests/symlinkat-3.sh
new file mode 100755
index 0000000..a01c750
--- /dev/null
+++ b/tests/symlinkat-3.sh
@@ -0,0 +1,9 @@
+#!/bin/sh
+# make sure we reject bad symlinks #612202
+
+addwrite $PWD
+mkdir deny
+adddeny $PWD/deny
+
+symlinkat-0 -1,EACCES ./ AT_FDCWD deny/sym || exit 1
+test -s "${SANDBOX_LOG}"

diff --git a/tests/symlinkat.at b/tests/symlinkat.at
index 081d7d2..eec4638 100644
--- a/tests/symlinkat.at
+++ b/tests/symlinkat.at
@@ -1 +1,3 @@
 SB_CHECK(1)
+SB_CHECK(2)
+SB_CHECK(3)

[gentoo-commits] proj/sandbox:master commit in: libsandbox/, tests/

[Michał Górny]

commit:     9ed52ff7daa39cdf4748f5b9c91358f421c8be7a
Author:     Michał Górny <mgorny <AT> gentoo <DOT> org>
AuthorDate: Mon Sep 25 18:42:03 2017 +0000
Commit:     Michał Górny <mgorny <AT> gentoo <DOT> org>
CommitDate: Tue Oct  3 16:38:51 2017 +0000
URL:        https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=9ed52ff7

libsandbox: Fix path matching not to dumbly match prefixes

Fix the path matching code to match prefixes component-wide rather than
literally. This means that a path such as '/foo' will no longer match
'/foobar' but only '/foo' and its subdirectories (if it is a directory).

 libsandbox/libsandbox.c | 22 +++++++++++++++++++---
 tests/script-14.sh      | 20 ++++++++++++++++++++
 tests/script.at         |  1 +
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/libsandbox/libsandbox.c b/libsandbox/libsandbox.c
index e164dcf..962690e 100644
--- a/libsandbox/libsandbox.c
+++ b/libsandbox/libsandbox.c
@@ -632,9 +632,25 @@ static int check_prefixes(char **prefixes, int num_prefixes, const char *path)
 		return 0;
 
 	size_t i;
-	for (i = 0; i < num_prefixes; ++i)
-		if (prefixes[i] && !strncmp(path, prefixes[i], strlen(prefixes[i])))
-			return 1;
+	for (i = 0; i < num_prefixes; ++i) {
+		if (unlikely(!prefixes[i]))
+			continue;
+
+		size_t prefix_len = strlen(prefixes[i]);
+		/* Start with a regular prefix match for speed */
+		if (strncmp(path, prefixes[i], prefix_len))
+			continue;
+
+		/* Now, if prefix did not end with a slash, we need to make sure
+		 * we are not matching in the middle of a filename. So check
+		 * whether the match is followed by a slash, or NUL.
+		 */
+		if (prefixes[i][prefix_len-1] != '/'
+				&& path[prefix_len] != '/' && path[prefix_len] != '\0')
+			continue;
+
+		return 1;
+	}
 
 	return 0;
 }

diff --git a/tests/script-14.sh b/tests/script-14.sh
new file mode 100644
index 0000000..6fa55a0
--- /dev/null
+++ b/tests/script-14.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+# check that paths don't accidentally match other files by prefix
+[ "${at_xfail}" = "yes" ] && exit 77 # see script-0
+
+(
+# This clobbers all existing writable paths for this one write.
+SANDBOX_PREDICT=/dev/null
+SANDBOX_WRITE="${PWD}/foo"
+echo FAIL >foobar
+)
+# the write to 'logfoobar' should be rejected since only 'log'
+# is supposed to be writable
+if [ $? -eq 0 ] ; then
+	exit 1
+fi
+
+# and we should have gotten a sandbox violation
+test -s "${SANDBOX_LOG}"
+
+exit $?

diff --git a/tests/script.at b/tests/script.at
index 58a5077..9134ac1 100644
--- a/tests/script.at
+++ b/tests/script.at
@@ -11,3 +11,4 @@ SB_CHECK(10)
 SB_CHECK(11)
 SB_CHECK(12)
 SB_CHECK(13)
+SB_CHECK(14)

[gentoo-commits] proj/sandbox:master commit in: libsandbox/, tests/

[Mike Frysinger]

commit:     e9946633344c34b295485130f8f0fceb10fedc96
Author:     Mike Frysinger <vapier <AT> gentoo <DOT> org>
AuthorDate: Thu Oct 21 09:51:08 2021 +0000
Commit:     Mike Frysinger <vapier <AT> gentoo <DOT> org>
CommitDate: Thu Oct 21 09:51:08 2021 +0000
URL:        https://gitweb.gentoo.org/proj/sandbox.git/commit/?id=e9946633

libsandbox: switch tracing from signal handler to waitpid

Since we can get all the details we need from the existing waitpid
call, there's no need for an async signal handler.  We can merge
that logic into the main synchronous loop.  This makes the code a
lot easier to reason about as we know it's fully contained here.

Signed-off-by: Mike Frysinger <vapier <AT> gentoo.org>

 libsandbox/trace.c | 126 ++++++++++++++++++++++++-----------------------------
 tests/trace-0      |   2 +-
 2 files changed, 59 insertions(+), 69 deletions(-)

diff --git a/libsandbox/trace.c b/libsandbox/trace.c
index fc700e3..8394b71 100644
--- a/libsandbox/trace.c
+++ b/libsandbox/trace.c
@@ -178,20 +178,6 @@ static char *do_peekstr(unsigned long lptr)
 	}
 }
 
-static const char *strcld_chld(int cld)
-{
-	switch (cld) {
-#define C(c) case CLD_##c: return "CLD_"#c;
-	C(CONTINUED)
-	C(DUMPED)
-	C(EXITED)
-	C(KILLED)
-	C(TRAPPED)
-	C(STOPPED)
-#undef C
-	default: return "CLD_???";
-	}
-}
 /* strsignal() translates the string when i want C define */
 static const char *strsig(int sig)
 {
@@ -214,47 +200,6 @@ static const char *strsig(int sig)
 	}
 }
 
-static void trace_child_signal(int signo, siginfo_t *info, void *context)
-{
-	sb_debug("got sig %s(%i): code:%s(%i) status:%s(%i)",
-		strsig(signo), signo,
-		strcld_chld(info->si_code), info->si_code,
-		strsig(info->si_status), info->si_status);
-
-	switch (info->si_code) {
-		case CLD_DUMPED:
-		case CLD_KILLED:
-			trace_exit(128 + info->si_status);
-
-		case CLD_EXITED:
-			__sb_debug(" = %i\n", info->si_status);
-			trace_exit(info->si_status);
-
-		case CLD_TRAPPED:
-			switch (info->si_status) {
-				case SIGSTOP:
-					kill(trace_pid, SIGCONT);
-				case SIGTRAP:
-				case SIGCONT:
-					return;
-			}
-
-			/* For whatever signal the child caught, let's ignore it and
-			 * continue on.  If it aborted, segfaulted, whatever, that's
-			 * its problem, not ours, so don't whine about it.  We just
-			 * have to be sure to bubble it back up.  #265072
-			 */
-			do_ptrace(PTRACE_CONT, NULL, (void *)(long)info->si_status);
-			return;
-	}
-
-	sb_eerror("ISE:trace_child_signal: child (%i) signal %s(%i), code %s(%i), status %s(%i)\n",
-		trace_pid,
-		strsig(signo), signo,
-		strcld_chld(info->si_code), info->si_code,
-		strsig(info->si_status), info->si_status);
-}
-
 static const struct syscall_entry *lookup_syscall_in_tbl(const struct syscall_entry *tbl, int nr)
 {
 	while (tbl->name)
@@ -441,8 +386,9 @@ static void trace_loop(void)
 {
 	trace_regs regs;
 	bool before_exec, before_syscall, fake_syscall_ret;
+	unsigned event;
 	long ret;
-	int nr, status;
+	int nr, status, sig;
 	const struct syscall_entry *se, *tbl_after_fork;
 
 	before_exec = true;
@@ -453,16 +399,63 @@ static void trace_loop(void)
 		ret = do_ptrace(PTRACE_SYSCALL, NULL, NULL);
 		waitpid(trace_pid, &status, 0);
 
-		if (before_exec) {
-			unsigned event = ((unsigned)status >> 16);
-			if (event == PTRACE_EVENT_EXEC) {
-				_sb_debug("hit exec!");
-				before_exec = false;
-			} else
+		event = (unsigned)status >> 16;
+
+		if (WIFSIGNALED(status)) {
+			int sig = WTERMSIG(status);
+			sb_debug("signaled %s %i", strsig(sig), sig);
+			kill(getpid(), sig);
+			trace_exit(128 + sig);
+
+		} else if (WIFEXITED(status)) {
+			ret = WEXITSTATUS(status);
+			sb_debug("exited %li", ret);
+			trace_exit(ret);
+
+		}
+
+		sb_assert(WIFSTOPPED(status));
+		sig = WSTOPSIG(status);
+
+		switch (event) {
+		case 0:
+			if (sig != (SIGTRAP | 0x80)) {
+				/* For whatever signal the child caught, let's ignore it and
+				 * continue on.  If it aborted, segfaulted, whatever, that's
+				 * its problem, not ours, so don't whine about it.  We just
+				 * have to be sure to bubble it back up.  #265072
+				 *
+				 * The next run of this loop should see the WIFSIGNALED status
+				 * and we'll exit then.
+				 */
+				sb_debug("passing signal through %s (%i)", strsig(sig), sig);
+				do_ptrace(PTRACE_CONT, NULL, (void *)(uintptr_t)(sig));
+				continue;
+			}
+
+			if (before_exec) {
 				_sb_debug("waiting for exec; status: %#x", status);
+				continue;
+			}
+			break;
+
+		case PTRACE_EVENT_EXEC:
+			__sb_debug("hit exec!");
+			before_exec = false;
 			ret = trace_get_regs(&regs);
 			tbl_after_fork = trace_check_personality(&regs);
 			continue;
+
+		case PTRACE_EVENT_EXIT:
+			/* We'll tell the process to resume, which should make it exit,
+			 * and then we'll pick up its exit status and exit above.
+			 */
+			__sb_debug(" exit event!\n");
+			continue;
+
+		default:
+			sb_ebort("ISE: unhandle ptrace signal %s (%i) event %u\n",
+			         strsig(sig), sig, event);
 		}
 
 		ret = trace_get_regs(&regs);
@@ -499,17 +492,14 @@ static void trace_loop(void)
 
 void trace_main(const char *filename, char *const argv[])
 {
-	struct sigaction sa, old_sa;
-
-	sa.sa_flags = SA_RESTART | SA_SIGINFO;
-	sa.sa_sigaction = trace_child_signal;
-	sigaction(SIGCHLD, &sa, &old_sa);
+	struct sigaction old_sa, sa = { .sa_handler = SIG_DFL, };
 
 	sb_debug_dyn("trace_main: tracing: %s\n", filename);
 
 	if (trace_pid)
 		sb_ebort("ISE: trace code assumes multiple threads are not forking\n");
 
+	sigaction(SIGCHLD, &sa, &old_sa);
 	trace_pid = fork();
 	if (unlikely(trace_pid == -1)) {
 		sb_ebort("ISE: vfork() failed: %s\n", strerror(errno));
@@ -517,7 +507,7 @@ void trace_main(const char *filename, char *const argv[])
 		sb_debug("parent waiting for child (pid=%i) to signal", trace_pid);
 		waitpid(trace_pid, NULL, 0);
 		do_ptrace(PTRACE_SETOPTIONS, NULL,
-			(void *)(PTRACE_O_TRACESYSGOOD | PTRACE_O_TRACEEXEC));
+			(void *)(PTRACE_O_TRACESYSGOOD | PTRACE_O_TRACEEXEC | PTRACE_O_TRACEEXIT));
 		sb_close_all_fds();
 		trace_loop();
 		sb_ebort("ISE: child should have quit, as should we\n");

diff --git a/tests/trace-0 b/tests/trace-0
index 5a91c7a..99f3037 100755
--- a/tests/trace-0
+++ b/tests/trace-0
@@ -1,6 +1,6 @@
 #!/bin/sh
 # make sure trace support exists
-if grep -q trace_child_signal "$abs_top_builddir"/libsandbox/.libs/libsandbox.so ; then
+if grep -q trace_loop "$abs_top_builddir"/libsandbox/.libs/libsandbox.so ; then
 	# see comment at top of script-0 -- same issue applies here because
 	# the ld.so isn't around to load the correct sandbox lib for us
 	exec script-0