From ae8d966f5af0bba9978a1aedac64038ef65a4cc9 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Thu, 11 Feb 2021 11:06:56 -0800 Subject: Assign controlling terminal when tty is opened and support NOCTTY PiperOrigin-RevId: 357015186 --- pkg/sentry/fs/tty/master.go | 4 +-- pkg/sentry/fs/tty/replica.go | 4 +-- pkg/sentry/fs/tty/terminal.go | 5 ++-- pkg/sentry/fsimpl/devpts/master.go | 5 ++-- pkg/sentry/fsimpl/devpts/replica.go | 11 +++++-- pkg/sentry/fsimpl/devpts/terminal.go | 6 ++-- pkg/sentry/fsimpl/kernfs/filesystem.go | 3 +- pkg/sentry/kernel/thread_group.go | 11 +++++-- test/syscalls/linux/BUILD | 1 + test/syscalls/linux/pty.cc | 54 ++++++++++++++++++++++++++++++++++ test/util/pty_util.cc | 7 ++++- test/util/pty_util.h | 8 ++++- 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 OpenReplica(const FileDescriptor& master) { + return OpenReplica(master, O_NONBLOCK | O_RDWR | O_NOCTTY); +} + +PosixErrorOr OpenReplica(const FileDescriptor& master, + int flags) { PosixErrorOr n = ReplicaID(master); if (!n.ok()) { return PosixErrorOr(n.error()); } - return Open(absl::StrCat("/dev/pts/", n.ValueOrDie()), O_RDWR | O_NONBLOCK); + return Open(absl::StrCat("/dev/pts/", n.ValueOrDie()), flags); } PosixErrorOr 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 OpenReplica(const FileDescriptor& master); +// Identical to the above OpenReplica, but flags are all specified by the +// caller. +PosixErrorOr OpenReplica(const FileDescriptor& master, + int flags); + // Get the number of the replica end of the master. PosixErrorOr ReplicaID(const FileDescriptor& master); -- cgit v1.2.3