From 0eb861df56016eba3758de7afed7181d52e611d0 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Mon, 24 Oct 2022 18:23:04 -0700 Subject: [PATCH] Replace `fsgofer-host-uds` flag The new `host-uds` flag has the following differences: * More granular control: user can specify whether to ignore host UDS, connect to an existing host UDS, create a host UDS, or both * Will shortly be used to also control pipes from the host, hence the removal of UDS from the name * Can be used with other filesystems, not gofer-based, hence the removal of fsgofer from the name This change also removes support for bind() from 9P based gofer since it's not supported and would make this change more complicated. Updates #8037 PiperOrigin-RevId: 483538832 --- pkg/lisafs/testsuite/testsuite.go | 2 +- runsc/cmd/gofer.go | 11 ++-- runsc/config/config.go | 95 +++++++++++++++++++++++++++++- runsc/config/config_test.go | 28 +++++++++ runsc/config/flags.go | 4 +- runsc/fsgofer/BUILD | 1 + runsc/fsgofer/filter/config.go | 16 +++-- runsc/fsgofer/filter/filter.go | 15 +++-- runsc/fsgofer/fsgofer.go | 97 +++---------------------------- runsc/fsgofer/fsgofer_test.go | 41 ------------- runsc/fsgofer/lisafs.go | 4 +- runsc/fsgofer/lisafs_test.go | 2 +- test/runner/main.go | 2 +- 13 files changed, 165 insertions(+), 153 deletions(-) diff --git a/pkg/lisafs/testsuite/testsuite.go b/pkg/lisafs/testsuite/testsuite.go index 2c8645ba9..bf46e9786 100644 --- a/pkg/lisafs/testsuite/testsuite.go +++ b/pkg/lisafs/testsuite/testsuite.go @@ -64,7 +64,7 @@ func RunAllLocalFSTests(t *testing.T, tester Tester) { // TestFunc describes the signature of a test method. type TestFunc func(context.Context, *testing.T, Tester, lisafs.ClientFD) -var localFSTests map[string]TestFunc = map[string]TestFunc{ +var localFSTests = map[string]TestFunc{ "Stat": testStat, "RegularFileIO": testRegularFileIO, "RegularFileOpen": testRegularFileOpen, diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go index a04d7a2ba..dbf9905f6 100644 --- a/runsc/cmd/gofer.go +++ b/runsc/cmd/gofer.go @@ -227,8 +227,9 @@ func (g *Gofer) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) // Initialize filters. opts := filter.Options{ - UDSEnabled: conf.FSGoferHostUDS, - ProfileEnabled: len(profileOpts) > 0, + UDSOpenEnabled: conf.GetHostUDS().AllowOpen(), + UDSCreateEnabled: conf.GetHostUDS().AllowCreate(), + ProfileEnabled: len(profileOpts) > 0, } if err := filter.Install(opts); err != nil { util.Fatalf("installing seccomp filters: %v", err) @@ -258,7 +259,7 @@ func (g *Gofer) serveLisafs(spec *specs.Spec, conf *config.Config, root string) server := fsgofer.NewLisafsServer(fsgofer.Config{ // These are global options. Ignore readonly configuration, that is set on // a per connection basis. - HostUDS: conf.FSGoferHostUDS, + HostUDS: conf.GetHostUDS(), }) // Start with root mount, then add any other additional mount as needed. @@ -318,7 +319,7 @@ func (g *Gofer) serve9P(spec *specs.Spec, conf *config.Config, root string) subc ats := make([]p9.Attacher, 0, len(spec.Mounts)+1) ap, err := fsgofer.NewAttachPoint("/", fsgofer.Config{ ROMount: spec.Root.Readonly || conf.Overlay, - HostUDS: conf.FSGoferHostUDS, + HostUDS: conf.GetHostUDS(), }) if err != nil { util.Fatalf("creating attach point: %v", err) @@ -331,7 +332,7 @@ func (g *Gofer) serve9P(spec *specs.Spec, conf *config.Config, root string) subc if specutils.IsGoferMount(m) { cfg := fsgofer.Config{ ROMount: isReadonlyMount(m.Options) || conf.Overlay, - HostUDS: conf.FSGoferHostUDS, + HostUDS: conf.GetHostUDS(), } ap, err := fsgofer.NewAttachPoint(m.Destination, cfg) if err != nil { diff --git a/runsc/config/config.go b/runsc/config/config.go index d12168a7d..cbbc98c84 100644 --- a/runsc/config/config.go +++ b/runsc/config/config.go @@ -76,10 +76,13 @@ type Config struct { // Overlay is whether to wrap the root filesystem in an overlay. Overlay bool `flag:"overlay"` - // FSGoferHostUDS enables the gofer to create and connect to host unix - // domain sockets. + // FSGoferHostUDS is deprecated: use host-uds=all. FSGoferHostUDS bool `flag:"fsgofer-host-uds"` + // HostUDS controls permission to access host Unix-domain sockets. + // DO NOT call it directly, use GetHostComm() instead. + HostUDS HostUDS `flag:"host-uds"` + // Network indicates what type of network to use. Network NetworkType `flag:"network"` @@ -283,9 +286,26 @@ func (c *Config) validate() error { if c.ProfileMutex != "" && !c.ProfileEnable { return fmt.Errorf("profile-mutex flag requires enabling profiling with profile flag") } + if c.FSGoferHostUDS && c.HostUDS != HostUDSNone { + // Deprecated flag was used together with flag that replaced it. + return fmt.Errorf("fsgofer-host-uds has been replaced with host-uds flag") + } return nil } +// GetHostUDS returns the FS gofer communication that is allowed, taking into +// consideration all flags what affect the result. +func (c *Config) GetHostUDS() HostUDS { + if c.FSGoferHostUDS { + if c.HostUDS != HostUDSNone { + panic(fmt.Sprintf("HostUDS cannot be set when --fsgofer-host-uds=true")) + } + // Using deprecated flag, honor it to avoid breaking users. + return HostUDSOpen + } + return c.HostUDS +} + // FileAccessType tells how the filesystem is accessed. type FileAccessType int @@ -442,3 +462,74 @@ func leakModePtr(v refs.LeakMode) *refs.LeakMode { func watchdogActionPtr(v watchdog.Action) *watchdog.Action { return &v } + +// HostUDS tells how much of the host UDS the file system has access to. +type HostUDS int + +const ( + // HostUDSNone doesn't allows UDS from the host to be manipulated. + HostUDSNone HostUDS = 0x0 + + // HostUDSOpen allows UDS from the host to be opened, e.g. connect(2). + HostUDSOpen HostUDS = 0x1 + + // HostUDSCreate allows UDS from the host to be created, e.g. bind(2). + HostUDSCreate HostUDS = 0x2 + + // HostUDSAll allows all form of communication with the host through UDS. + HostUDSAll = HostUDSOpen | HostUDSCreate +) + +func hostUDSPtr(v HostUDS) *HostUDS { + return &v +} + +// Set implements flag.Value. +func (g *HostUDS) Set(v string) error { + switch v { + case "", "none": + *g = HostUDSNone + case "open": + *g = HostUDSOpen + case "create": + *g = HostUDSCreate + case "all": + *g = HostUDSAll + default: + return fmt.Errorf("invalid host UDS type %q", v) + } + return nil +} + +// Get implements flag.Value. +func (g *HostUDS) Get() interface{} { + return *g +} + +// String implements flag.Value. +func (g HostUDS) String() string { + // Note: the order of operations is important given that HostUDS is a bitmap. + if g == HostUDSNone { + return "none" + } + if g == HostUDSAll { + return "all" + } + if g == HostUDSOpen { + return "open" + } + if g == HostUDSCreate { + return "create" + } + panic(fmt.Sprintf("Invalid host UDS type %d", g)) +} + +// AllowOpen returns true if it can consume UDS from the host. +func (g HostUDS) AllowOpen() bool { + return g&HostUDSOpen != 0 +} + +// AllowCreate returns true if it can create UDS in the host. +func (g HostUDS) AllowCreate() bool { + return g&HostUDSCreate != 0 +} diff --git a/runsc/config/config_test.go b/runsc/config/config_test.go index ad705479a..f67bb7592 100644 --- a/runsc/config/config_test.go +++ b/runsc/config/config_test.go @@ -137,6 +137,10 @@ func TestInvalidFlags(t *testing.T) { name: "ref-leak-mode", error: "invalid ref leak mode", }, + { + name: "host-uds", + error: "invalid host UDS", + }, } { t.Run(tc.name, func(t *testing.T) { testFlags := flag.NewFlagSet("test", flag.ContinueOnError) @@ -169,6 +173,30 @@ func TestValidationFail(t *testing.T) { }, error: "num_network_channels must be > 0", }, + { + name: "fsgofer-host-uds+comm:open", + flags: map[string]string{ + "fsgofer-host-uds": "true", + "host-uds": "open", + }, + error: "fsgofer-host-uds has been replaced with host-uds flag", + }, + { + name: "fsgofer-host-uds+comm:create", + flags: map[string]string{ + "fsgofer-host-uds": "true", + "host-uds": "create", + }, + error: "fsgofer-host-uds has been replaced with host-uds flag", + }, + { + name: "fsgofer-host-uds+comm:all", + flags: map[string]string{ + "fsgofer-host-uds": "true", + "host-uds": "all", + }, + error: "fsgofer-host-uds has been replaced with host-uds flag", + }, } { t.Run(tc.name, func(t *testing.T) { testFlags := flag.NewFlagSet("test", flag.ContinueOnError) diff --git a/runsc/config/flags.go b/runsc/config/flags.go index 2db0a5be8..f90f2b0f5 100644 --- a/runsc/config/flags.go +++ b/runsc/config/flags.go @@ -79,7 +79,9 @@ func RegisterFlags(flagSet *flag.FlagSet) { flagSet.Var(fileAccessTypePtr(FileAccessExclusive), "file-access", "specifies which filesystem validation to use for the root mount: exclusive (default), shared.") flagSet.Var(fileAccessTypePtr(FileAccessShared), "file-access-mounts", "specifies which filesystem validation to use for volumes other than the root mount: shared (default), exclusive.") flagSet.Bool("overlay", false, "wrap filesystem mounts with writable overlay. All modifications are stored in memory inside the sandbox.") - flagSet.Bool("fsgofer-host-uds", false, "allow the gofer to mount Unix Domain Sockets.") + flagSet.Bool("fsgofer-host-uds", false, "DEPRECATED: use host-uds=all") + flagSet.Var(hostUDSPtr(0), "host-uds", "controls permission to access host Unix-domain sockets. Values: none|open|create|all, default: none") + flagSet.Bool("vfs2", true, "DEPRECATED: this flag has no effect.") flagSet.Bool("fuse", false, "TEST ONLY; use while FUSE in VFSv2 is landing. This allows the use of the new experimental FUSE filesystem.") flagSet.Bool("lisafs", true, "Enables lisafs protocol instead of 9P.") diff --git a/runsc/fsgofer/BUILD b/runsc/fsgofer/BUILD index a7ac16bac..a02c201b1 100644 --- a/runsc/fsgofer/BUILD +++ b/runsc/fsgofer/BUILD @@ -23,6 +23,7 @@ go_library( "//pkg/p9", "//pkg/sync", "//pkg/syserr", + "//runsc/config", "@org_golang_x_sys//unix:go_default_library", ], ) diff --git a/runsc/fsgofer/filter/config.go b/runsc/fsgofer/filter/config.go index 9c91149da..9e3db2703 100644 --- a/runsc/fsgofer/filter/config.go +++ b/runsc/fsgofer/filter/config.go @@ -212,11 +212,7 @@ var allowedSyscalls = seccomp.SyscallRules{ unix.SYS_WRITE: {}, } -var udsSyscalls = seccomp.SyscallRules{ - unix.SYS_ACCEPT4: {}, - unix.SYS_BIND: {}, - unix.SYS_CONNECT: {}, - unix.SYS_LISTEN: {}, +var udsCommonSyscalls = seccomp.SyscallRules{ unix.SYS_SOCKET: []seccomp.Rule{ { seccomp.EqualTo(unix.AF_UNIX), @@ -236,6 +232,16 @@ var udsSyscalls = seccomp.SyscallRules{ }, } +var udsOpenSyscalls = seccomp.SyscallRules{ + unix.SYS_CONNECT: {}, +} + +var udsCreateSyscalls = seccomp.SyscallRules{ + unix.SYS_ACCEPT4: {}, + unix.SYS_BIND: {}, + unix.SYS_LISTEN: {}, +} + var xattrSyscalls = seccomp.SyscallRules{ unix.SYS_FGETXATTR: {}, unix.SYS_FSETXATTR: {}, diff --git a/runsc/fsgofer/filter/filter.go b/runsc/fsgofer/filter/filter.go index 8665c548b..9e03f9d91 100644 --- a/runsc/fsgofer/filter/filter.go +++ b/runsc/fsgofer/filter/filter.go @@ -24,8 +24,9 @@ import ( // Options are seccomp filter related options. type Options struct { - UDSEnabled bool - ProfileEnabled bool + UDSOpenEnabled bool + UDSCreateEnabled bool + ProfileEnabled bool } // Install installs seccomp filters. @@ -37,9 +38,15 @@ func Install(opt Options) error { s.Merge(profileFilters) } - if opt.UDSEnabled { + if opt.UDSOpenEnabled || opt.UDSCreateEnabled { report("host UDS enabled: syscall filters less restrictive!") - s.Merge(udsSyscalls) + s.Merge(udsCommonSyscalls) + if opt.UDSOpenEnabled { + s.Merge(udsOpenSyscalls) + } + if opt.UDSCreateEnabled { + s.Merge(udsCreateSyscalls) + } } // Set of additional filters used by -race and -msan. Returns empty diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index 4f37b60b3..ac06deb06 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -37,6 +37,7 @@ import ( "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/p9" "gvisor.dev/gvisor/pkg/sync" + "gvisor.dev/gvisor/runsc/config" ) const ( @@ -62,9 +63,8 @@ type Config struct { // PanicOnWrite panics on attempts to write to RO mounts. PanicOnWrite bool - // HostUDS signals whether the gofer can create and connect to host - // unix domain sockets. - HostUDS bool + // HostUDS signals whether the gofer can connect to host unix domain sockets. + HostUDS config.HostUDS } type attachPoint struct { @@ -315,13 +315,13 @@ func openAnyFile(pathDebug string, fn func(mode int) (*fd.FD, error)) (*fd.FD, b return nil, false, extractErrno(err) } -func checkSupportedFileType(mode uint32, permitSocket bool) error { +func checkSupportedFileType(mode uint32, hostComm config.HostUDS) error { switch mode & unix.S_IFMT { case unix.S_IFREG, unix.S_IFDIR, unix.S_IFLNK: return nil case unix.S_IFSOCK: - if !permitSocket { + if !hostComm.AllowOpen() { return unix.EPERM } return nil @@ -1110,76 +1110,12 @@ func (l *localFile) Flush() error { // Bind implements p9.File. func (l *localFile) Bind(sockType uint32, sockName string, uid p9.UID, gid p9.GID) (p9.File, p9.QID, p9.AttrMask, p9.Attr, error) { - if !l.attachPoint.conf.HostUDS { - // Bind on host UDS is not allowed. As per mknod(2), which is invoked as - // part of bind(2), if "the filesystem containing pathname does not support - // the type of node requested." then EPERM must be returned. - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, unix.EPERM - } - - // Create socket only for supported types. - if !isSockTypeSupported(sockType) { - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, unix.ENXIO - } - sock, err := unix.Socket(unix.AF_UNIX, int(sockType), 0) - if err != nil { - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, extractErrno(err) - } - - // Because there is no "bindat" syscall in Linux, we must create an - // absolute path to the socket we are creating. But go through /proc/self/fd - // to avoid host path walk of the entire host path. It also helps avoid the - // UNIX_PATH_MAX bytes limit on the path, in case path is too long. - sockPath := filepath.Join("/proc/self/fd", strconv.Itoa(l.file.FD()), sockName) - - // Revert operations on error paths. - didBind := false - cu := cleanup.Make(func() { - _ = unix.Close(sock) - if didBind { - if err := unix.Unlinkat(l.file.FD(), sockName, 0); err != nil { - log.Warningf("error unlinking file %q after failure: %v", sockPath, err) - } - } - }) - defer cu.Clean() - - // socket FD must be non blocking because RPC operations like Accept on this - // socket must be non blocking. - if err := unix.SetNonblock(sock, true); err != nil { - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, extractErrno(err) - } - - // Bind at the given path which should create the socket file. - if err := unix.Bind(sock, &unix.SockaddrUnix{Name: sockPath}); err != nil { - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, extractErrno(err) - } - didBind = true - - // Open socket to change ownership. - tempSockFD, err := fd.OpenAt(l.file, sockName, unix.O_PATH|openFlags, 0) - if err != nil { - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, extractErrno(err) - } - defer tempSockFD.Close() - - if _, err = setOwnerIfNeeded(tempSockFD.FD(), uid, gid); err != nil { - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, extractErrno(err) - } - - // Generate file for this socket by walking on it. - qid, sockF, valid, attr, err := l.WalkGetAttr([]string{sockName}) - if err != nil { - return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, err - } - - cu.Release() - return &socketLocalFile{localFile: sockF.(*localFile), sock: sock}, qid[0], valid, attr, nil + return nil, p9.QID{}, p9.AttrMask{}, p9.Attr{}, unix.EPERM } // Connect implements p9.File. func (l *localFile) Connect(socketType p9.SocketType) (*fd.FD, error) { - if !l.attachPoint.conf.HostUDS { + if !l.attachPoint.conf.HostUDS.AllowOpen() { return nil, unix.ECONNREFUSED } @@ -1332,22 +1268,3 @@ func (l *localFile) MultiGetAttr(names []string) ([]p9.FullStat, error) { } return stats, nil } - -// socketLocalFile is an extension of localFile which is only created via Bind -// and additionally implements Listen and Accept. It also tracks the lifecycle -// of the socket FD created by socket(2) in addition to the FD opened on the -// socket file itself. -type socketLocalFile struct { - *localFile - sock int -} - -// Close implements p9.File. -func (l *socketLocalFile) Close() error { - err := l.localFile.Close() - err2 := unix.Close(l.sock) - if err != nil { - return err - } - return err2 -} diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go index 9889807f0..63fea29a3 100644 --- a/runsc/fsgofer/fsgofer_test.go +++ b/runsc/fsgofer/fsgofer_test.go @@ -785,47 +785,6 @@ func TestReaddir(t *testing.T) { }) } -func TestUDS(t *testing.T) { - config := Config{ROMount: false, HostUDS: true} - dir, err := ioutil.TempDir("", "root-") - if err != nil { - t.Fatalf("ioutil.TempDir() failed, err: %v", err) - } - defer os.RemoveAll(dir) - - // First attach with writable configuration to setup tree. - a, err := NewAttachPoint(dir, config) - if err != nil { - t.Fatalf("NewAttachPoint failed: %v", err) - } - root, err := a.Attach() - if err != nil { - t.Fatalf("attach failed, err: %v", err) - } - defer root.Close() - - name := "sock" - uid := p9.UID(os.Getuid()) - gid := p9.GID(os.Getgid()) - sockF, _, valid, attr, err := root.Bind(unix.SOCK_STREAM, name, uid, gid) - if err != nil { - t.Fatalf("Bind failed: %v", err) - } - defer sockF.Close() - - if valid.Mode && !attr.Mode.IsSocket() { - t.Errorf("socket file mode is incorrect: want %d, got %d", p9.ModeSocket, attr.Mode) - } - if valid.UID && attr.UID != uid { - t.Errorf("socket file uid is incorrect: want %d, got %d", uid, attr.UID) - } - if valid.GID && attr.GID != gid { - t.Errorf("socket file gid is incorrect: want %d, got %d", gid, attr.GID) - } - // TODO(b/194709873): Once listen and accept are implemented, test connecting - // and accepting a connection using sockF. -} - // Test that attach point can be written to when it points to a file, e.g. // /etc/hosts. func TestAttachFile(t *testing.T) { diff --git a/runsc/fsgofer/lisafs.go b/runsc/fsgofer/lisafs.go index b3998816c..d16e5c4ac 100644 --- a/runsc/fsgofer/lisafs.go +++ b/runsc/fsgofer/lisafs.go @@ -673,7 +673,7 @@ func isSockTypeSupported(sockType uint32) bool { // Connect implements lisafs.ControlFDImpl.Connect. func (fd *controlFDLisa) Connect(sockType uint32) (int, error) { - if !fd.Conn().ServerImpl().(*LisafsServer).config.HostUDS { + if !fd.Conn().ServerImpl().(*LisafsServer).config.HostUDS.AllowOpen() { return -1, unix.EPERM } @@ -699,7 +699,7 @@ func (fd *controlFDLisa) Connect(sockType uint32) (int, error) { // BindAt implements lisafs.ControlFDImpl.BindAt. func (fd *controlFDLisa) BindAt(name string, sockType uint32, mode linux.FileMode, uid lisafs.UID, gid lisafs.GID) (*lisafs.ControlFD, linux.Statx, *lisafs.BoundSocketFD, int, error) { - if !fd.Conn().ServerImpl().(*LisafsServer).config.HostUDS { + if !fd.Conn().ServerImpl().(*LisafsServer).config.HostUDS.AllowCreate() { return nil, linux.Statx{}, nil, -1, unix.EPERM } diff --git a/runsc/fsgofer/lisafs_test.go b/runsc/fsgofer/lisafs_test.go index 77bba9691..1b0e3fa64 100644 --- a/runsc/fsgofer/lisafs_test.go +++ b/runsc/fsgofer/lisafs_test.go @@ -38,7 +38,7 @@ type tester struct{} // NewServer implements testsuite.Tester.NewServer. func (tester) NewServer(t *testing.T) *lisafs.Server { - return &fsgofer.NewLisafsServer(fsgofer.Config{HostUDS: true}).Server + return &fsgofer.NewLisafsServer(fsgofer.Config{}).Server } // LinkSupported implements testsuite.Tester.LinkSupported. diff --git a/test/runner/main.go b/test/runner/main.go index 953377777..45d0158cf 100644 --- a/test/runner/main.go +++ b/test/runner/main.go @@ -218,7 +218,7 @@ func runRunsc(tc *gtest.TestCase, spec *specs.Spec) error { args = append(args, "-strace") } if *addUDSTree { - args = append(args, "-fsgofer-host-uds") + args = append(args, "-host-uds=all") } if *leakCheck { args = append(args, "-ref-leak-mode=log-names")