diff --git a/pkg/sentry/kernel/sessions.go b/pkg/sentry/kernel/sessions.go index 36b602e74..1575afada 100644 --- a/pkg/sentry/kernel/sessions.go +++ b/pkg/sentry/kernel/sessions.go @@ -431,13 +431,9 @@ func (tg *ThreadGroup) CreateProcessGroup() error { // JoinProcessGroup joins an existing process group. // -// This function will return EACCES if an exec has been performed since fork -// by the given ThreadGroup, and EPERM if the Sessions are not the same or the +// This function will return EPERM if the Sessions are not the same or the // group does not exist. -// -// If checkExec is set, then the join is not permitted after the process has -// executed exec at least once. -func (tg *ThreadGroup) JoinProcessGroup(pidns *PIDNamespace, pgid ProcessGroupID, checkExec bool) error { +func (tg *ThreadGroup) JoinProcessGroup(pidns *PIDNamespace, pgid ProcessGroupID) error { pidns.owner.mu.Lock() defer pidns.owner.mu.Unlock() @@ -452,11 +448,6 @@ func (tg *ThreadGroup) JoinProcessGroup(pidns *PIDNamespace, pgid ProcessGroupID return linuxerr.EPERM } - // Disallow the join if an execve has performed, per POSIX. - if checkExec && tg.execed { - return linuxerr.EACCES - } - // See if it's in the same session as ours. if pg.session != tg.processGroup.session { return linuxerr.EPERM diff --git a/pkg/sentry/kernel/task_exec.go b/pkg/sentry/kernel/task_exec.go index 170df86d2..85a662810 100644 --- a/pkg/sentry/kernel/task_exec.go +++ b/pkg/sentry/kernel/task_exec.go @@ -205,9 +205,9 @@ func (r *runSyscallAfterExecStop) execute(t *Task) taskRunState { t.signalStack = linux.SignalStack{Flags: linux.SS_DISABLE} // "The termination signal is reset to SIGCHLD (see clone(2))." t.tg.terminationSignal = linux.SIGCHLD - // execed indicates that the process can no longer join a process group + // execed indicates that the process's pgid cannot be changed // in some scenarios (namely, the parent call setpgid(2) on the child). - // See the JoinProcessGroup function in sessions.go for more context. + // See the Setpgid function in sys_thread.go for more context. t.tg.execed = true // Maximum RSS is preserved across execve(2). t.updateRSSLocked() diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 26566de04..b274c5b9b 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -651,3 +651,11 @@ func (tg *ThreadGroup) IsInitIn(pidns *PIDNamespace) bool { func (tg *ThreadGroup) isInitInLocked(pidns *PIDNamespace) bool { return pidns.tgids[tg] == initTID } + +// Execed returns whether this ThreadGroup has execed since creation. +func (tg *ThreadGroup) Execed() bool { + ts := tg.TaskSet() + ts.mu.RLock() + defer ts.mu.RUnlock() + return tg.execed +} diff --git a/pkg/sentry/syscalls/linux/sys_thread.go b/pkg/sentry/syscalls/linux/sys_thread.go index a523b38b0..df61b96b3 100644 --- a/pkg/sentry/syscalls/linux/sys_thread.go +++ b/pkg/sentry/syscalls/linux/sys_thread.go @@ -620,6 +620,13 @@ func Setpgid(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr return 0, nil, linuxerr.EINVAL } + // Return EACCES if an attempt was made to change the process group ID of one + // of the children of the calling process and the child had + // already performed an execve(2) + if tg != t.ThreadGroup() && tg.Execed() { + return 0, nil, linuxerr.EACCES + } + // If the pgid is the same as the group, then create a new one. Otherwise, // we attempt to join an existing process group. if pgid == defaultPGID { @@ -635,7 +642,7 @@ func Setpgid(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintptr } } else { // Same as CreateProcessGroup, above. - if err := tg.JoinProcessGroup(t.PIDNamespace(), pgid, tg != t.ThreadGroup()); err != nil { + if err := tg.JoinProcessGroup(t.PIDNamespace(), pgid); err != nil { // See above. if t.PIDNamespace().IDOfProcessGroup(tg.ProcessGroup()) == pgid { return 0, nil, nil diff --git a/test/syscalls/linux/exec.cc b/test/syscalls/linux/exec.cc index c17d58282..e256ca901 100644 --- a/test/syscalls/linux/exec.cc +++ b/test/syscalls/linux/exec.cc @@ -17,18 +17,22 @@ #include #include #include +#include #include #include #include +#include +#include +#include +#include #include -#include #include #include +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/strings/match.h" -#include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" @@ -813,6 +817,40 @@ TEST(GetpriorityTest, ExecveMaintainsPriority) { {}, W_EXITCODE(expected_exit_code, 0), ""); } +// Test that setpgid() fails on child processes after they call execve(). +TEST(ExecTest, Setpgid) { + const pid_t pid = fork(); + int status; + ASSERT_NE(pid, -1); + if (pid == 0) { + ASSERT_THAT(ptrace(PTRACE_TRACEME, 0, 0, 0), SyscallSucceeds()); + raise(SIGSTOP); + char* argv[] = {nullptr}; + char* envp[] = {nullptr}; + ASSERT_THAT(execve("/proc/self/exe", argv, envp), SyscallSucceeds()); + } + + EXPECT_THAT(setpgid(pid, pid), SyscallSucceeds()) + << "setpgid failed before execve"; + ASSERT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid)) + << "waitpid failed"; + ASSERT_THAT(WIFSTOPPED(status), 1); + ASSERT_THAT(WSTOPSIG(status), SIGSTOP); + ASSERT_THAT( + ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_EXITKILL | PTRACE_O_TRACEEXEC), + SyscallSucceeds()) + << "ptrace failed"; + ASSERT_THAT(ptrace(PTRACE_CONT, pid, 0, 0), SyscallSucceeds()) + << "ptrace (PTRACE_CONT) failed"; + ASSERT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid)) + << "waitpid failed"; + ASSERT_THAT(WIFSTOPPED(status), 1); + ASSERT_THAT(WSTOPSIG(status), SIGTRAP); + EXPECT_THAT(setpgid(pid, pid), SyscallFailsWithErrno(EACCES)); + EXPECT_THAT(setpgid(pid, getpid()), SyscallFailsWithErrno(EACCES)); + EXPECT_THAT(setpgid(getpid(), pid), SyscallSucceeds()); +} + void ExecWithThread() { // Used to ensure that the thread has actually started. absl::Mutex mu;