Add additional sanity checks for walk.

PiperOrigin-RevId: 212058684
Change-Id: I319709b9ffcfccb3231bac98df345d2a20eca24b

pkg/p9/file.go

@@ -20,10 +20,13 @@ import (
 	"gvisor.googlesource.com/gvisor/pkg/fd"
 )
 
-// Attacher is provided by the user.
+// Attacher is provided by the server.
 type Attacher interface {
 	// Attach returns a new File.
-	Attach(attachName string) (File, error)
+	//
+	// The client-side attach will be translate to a series of walks from
+	// the file returned by this Attach call.
+	Attach() (File, error)
 }
 
 // File is a set of operations corresponding to a single node.

pkg/p9/handlers.go

@@ -17,6 +17,7 @@ package p9
 import (
 	"io"
 	"os"
+	"path"
 	"strings"
 	"sync/atomic"
 	"syscall"
@@ -93,39 +94,6 @@ func isSafeName(name string) bool {
 	return name != "" && !strings.Contains(name, "/") && name != "." && name != ".."
 }
 
-// handle implements handler.handle.
-func (t *Twalk) handle(cs *connState) message {
-	// Check the names.
-	for _, name := range t.Names {
-		if !isSafeName(name) {
-			return newErr(syscall.EINVAL)
-		}
-	}
-
-	// Lookup the FID.
-	ref, ok := cs.LookupFID(t.FID)
-	if !ok {
-		return newErr(syscall.EBADF)
-	}
-	defer ref.DecRef()
-
-	// Has it been opened already?
-	if _, opened := ref.OpenFlags(); opened {
-		return newErr(syscall.EBUSY)
-	}
-
-	// Do the walk.
-	qids, sf, err := ref.file.Walk(t.Names)
-	if err != nil {
-		return newErr(err)
-	}
-
-	// Install the new FID.
-	cs.InsertFID(t.NewFID, &fidRef{file: sf})
-
-	return &Rwalk{QIDs: qids}
-}
-
 // handle implements handler.handle.
 func (t *Tclunk) handle(cs *connState) message {
 	if !cs.DeleteFID(t.FID) {
@@ -175,14 +143,57 @@ func (t *Tattach) handle(cs *connState) message {
 		return newErr(syscall.EINVAL)
 	}
 
-	// Do the attach.
-	sf, err := cs.server.attacher.Attach(t.Auth.AttachName)
+	// Must provide an absolute path.
+	if path.IsAbs(t.Auth.AttachName) {
+		// Trim off the leading / if the path is absolute. We always
+		// treat attach paths as absolute and call attach with the root
+		// argument on the server file for clarity.
+		t.Auth.AttachName = t.Auth.AttachName[1:]
+	}
+
+	// Do the attach on the root.
+	sf, err := cs.server.attacher.Attach()
 	if err != nil {
 		return newErr(err)
 	}
-	cs.InsertFID(t.FID, &fidRef{file: sf})
+	_, valid, attr, err := sf.GetAttr(AttrMaskAll())
+	if err != nil {
+		sf.Close() // Drop file.
+		return newErr(err)
+	}
+	if !valid.Mode {
+		sf.Close() // Drop file.
+		return newErr(syscall.EINVAL)
+	}
+
+	// Build a transient reference.
+	root := &fidRef{
+		file:     sf,
+		refs:     1,
+		walkable: attr.Mode.IsDir(),
+	}
+	defer root.DecRef()
+
+	// Attach the root?
+	if len(t.Auth.AttachName) == 0 {
+		cs.InsertFID(t.FID, root)
+		return &Rattach{}
+	}
+
+	// We want the same traversal checks to apply on attach, so always
+	// attach at the root and use the regular walk paths.
+	names := strings.Split(t.Auth.AttachName, "/")
+	_, target, _, attr, err := doWalk(cs, root, names)
+	if err != nil {
+		return newErr(err)
+	}
+
+	// Insert the FID.
+	cs.InsertFID(t.FID, &fidRef{
+		file:     target,
+		walkable: attr.Mode.IsDir(),
+	})
 
-	// Return an empty QID.
 	return &Rattach{}
 }
 
@@ -678,15 +689,104 @@ func (t *Tflushf) handle(cs *connState) message {
 	return &Rflushf{}
 }
 
-// handle implements handler.handle.
-func (t *Twalkgetattr) handle(cs *connState) message {
+// walkOne walks zero or one path elements.
+//
+// The slice passed as qids is append and returned.
+func walkOne(qids []QID, from File, names []string) ([]QID, File, AttrMask, Attr, error) {
+	if len(names) > 1 {
+		// We require exactly zero or one elements.
+		return nil, nil, AttrMask{}, Attr{}, syscall.EINVAL
+	}
+	var localQIDs []QID
+	localQIDs, sf, valid, attr, err := from.WalkGetAttr(names)
+	if err == syscall.ENOSYS {
+		localQIDs, sf, err = from.Walk(names)
+		if err != nil {
+			// No way to walk this element.
+			return nil, nil, AttrMask{}, Attr{}, err
+		}
+		// Need a manual getattr.
+		_, valid, attr, err = sf.GetAttr(AttrMaskAll())
+		if err != nil {
+			// Don't leak the file.
+			sf.Close()
+		}
+	}
+	if err != nil {
+		// Error walking, don't return anything.
+		return nil, nil, AttrMask{}, Attr{}, err
+	}
+	if len(localQIDs) != 1 {
+		// Expected a single QID.
+		sf.Close()
+		return nil, nil, AttrMask{}, Attr{}, syscall.EINVAL
+	}
+	return append(qids, localQIDs...), sf, valid, attr, nil
+}
+
+// doWalk walks from a given fidRef.
+//
+// This enforces that all intermediate nodes are walkable (directories).
+func doWalk(cs *connState, ref *fidRef, names []string) (qids []QID, sf File, valid AttrMask, attr Attr, err error) {
 	// Check the names.
-	for _, name := range t.Names {
+	for _, name := range names {
 		if !isSafeName(name) {
-			return newErr(syscall.EINVAL)
+			err = syscall.EINVAL
+			return
 		}
 	}
 
+	// Has it been opened already?
+	if _, opened := ref.OpenFlags(); opened {
+		err = syscall.EBUSY
+		return
+	}
+
+	// Is this an empty list? Handle specially. We don't actually need to
+	// validate anything since this is always permitted.
+	if len(names) == 0 {
+		return walkOne(nil, ref.file, nil)
+	}
+
+	// Is it walkable?
+	if !ref.walkable {
+		err = syscall.EINVAL
+		return
+	}
+
+	from := ref.file // Start at the passed ref.
+
+	// Do the walk, one element at a time.
+	for i := 0; i < len(names); i++ {
+		qids, sf, valid, attr, err = walkOne(qids, from, names[i:i+1])
+
+		// Close the intermediate file. Note that we don't close the
+		// first file because in that case we are walking from the
+		// existing reference.
+		if i > 0 {
+			from.Close()
+		}
+		from = sf // Use the new file.
+
+		// Was there an error walking?
+		if err != nil {
+			return nil, nil, AttrMask{}, Attr{}, err
+		}
+
+		// We won't allow beyond past symlinks; stop here if this isn't
+		// a proper directory and we have additional paths to walk.
+		if !valid.Mode || (!attr.Mode.IsDir() && i < len(names)-1) {
+			from.Close() // Not using the file object.
+			return nil, nil, AttrMask{}, Attr{}, syscall.EINVAL
+		}
+	}
+
+	// Success.
+	return qids, sf, valid, attr, nil
+}
+
+// handle implements handler.handle.
+func (t *Twalk) handle(cs *connState) message {
 	// Lookup the FID.
 	ref, ok := cs.LookupFID(t.FID)
 	if !ok {
@@ -694,26 +794,41 @@ func (t *Twalkgetattr) handle(cs *connState) message {
 	}
 	defer ref.DecRef()
 
-	// Has it been opened already?
-	if _, opened := ref.OpenFlags(); opened {
-		return newErr(syscall.EBUSY)
-	}
-
 	// Do the walk.
-	qids, sf, valid, attr, err := ref.file.WalkGetAttr(t.Names)
-	if err == syscall.ENOSYS {
-		qids, sf, err = ref.file.Walk(t.Names)
-		if err != nil {
-			return newErr(err)
-		}
-		_, valid, attr, err = sf.GetAttr(AttrMaskAll())
-	}
+	qids, sf, _, attr, err := doWalk(cs, ref, t.Names)
 	if err != nil {
 		return newErr(err)
 	}
 
 	// Install the new FID.
-	cs.InsertFID(t.NewFID, &fidRef{file: sf})
+	cs.InsertFID(t.NewFID, &fidRef{
+		file:     sf,
+		walkable: attr.Mode.IsDir(),
+	})
+
+	return &Rwalk{QIDs: qids}
+}
+
+// handle implements handler.handle.
+func (t *Twalkgetattr) handle(cs *connState) message {
+	// Lookup the FID.
+	ref, ok := cs.LookupFID(t.FID)
+	if !ok {
+		return newErr(syscall.EBADF)
+	}
+	defer ref.DecRef()
+
+	// Do the walk.
+	qids, sf, valid, attr, err := doWalk(cs, ref, t.Names)
+	if err != nil {
+		return newErr(err)
+	}
+
+	// Install the new FID.
+	cs.InsertFID(t.NewFID, &fidRef{
+		file:     sf,
+		walkable: attr.Mode.IsDir(),
+	})
 
 	return &Rwalkgetattr{QIDs: qids, Valid: valid, Attr: attr}
 }

pkg/p9/local_server/local_server.go

@@ -70,8 +70,8 @@ func (l *local) info() (p9.QID, os.FileInfo, error) {
 }
 
 // Attach implements p9.Attacher.Attach.
-func (l *local) Attach(name string) (p9.File, error) {
-	return &local{path: path.Clean(name)}, nil
+func (l *local) Attach() (p9.File, error) {
+	return &local{path: "/"}, nil
 }
 
 // Walk implements p9.File.Walk.

pkg/p9/p9test/client_test.go

@@ -50,8 +50,16 @@ func TestDonateFD(t *testing.T) {
 
 	// Craft attacher to attach to the mocked file which will return our
 	// temporary file.
-	fileMock := &FileMock{OpenMock: OpenMock{File: f}}
-	attacher := &AttachMock{File: fileMock}
+	fileMock := &FileMock{
+		OpenMock: OpenMock{File: f},
+		GetAttrMock: GetAttrMock{
+			// The mode must be valid always.
+			Valid: p9.AttrMask{Mode: true},
+		},
+	}
+	attacher := &AttachMock{
+		File: fileMock,
+	}
 
 	// Make socket pair.
 	serverSocket, clientSocket, err := unet.SocketPair(false)
@@ -139,15 +147,14 @@ func TestClient(t *testing.T) {
 				a.Called = false
 				a.File = sf
 				a.Err = nil
+				// The attached root must have a valid mode.
+				sf.GetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory}
+				sf.GetAttrMock.Valid = p9.AttrMask{Mode: true}
 				var err error
-				sfFile, err = c.Attach("foo")
+				sfFile, err = c.Attach("")
 				if !a.Called {
 					t.Errorf("Attach never Called?")
 				}
-				if a.AttachName != "foo" {
-					// This wasn't carried through?
-					t.Errorf("attachName got %v wanted foo", a.AttachName)
-				}
 				return err
 			},
 		},
@@ -155,6 +162,8 @@ func TestClient(t *testing.T) {
 			name: "bad-walk",
 			want: sentinelErr,
 			fn: func(c *p9.Client) error {
+				// Walk only called when WalkGetAttr not available.
+				sf.WalkGetAttrMock.Err = syscall.ENOSYS
 				sf.WalkMock.File = d
 				sf.WalkMock.Err = sentinelErr
 				_, _, err := sfFile.Walk([]string{"foo", "bar"})
@@ -164,21 +173,39 @@ func TestClient(t *testing.T) {
 		{
 			name: "walk-to-dir",
 			fn: func(c *p9.Client) error {
+				// Walk only called when WalkGetAttr not available.
+				sf.WalkGetAttrMock.Err = syscall.ENOSYS
 				sf.WalkMock.Called = false
+				sf.WalkMock.Names = nil
 				sf.WalkMock.File = d
 				sf.WalkMock.Err = nil
 				sf.WalkMock.QIDs = []p9.QID{{Type: 1}}
+				// All intermediate values must be directories.
+				d.WalkGetAttrMock.Err = syscall.ENOSYS
+				d.WalkMock.Called = false
+				d.WalkMock.Names = nil
+				d.WalkMock.File = d // Walk to self.
+				d.WalkMock.Err = nil
+				d.WalkMock.QIDs = []p9.QID{{Type: 1}}
+				d.GetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory}
+				d.GetAttrMock.Valid = p9.AttrMask{Mode: true}
 				var qids []p9.QID
 				var err error
 				qids, _, err = sfFile.Walk([]string{"foo", "bar"})
 				if !sf.WalkMock.Called {
 					t.Errorf("Walk never Called?")
 				}
-				if !reflect.DeepEqual(sf.WalkMock.Names, []string{"foo", "bar"}) {
-					t.Errorf("got names %v wanted []{foo, bar}", sf.WalkMock.Names)
+				if !d.GetAttrMock.Called {
+					t.Errorf("GetAttr never Called?")
 				}
-				if len(qids) != 1 || qids[0].Type != 1 {
-					t.Errorf("got qids %v wanted []{{Type: 1}}", qids)
+				if !reflect.DeepEqual(sf.WalkMock.Names, []string{"foo"}) {
+					t.Errorf("got names %v wanted []{foo}", sf.WalkMock.Names)
+				}
+				if !reflect.DeepEqual(d.WalkMock.Names, []string{"bar"}) {
+					t.Errorf("got names %v wanted []{bar}", d.WalkMock.Names)
+				}
+				if len(qids) != 2 || qids[len(qids)-1].Type != 1 {
+					t.Errorf("got qids %v wanted []{..., {Type: 1}}", qids)
 				}
 				return err
 			},
@@ -187,11 +214,20 @@ func TestClient(t *testing.T) {
 			name: "walkgetattr-to-dir",
 			fn: func(c *p9.Client) error {
 				sf.WalkGetAttrMock.Called = false
+				sf.WalkGetAttrMock.Names = nil
 				sf.WalkGetAttrMock.File = d
 				sf.WalkGetAttrMock.Err = nil
 				sf.WalkGetAttrMock.QIDs = []p9.QID{{Type: 1}}
-				sf.WalkGetAttrMock.Attr = p9.Attr{UID: 1}
+				sf.WalkGetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory, UID: 1}
 				sf.WalkGetAttrMock.Valid = p9.AttrMask{Mode: true}
+				// See above.
+				d.WalkGetAttrMock.Called = false
+				d.WalkGetAttrMock.Names = nil
+				d.WalkGetAttrMock.File = d // Walk to self.
+				d.WalkGetAttrMock.Err = nil
+				d.WalkGetAttrMock.QIDs = []p9.QID{{Type: 1}}
+				d.WalkGetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory, UID: 1}
+				d.WalkGetAttrMock.Valid = p9.AttrMask{Mode: true}
 				var qids []p9.QID
 				var err error
 				var mask p9.AttrMask
@@ -200,11 +236,14 @@ func TestClient(t *testing.T) {
 				if !sf.WalkGetAttrMock.Called {
 					t.Errorf("Walk never Called?")
 				}
-				if !reflect.DeepEqual(sf.WalkGetAttrMock.Names, []string{"foo", "bar"}) {
-					t.Errorf("got names %v wanted []{foo, bar}", sf.WalkGetAttrMock.Names)
+				if !reflect.DeepEqual(sf.WalkGetAttrMock.Names, []string{"foo"}) {
+					t.Errorf("got names %v wanted []{foo}", sf.WalkGetAttrMock.Names)
 				}
-				if len(qids) != 1 || qids[0].Type != 1 {
-					t.Errorf("got qids %v wanted []{{Type: 1}}", qids)
+				if !reflect.DeepEqual(d.WalkGetAttrMock.Names, []string{"bar"}) {
+					t.Errorf("got names %v wanted []{bar}", d.WalkGetAttrMock.Names)
+				}
+				if len(qids) != 2 || qids[len(qids)-1].Type != 1 {
+					t.Errorf("got qids %v wanted []{..., {Type: 1}}", qids)
 				}
 				if !reflect.DeepEqual(attr, sf.WalkGetAttrMock.Attr) {
 					t.Errorf("got attrs %s wanted %s", attr, sf.WalkGetAttrMock.Attr)

pkg/p9/p9test/mocks.go

@@ -71,7 +71,8 @@ type WalkGetAttrMock struct {
 
 // WalkGetAttr implements p9.File.WalkGetAttr.
 func (w *WalkGetAttrMock) WalkGetAttr(names []string) ([]p9.QID, p9.File, p9.AttrMask, p9.Attr, error) {
-	w.Called, w.Names = true, names
+	w.Called = true
+	w.Names = append(w.Names, names...)
 	return w.QIDs, w.File, w.Valid, w.Attr, w.Err
 }
 
@@ -300,17 +301,14 @@ func (r *ReadlinkMock) Readlink() (string, error) {
 type AttachMock struct {
 	Called bool
 
-	// Args.
-	AttachName string
-
 	// Return.
 	File p9.File
 	Err  error
 }
 
 // Attach implements p9.Attacher.Attach.
-func (a *AttachMock) Attach(attachName string) (p9.File, error) {
-	a.Called, a.AttachName = true, attachName
+func (a *AttachMock) Attach() (p9.File, error) {
+	a.Called = true
 	return a.File, a.Err
 }
 
@@ -329,7 +327,8 @@ type WalkMock struct {
 
 // Walk implements p9.File.Walk.
 func (w *WalkMock) Walk(names []string) ([]p9.QID, p9.File, error) {
-	w.Called, w.Names = true, names
+	w.Called = true
+	w.Names = append(w.Names, names...)
 	return w.QIDs, w.File, w.Err
 }
 

pkg/p9/server.go

@@ -97,6 +97,9 @@ type fidRef struct {
 	// This is updated in handlers.go.
 	opened bool
 
+	// walkable indicates this fidRef may be walked.
+	walkable bool
+
 	// openFlags is the mode used in the open.
 	//
 	// This is updated in handlers.go.

runsc/fsgofer/fsgofer.go

@@ -117,17 +117,9 @@ func NewAttachPoint(prefix string, c Config) p9.Attacher {
 }
 
 // Attach implements p9.Attacher.
-func (a *attachPoint) Attach(appPath string) (p9.File, error) {
-	// Only proceed if 'appPath' is valid.
-	if !path.IsAbs(appPath) {
-		return nil, fmt.Errorf("invalid path %q", appPath)
-	}
-	if path.Clean(appPath) != appPath {
-		return nil, fmt.Errorf("invalid path %q", appPath)
-	}
-
-	root := path.Join(a.prefix, appPath)
-	fi, err := os.Stat(root)
+func (a *attachPoint) Attach() (p9.File, error) {
+	// Sanity check the prefix.
+	fi, err := os.Stat(a.prefix)
 	if err != nil {
 		return nil, err
 	}
@@ -136,14 +128,15 @@ func (a *attachPoint) Attach(appPath string) (p9.File, error) {
 		mode = os.O_RDONLY
 	}
 
-	f, err := os.OpenFile(root, mode|openFlags, 0)
+	// Open the root directory.
+	f, err := os.OpenFile(a.prefix, mode|openFlags, 0)
 	if err != nil {
-		return nil, fmt.Errorf("unable to open file %q, err: %v", root, err)
+		return nil, fmt.Errorf("unable to open file %q, err: %v", a.prefix, err)
 	}
 	stat, err := stat(int(f.Fd()))
 	if err != nil {
 		f.Close()
-		return nil, fmt.Errorf("failed to stat file %q, err: %v", root, err)
+		return nil, fmt.Errorf("failed to stat file %q, err: %v", a.prefix, err)
 	}
 
 	a.attachedMu.Lock()
@@ -154,7 +147,7 @@ func (a *attachPoint) Attach(appPath string) (p9.File, error) {
 	}
 	a.attached = true
 
-	return newLocalFile(a, f, root, stat)
+	return newLocalFile(a, f, a.prefix, stat)
 }
 
 // makeQID returns a unique QID for the given stat buffer.

runsc/fsgofer/fsgofer_test.go

@@ -19,7 +19,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
-	"strings"
 	"syscall"
 	"testing"
 
@@ -88,9 +87,9 @@ func runCustom(t *testing.T, types []fileType, confs []Config, test func(*testin
 			defer os.RemoveAll(path)
 
 			a := NewAttachPoint(path, c)
-			root, err := a.Attach("/")
+			root, err := a.Attach()
 			if err != nil {
-				t.Fatalf("Attach(%q) failed, err: %v", "/", err)
+				t.Fatalf("Attach failed, err: %v", err)
 			}
 
 			_, file, err := root.Walk([]string{name})
@@ -115,9 +114,9 @@ func setup(ft fileType) (string, string, error) {
 
 	// First attach with writable configuration to setup tree.
 	a := NewAttachPoint(path, Config{})
-	root, err := a.Attach("/")
+	root, err := a.Attach()
 	if err != nil {
-		return "", "", fmt.Errorf("Attach(%q) failed, err: %v", "/", err)
+		return "", "", fmt.Errorf("Attach failed, err: %v", err)
 	}
 	defer root.Close()
 
@@ -618,9 +617,9 @@ func TestAttachFile(t *testing.T) {
 	}
 
 	a := NewAttachPoint(path, conf)
-	root, err := a.Attach("/")
+	root, err := a.Attach()
 	if err != nil {
-		t.Fatalf("Attach(%q) failed, err: %v", "/", err)
+		t.Fatalf("Attach failed, err: %v", err)
 	}
 
 	if _, _, _, err := root.Open(p9.ReadWrite); err != nil {
@@ -649,31 +648,6 @@ func TestAttachFile(t *testing.T) {
 	}
 }
 
-func TestAttachError(t *testing.T) {
-	conf := Config{ROMount: false}
-	root, err := ioutil.TempDir("", "root-")
-	if err != nil {
-		t.Fatalf("ioutil.TempDir() failed, err: %v", err)
-	}
-	defer os.RemoveAll(root)
-	a := NewAttachPoint(root, conf)
-
-	c := path.Join(root, "test")
-	if err := os.Mkdir(c, 0700); err != nil {
-		t.Fatalf("os.Create(%q) failed, err: %v", c, err)
-	}
-
-	for _, p := range []string{"test", "/test/../", "/test/./", "/test//"} {
-		_, err := a.Attach(p)
-		if err == nil {
-			t.Fatalf("Attach(%q) should have failed", p)
-		}
-		if want := "invalid path"; !strings.Contains(err.Error(), want) {
-			t.Fatalf("Attach(%q) wrong error, got: %v, wanted: %v", p, err, want)
-		}
-	}
-}
-
 func TestDoubleAttachError(t *testing.T) {
 	conf := Config{ROMount: false}
 	root, err := ioutil.TempDir("", "root-")
@@ -683,10 +657,10 @@ func TestDoubleAttachError(t *testing.T) {
 	defer os.RemoveAll(root)
 	a := NewAttachPoint(root, conf)
 
-	if _, err := a.Attach("/"); err != nil {
-		t.Fatalf("Attach(%q) failed: %v", "/", err)
+	if _, err := a.Attach(); err != nil {
+		t.Fatalf("Attach failed: %v", err)
 	}
-	if _, err := a.Attach("/"); err == nil {
-		t.Fatalf("Attach(%q) should have failed", "test")
+	if _, err := a.Attach(); err == nil {
+		t.Fatalf("Attach should have failed, got %v want non-nil", err)
 	}
 }