Assign controlling terminal when tty is opened and support NOCTTY

PiperOrigin-RevId: 357015186

12 files changed, 101 insertions, 18 deletions

diff --git a/pkg/sentry/fs/tty/master.go b/pkg/sentry/fs/tty/master.go
index b91184b1b..1cf869b62 100644
--- a/pkg/sentry/fs/tty/master.go
+++ b/pkg/sentry/fs/tty/master.go
@@ -153,7 +153,7 @@ func (mf *masterFileOperations) Write(ctx context.Context, _ *fs.File, src userm
 }
 
 // Ioctl implements fs.FileOperations.Ioctl.
-func (mf *masterFileOperations) Ioctl(ctx context.Context, _ *fs.File, io usermem.IO, args arch.SyscallArguments) (uintptr, error) {
+func (mf *masterFileOperations) Ioctl(ctx context.Context, file *fs.File, io usermem.IO, args arch.SyscallArguments) (uintptr, error) {
 	t := kernel.TaskFromContext(ctx)
 	if t == nil {
 		// ioctl(2) may only be called from a task goroutine.
@@ -189,7 +189,7 @@ func (mf *masterFileOperations) Ioctl(ctx context.Context, _ *fs.File, io userme
 	case linux.TIOCSCTTY:
 		// Make the given terminal the controlling terminal of the
 		// calling process.
-		return 0, mf.t.setControllingTTY(ctx, args, true /* isMaster */)
+		return 0, mf.t.setControllingTTY(ctx, args, true /* isMaster */, file.Flags().Read)
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
 		return 0, mf.t.releaseControllingTTY(ctx, args, true /* isMaster */)
diff --git a/pkg/sentry/fs/tty/replica.go b/pkg/sentry/fs/tty/replica.go
index 385d230fb..0e3eea3bd 100644
--- a/pkg/sentry/fs/tty/replica.go
+++ b/pkg/sentry/fs/tty/replica.go
@@ -138,7 +138,7 @@ func (sf *replicaFileOperations) Write(ctx context.Context, _ *fs.File, src user
 }
 
 // Ioctl implements fs.FileOperations.Ioctl.
-func (sf *replicaFileOperations) Ioctl(ctx context.Context, _ *fs.File, io usermem.IO, args arch.SyscallArguments) (uintptr, error) {
+func (sf *replicaFileOperations) Ioctl(ctx context.Context, file *fs.File, io usermem.IO, args arch.SyscallArguments) (uintptr, error) {
 	t := kernel.TaskFromContext(ctx)
 	if t == nil {
 		// ioctl(2) may only be called from a task goroutine.
@@ -167,7 +167,7 @@ func (sf *replicaFileOperations) Ioctl(ctx context.Context, _ *fs.File, io userm
 	case linux.TIOCSCTTY:
 		// Make the given terminal the controlling terminal of the
 		// calling process.
-		return 0, sf.si.t.setControllingTTY(ctx, args, false /* isMaster */)
+		return 0, sf.si.t.setControllingTTY(ctx, args, false /* isMaster */, file.Flags().Read)
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
 		return 0, sf.si.t.releaseControllingTTY(ctx, args, false /* isMaster */)
diff --git a/pkg/sentry/fs/tty/terminal.go b/pkg/sentry/fs/tty/terminal.go
index 4f431d74d..d551267af 100644
--- a/pkg/sentry/fs/tty/terminal.go
+++ b/pkg/sentry/fs/tty/terminal.go
@@ -64,13 +64,14 @@ func newTerminal(ctx context.Context, d *dirInodeOperations, n uint32) *Terminal
 
 // setControllingTTY makes tm the controlling terminal of the calling thread
 // group.
-func (tm *Terminal) setControllingTTY(ctx context.Context, args arch.SyscallArguments, isMaster bool) error {
+func (tm *Terminal) setControllingTTY(ctx context.Context, args arch.SyscallArguments, isMaster bool, readable bool) error {
 	task := kernel.TaskFromContext(ctx)
 	if task == nil {
 		panic("setControllingTTY must be called from a task context")
 	}
 
-	return task.ThreadGroup().SetControllingTTY(tm.tty(isMaster), args[2].Int())
+	steal := args[2].Int() == 1
+	return task.ThreadGroup().SetControllingTTY(tm.tty(isMaster), steal, readable)
 }
 
 // releaseControllingTTY removes tm as the controlling terminal of the calling
diff --git a/pkg/sentry/fsimpl/devpts/master.go b/pkg/sentry/fsimpl/devpts/master.go
index b44117f40..93c031c89 100644
--- a/pkg/sentry/fsimpl/devpts/master.go
+++ b/pkg/sentry/fsimpl/devpts/master.go
@@ -164,10 +164,11 @@ func (mfd *masterFileDescription) Ioctl(ctx context.Context, io usermem.IO, args
 	case linux.TIOCSCTTY:
 		// Make the given terminal the controlling terminal of the
 		// calling process.
-		return 0, mfd.t.setControllingTTY(ctx, args, true /* isMaster */)
+		steal := args[2].Int() == 1
+		return 0, mfd.t.setControllingTTY(ctx, steal, true /* isMaster */, mfd.vfsfd.IsReadable())
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
-		return 0, mfd.t.releaseControllingTTY(ctx, args, true /* isMaster */)
+		return 0, mfd.t.releaseControllingTTY(ctx, true /* isMaster */)
 	case linux.TIOCGPGRP:
 		// Get the foreground process group.
 		return mfd.t.foregroundProcessGroup(ctx, args, true /* isMaster */)
diff --git a/pkg/sentry/fsimpl/devpts/replica.go b/pkg/sentry/fsimpl/devpts/replica.go
index a0c5b5af5..96d2054cb 100644
--- a/pkg/sentry/fsimpl/devpts/replica.go
+++ b/pkg/sentry/fsimpl/devpts/replica.go
@@ -58,6 +58,12 @@ func (ri *replicaInode) Open(ctx context.Context, rp *vfs.ResolvingPath, d *kern
 	if err := fd.vfsfd.Init(fd, opts.Flags, rp.Mount(), d.VFSDentry(), &vfs.FileDescriptionOptions{}); err != nil {
 		return nil, err
 	}
+	if opts.Flags&linux.O_NOCTTY == 0 {
+		// Opening a replica sets the process' controlling TTY when
+		// possible. An error indicates it cannot be set, and is
+		// ignored silently.
+		_ = fd.inode.t.setControllingTTY(ctx, false /* steal */, false /* isMaster */, fd.vfsfd.IsReadable())
+	}
 	return &fd.vfsfd, nil
 
 }
@@ -160,10 +166,11 @@ func (rfd *replicaFileDescription) Ioctl(ctx context.Context, io usermem.IO, arg
 	case linux.TIOCSCTTY:
 		// Make the given terminal the controlling terminal of the
 		// calling process.
-		return 0, rfd.inode.t.setControllingTTY(ctx, args, false /* isMaster */)
+		steal := args[2].Int() == 1
+		return 0, rfd.inode.t.setControllingTTY(ctx, steal, false /* isMaster */, rfd.vfsfd.IsReadable())
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
-		return 0, rfd.inode.t.releaseControllingTTY(ctx, args, false /* isMaster */)
+		return 0, rfd.inode.t.releaseControllingTTY(ctx, false /* isMaster */)
 	case linux.TIOCGPGRP:
 		// Get the foreground process group.
 		return rfd.inode.t.foregroundProcessGroup(ctx, args, false /* isMaster */)
diff --git a/pkg/sentry/fsimpl/devpts/terminal.go b/pkg/sentry/fsimpl/devpts/terminal.go
index 510bd6d89..d9e0164a6 100644
--- a/pkg/sentry/fsimpl/devpts/terminal.go
+++ b/pkg/sentry/fsimpl/devpts/terminal.go
@@ -54,18 +54,18 @@ func newTerminal(n uint32) *Terminal {
 
 // setControllingTTY makes tm the controlling terminal of the calling thread
 // group.
-func (tm *Terminal) setControllingTTY(ctx context.Context, args arch.SyscallArguments, isMaster bool) error {
+func (tm *Terminal) setControllingTTY(ctx context.Context, steal bool, isMaster, isReadable bool) error {
 	task := kernel.TaskFromContext(ctx)
 	if task == nil {
 		panic("setControllingTTY must be called from a task context")
 	}
 
-	return task.ThreadGroup().SetControllingTTY(tm.tty(isMaster), args[2].Int())
+	return task.ThreadGroup().SetControllingTTY(tm.tty(isMaster), steal, isReadable)
 }
 
 // releaseControllingTTY removes tm as the controlling terminal of the calling
 // thread group.
-func (tm *Terminal) releaseControllingTTY(ctx context.Context, args arch.SyscallArguments, isMaster bool) error {
+func (tm *Terminal) releaseControllingTTY(ctx context.Context, isMaster bool) error {
 	task := kernel.TaskFromContext(ctx)
 	if task == nil {
 		panic("releaseControllingTTY must be called from a task context")
diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go
index d6dd6bc41..beb9302f6 100644
--- a/pkg/sentry/fsimpl/kernfs/filesystem.go
+++ b/pkg/sentry/fsimpl/kernfs/filesystem.go
@@ -464,7 +464,8 @@ func (fs *Filesystem) OpenAt(ctx context.Context, rp *vfs.ResolvingPath, opts vf
 	// O_NOFOLLOW have no effect here (they're handled by VFS by setting
 	// appropriate bits in rp), but are returned by
 	// FileDescriptionImpl.StatusFlags().
-	opts.Flags &= linux.O_ACCMODE | linux.O_CREAT | linux.O_EXCL | linux.O_TRUNC | linux.O_DIRECTORY | linux.O_NOFOLLOW | linux.O_NONBLOCK
+	opts.Flags &= linux.O_ACCMODE | linux.O_CREAT | linux.O_EXCL | linux.O_TRUNC |
+		linux.O_DIRECTORY | linux.O_NOFOLLOW | linux.O_NONBLOCK | linux.O_NOCTTY
 	ats := vfs.AccessTypesForOpenFlags(&opts)
 
 	// Do not create new file.
diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go
index a183b28c1..b92e98fa1 100644
--- a/pkg/sentry/kernel/thread_group.go
+++ b/pkg/sentry/kernel/thread_group.go
@@ -344,7 +344,7 @@ func (tg *ThreadGroup) forEachChildThreadGroupLocked(fn func(*ThreadGroup)) {
 }
 
 // SetControllingTTY sets tty as the controlling terminal of tg.
-func (tg *ThreadGroup) SetControllingTTY(tty *TTY, arg int32) error {
+func (tg *ThreadGroup) SetControllingTTY(tty *TTY, steal bool, isReadable bool) error {
 	tty.mu.Lock()
 	defer tty.mu.Unlock()
 
@@ -361,6 +361,9 @@ func (tg *ThreadGroup) SetControllingTTY(tty *TTY, arg int32) error {
 		return syserror.EINVAL
 	}
 
+	creds := auth.CredentialsFromContext(tg.leader)
+	hasAdmin := creds.HasCapabilityIn(linux.CAP_SYS_ADMIN, creds.UserNamespace.Root())
+
 	// "If this terminal is already the controlling terminal of a different
 	// session group, then the ioctl fails with EPERM, unless the caller
 	// has the CAP_SYS_ADMIN capability and arg equals 1, in which case the
@@ -368,7 +371,7 @@ func (tg *ThreadGroup) SetControllingTTY(tty *TTY, arg int32) error {
 	// terminal lose it." - tty_ioctl(4)
 	if tty.tg != nil && tg.processGroup.session != tty.tg.processGroup.session {
 		// Stealing requires CAP_SYS_ADMIN in the root user namespace.
-		if creds := auth.CredentialsFromContext(tg.leader); !creds.HasCapabilityIn(linux.CAP_SYS_ADMIN, creds.UserNamespace.Root()) || arg != 1 {
+		if !hasAdmin || !steal {
 			return syserror.EPERM
 		}
 		// Steal the TTY away. Unlike TIOCNOTTY, don't send signals.
@@ -388,6 +391,10 @@ func (tg *ThreadGroup) SetControllingTTY(tty *TTY, arg int32) error {
 		}
 	}
 
+	if !isReadable && !hasAdmin {
+		return syserror.EPERM
+	}
+
 	// Set the controlling terminal and foreground process group.
 	tg.tty = tty
 	tg.processGroup.session.foreground = tg.processGroup
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index 42fc363a2..31dc2525c 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -1441,6 +1441,7 @@ cc_binary(
         "@com_google_absl//absl/synchronization",
         "@com_google_absl//absl/time",
         gtest,
+        "//test/util:cleanup",
         "//test/util:posix_error",
         "//test/util:pty_util",
         "//test/util:test_main",
diff --git a/test/syscalls/linux/pty.cc b/test/syscalls/linux/pty.cc
index 85ff258df..e6b12f81c 100644
--- a/test/syscalls/linux/pty.cc
+++ b/test/syscalls/linux/pty.cc
@@ -36,6 +36,7 @@
 #include "absl/time/clock.h"
 #include "absl/time/time.h"
 #include "test/util/capability_util.h"
+#include "test/util/cleanup.h"
 #include "test/util/file_descriptor.h"
 #include "test/util/posix_error.h"
 #include "test/util/pty_util.h"
@@ -459,6 +460,59 @@ TEST(BasicPtyTest, OpenMasterReplica) {
   FileDescriptor replica = ASSERT_NO_ERRNO_AND_VALUE(OpenReplica(master));
 }
 
+TEST(BasicPtyTest, OpenSetsControllingTTY) {
+  SKIP_IF(IsRunningWithVFS1());
+  // setsid either puts us in a new session or fails because we're already the
+  // session leader. Either way, this ensures we're the session leader.
+  setsid();
+
+  // Make sure we're ignoring SIGHUP, which will be sent to this process once we
+  // disconnect they TTY.
+  struct sigaction sa = {};
+  sa.sa_handler = SIG_IGN;
+  sa.sa_flags = 0;
+  sigemptyset(&sa.sa_mask);
+  struct sigaction old_sa;
+  ASSERT_THAT(sigaction(SIGHUP, &sa, &old_sa), SyscallSucceeds());
+  auto cleanup = Cleanup([old_sa] {
+    EXPECT_THAT(sigaction(SIGHUP, &old_sa, NULL), SyscallSucceeds());
+  });
+
+  FileDescriptor master = ASSERT_NO_ERRNO_AND_VALUE(Open("/dev/ptmx", O_RDWR));
+  FileDescriptor replica =
+      ASSERT_NO_ERRNO_AND_VALUE(OpenReplica(master, O_NONBLOCK | O_RDWR));
+
+  // Opening replica should make it our controlling TTY, and therefore we are
+  // able to give it up.
+  ASSERT_THAT(ioctl(replica.get(), TIOCNOTTY), SyscallSucceeds());
+}
+
+TEST(BasicPtyTest, OpenMasterDoesNotSetsControllingTTY) {
+  SKIP_IF(IsRunningWithVFS1());
+  // setsid either puts us in a new session or fails because we're already the
+  // session leader. Either way, this ensures we're the session leader.
+  setsid();
+  FileDescriptor master = ASSERT_NO_ERRNO_AND_VALUE(Open("/dev/ptmx", O_RDWR));
+
+  // Opening master does not set the controlling TTY, and therefore we are
+  // unable to give it up.
+  ASSERT_THAT(ioctl(master.get(), TIOCNOTTY), SyscallFailsWithErrno(ENOTTY));
+}
+
+TEST(BasicPtyTest, OpenNOCTTY) {
+  SKIP_IF(IsRunningWithVFS1());
+  // setsid either puts us in a new session or fails because we're already the
+  // session leader. Either way, this ensures we're the session leader.
+  setsid();
+  FileDescriptor master = ASSERT_NO_ERRNO_AND_VALUE(Open("/dev/ptmx", O_RDWR));
+  FileDescriptor replica = ASSERT_NO_ERRNO_AND_VALUE(
+      OpenReplica(master, O_NOCTTY | O_NONBLOCK | O_RDWR));
+
+  // Opening replica with O_NOCTTY won't make it our controlling TTY, and
+  // therefore we are unable to give it up.
+  ASSERT_THAT(ioctl(replica.get(), TIOCNOTTY), SyscallFailsWithErrno(ENOTTY));
+}
+
 // The replica entry in /dev/pts/ disappears when the master is closed, even if
 // the replica is still open.
 TEST(BasicPtyTest, ReplicaEntryGoneAfterMasterClose) {
diff --git a/test/util/pty_util.cc b/test/util/pty_util.cc
index 2cf0bea74..351f4730c 100644
--- a/test/util/pty_util.cc
+++ b/test/util/pty_util.cc
@@ -24,11 +24,16 @@ namespace gvisor {
 namespace testing {
 
 PosixErrorOr<FileDescriptor> OpenReplica(const FileDescriptor& master) {
+  return OpenReplica(master, O_NONBLOCK | O_RDWR | O_NOCTTY);
+}
+
+PosixErrorOr<FileDescriptor> OpenReplica(const FileDescriptor& master,
+                                         int flags) {
   PosixErrorOr<int> n = ReplicaID(master);
   if (!n.ok()) {
     return PosixErrorOr<FileDescriptor>(n.error());
   }
-  return Open(absl::StrCat("/dev/pts/", n.ValueOrDie()), O_RDWR | O_NONBLOCK);
+  return Open(absl::StrCat("/dev/pts/", n.ValueOrDie()), flags);
 }
 
 PosixErrorOr<int> ReplicaID(const FileDescriptor& master) {
diff --git a/test/util/pty_util.h b/test/util/pty_util.h
index ed7658868..0cca2182c 100644
--- a/test/util/pty_util.h
+++ b/test/util/pty_util.h
@@ -21,9 +21,15 @@
 namespace gvisor {
 namespace testing {
 
-// Opens the replica end of the passed master as R/W and nonblocking.
+// Opens the replica end of the passed master as R/W and nonblocking. It does
+// not set the replica as the controlling TTY.
 PosixErrorOr<FileDescriptor> OpenReplica(const FileDescriptor& master);
 
+// Identical to the above OpenReplica, but flags are all specified by the
+// caller.
+PosixErrorOr<FileDescriptor> OpenReplica(const FileDescriptor& master,
+                                         int flags);
+
 // Get the number of the replica end of the master.
 PosixErrorOr<int> ReplicaID(const FileDescriptor& master);