From 65a26689cb04c977610a7a02ffeda0b66b0fa572 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Thu, 20 Jan 2022 19:58:40 -0800 Subject: [PATCH] Lock and check pathNodes during walk in MultiGetAttr. Earlier we were only locking the start directory's opMu while performing the entire walk in MultiGetAttr. This means that a compromised client could potentially delete and replace certain path components inside the start directory while the walk is going on. Depending on the Server's File implementation, this could lead to symlink based attacks. Furthermore, calling WalkGetAttr requires the server to provide read concurrency guarantee as documented while the DefaultMultiGetAttr implementation was not providing. PiperOrigin-RevId: 423218838 --- pkg/p9/client_file.go | 56 +++++++++++++++++++++++++--- pkg/p9/file.go | 53 ++------------------------ pkg/p9/handlers.go | 80 +++++++++++++++++++++++++++++++++++++--- runsc/fsgofer/fsgofer.go | 20 +++++++--- 4 files changed, 142 insertions(+), 67 deletions(-) diff --git a/pkg/p9/client_file.go b/pkg/p9/client_file.go index f84352a73..3382af540 100644 --- a/pkg/p9/client_file.go +++ b/pkg/p9/client_file.go @@ -15,6 +15,7 @@ package p9 import ( + "errors" "fmt" "io" "sync/atomic" @@ -126,15 +127,58 @@ func (c *clientFile) MultiGetAttr(names []string) ([]FullStat, error) { return nil, unix.EBADF } - if !versionSupportsTmultiGetAttr(c.client.version) { - return DefaultMultiGetAttr(c, names) + if versionSupportsTmultiGetAttr(c.client.version) { + rmultigetattr := Rmultigetattr{} + if err := c.client.sendRecv(&Tmultigetattr{FID: c.fid, Names: names}, &rmultigetattr); err != nil { + return nil, err + } + return rmultigetattr.Stats, nil } - rmultigetattr := Rmultigetattr{} - if err := c.client.sendRecv(&Tmultigetattr{FID: c.fid, Names: names}, &rmultigetattr); err != nil { - return nil, err + stats := make([]FullStat, 0, len(names)) + var start File = c + parent := start + closeParent := func() { + if parent != start { + _ = parent.Close() + } } - return rmultigetattr.Stats, nil + defer closeParent() + mask := AttrMaskAll() + for i, name := range names { + if len(name) == 0 && i == 0 { + qid, valid, attr, err := parent.GetAttr(mask) + if err != nil { + return nil, err + } + stats = append(stats, FullStat{ + QID: qid, + Valid: valid, + Attr: attr, + }) + continue + } + qids, child, valid, attr, err := parent.WalkGetAttr([]string{name}) + if err != nil { + if errors.Is(err, unix.ENOENT) { + return stats, nil + } + return nil, err + } + closeParent() + parent = child + stats = append(stats, FullStat{ + QID: qids[0], + Valid: valid, + Attr: attr, + }) + if attr.Mode.FileType() != ModeDirectory { + // Doesn't need to continue if entry is not a dir. Including symlinks + // that cannot be followed. + break + } + } + return stats, nil } // StatFS implements File.StatFS. diff --git a/pkg/p9/file.go b/pkg/p9/file.go index ce649f69e..6ebcd0cbe 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -15,8 +15,6 @@ package p9 import ( - "errors" - "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/fd" ) @@ -30,6 +28,10 @@ type AttacherOptions struct { // AllocateOnDeleted is set to true if it's safe to call File.Allocate for // deleted files. AllocateOnDeleted bool + + // MultiGetAttrSupported is set to true if it's safe to call + // File.MultiGetAttr with read concurrency guarantee only on start directory. + MultiGetAttrSupported bool } // NoServerOptions partially implements Attacher with empty AttacherOptions. @@ -360,50 +362,3 @@ func (*DisallowServerCalls) Renamed(File, string) { func (*DisallowServerCalls) ServerOptions() AttacherOptions { panic("ServerOptions should not be called on the client") } - -// DefaultMultiGetAttr implements File.MultiGetAttr() on top of File. -func DefaultMultiGetAttr(start File, names []string) ([]FullStat, error) { - stats := make([]FullStat, 0, len(names)) - parent := start - closeParent := func() { - if parent != start { - _ = parent.Close() - } - } - defer closeParent() - mask := AttrMaskAll() - for i, name := range names { - if len(name) == 0 && i == 0 { - qid, valid, attr, err := parent.GetAttr(mask) - if err != nil { - return nil, err - } - stats = append(stats, FullStat{ - QID: qid, - Valid: valid, - Attr: attr, - }) - continue - } - qids, child, valid, attr, err := parent.WalkGetAttr([]string{name}) - if err != nil { - if errors.Is(err, unix.ENOENT) { - return stats, nil - } - return nil, err - } - closeParent() - parent = child - stats = append(stats, FullStat{ - QID: qids[0], - Valid: valid, - Attr: attr, - }) - if attr.Mode.FileType() != ModeDirectory { - // Doesn't need to continue if entry is not a dir. Including symlinks - // that cannot be followed. - break - } - } - return stats, nil -} diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index 842b00855..4d331a950 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -15,6 +15,7 @@ package p9 import ( + errors2 "errors" "fmt" "io" "os" @@ -1537,12 +1538,79 @@ func (t *Tmultigetattr) handle(cs *connState) message { } defer ref.DecRef() - var stats []FullStat - if err := ref.safelyRead(func() (err error) { - stats, err = ref.file.MultiGetAttr(t.Names) - return err - }); err != nil { - return newErr(err) + if cs.server.options.MultiGetAttrSupported { + var stats []FullStat + if err := ref.safelyRead(func() (err error) { + stats, err = ref.file.MultiGetAttr(t.Names) + return err + }); err != nil { + return newErr(err) + } + return &Rmultigetattr{Stats: stats} } + + stats := make([]FullStat, 0, len(t.Names)) + mask := AttrMaskAll() + start := ref.file + startNode := ref.pathNode + parent := start + parentNode := startNode + closeParent := func() { + if parent != start { + _ = parent.Close() + } + } + defer closeParent() + + cs.server.renameMu.RLock() + defer cs.server.renameMu.RUnlock() + + for i, name := range t.Names { + if len(name) == 0 && i == 0 { + startNode.opMu.RLock() + qid, valid, attr, err := start.GetAttr(mask) + startNode.opMu.RUnlock() + if err != nil { + return newErr(err) + } + stats = append(stats, FullStat{ + QID: qid, + Valid: valid, + Attr: attr, + }) + continue + } + + parentNode.opMu.RLock() + if atomic.LoadUint32(&parentNode.deleted) != 0 { + parentNode.opMu.RUnlock() + break + } + qids, child, valid, attr, err := parent.WalkGetAttr([]string{name}) + if err != nil { + parentNode.opMu.RUnlock() + if errors2.Is(err, unix.ENOENT) { + break + } + return newErr(err) + } + stats = append(stats, FullStat{ + QID: qids[0], + Valid: valid, + Attr: attr, + }) + // Update with next generation. + closeParent() + parent = child + childNode := parentNode.pathNodeFor(name) + parentNode.opMu.RUnlock() + parentNode = childNode + if attr.Mode.FileType() != ModeDirectory { + // Doesn't need to continue if entry is not a dir. Including symlinks + // that cannot be followed. + break + } + } + return &Rmultigetattr{Stats: stats} } diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index eb86332cb..a317e0c7a 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -150,8 +150,9 @@ func (a *attachPoint) Attach() (p9.File, error) { // a file that was created in the same path as the delete file. func (a *attachPoint) ServerOptions() p9.AttacherOptions { return p9.AttacherOptions{ - SetAttrOnDeleted: true, - AllocateOnDeleted: true, + SetAttrOnDeleted: true, + AllocateOnDeleted: true, + MultiGetAttrSupported: true, } } @@ -869,7 +870,7 @@ func (*localFile) Rename(p9.File, string) error { panic("rename called directly") } -// RenameAt implements p9.File.RenameAt. +// RenameAt implements p9.File. func (l *localFile) RenameAt(oldName string, directory p9.File, newName string) error { if err := l.checkROMount(); err != nil { return err @@ -1133,7 +1134,7 @@ func (l *localFile) Flush() error { return nil } -// Bind implements p9.File.Bind. +// 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 @@ -1252,7 +1253,7 @@ func (l *localFile) Connect(flags p9.ConnectFlags) (*fd.FD, error) { return fd.New(f), nil } -// Close implements p9.File.Close. +// Close implements p9.File. func (l *localFile) Close() error { l.mode = invalidMode err := l.file.Close() @@ -1313,6 +1314,7 @@ func (l *localFile) checkROMount() error { return nil } +// MultiGetAttr implements p9.File. func (l *localFile) MultiGetAttr(names []string) ([]p9.FullStat, error) { stats := make([]p9.FullStat, 0, len(names)) @@ -1329,6 +1331,12 @@ func (l *localFile) MultiGetAttr(names []string) ([]p9.FullStat, error) { names = names[1:] } + // Note that while performing the walk below, we do not have read + // concurrency guarantee for any descendants. So files can be created/deleted + // while the walk is being performed. However, this should be fine from a + // security perspective as we are using host FDs to walk and checking that + // each opened path component is a directory. We also set O_NOFOLLOW to + // ensure no symlinks are accidentally followed during walk. parent := l.file.FD() closeParent := func() { if parent != l.file.FD() { @@ -1377,7 +1385,7 @@ type socketLocalFile struct { sock int } -// Close implements p9.File.Close. +// Close implements p9.File. func (l *socketLocalFile) Close() error { err := l.localFile.Close() err2 := unix.Close(l.sock)