Merge pull request #1786 from hashicorp/b-fs-enforce

Do not allow path to escape the alloc dir for the FS commands

client/allocdir/alloc_dir.go

@@ -110,7 +110,7 @@ type AllocDirFS interface {
 	Stat(path string) (*AllocFileInfo, error)
 	ReadAt(path string, offset int64) (io.ReadCloser, error)
 	Snapshot(w io.Writer) error
-	BlockUntilExists(path string, t *tomb.Tomb) chan error
+	BlockUntilExists(path string, t *tomb.Tomb) (chan error, error)
 	ChangeEvents(path string, curOffset int64, t *tomb.Tomb) (*watch.FileChanges, error)
 }
 
@@ -459,6 +459,12 @@ func (d *AllocDir) LogDir() string {
 
 // List returns the list of files at a path relative to the alloc dir
 func (d *AllocDir) List(path string) ([]*AllocFileInfo, error) {
+	if escapes, err := structs.PathEscapesAllocDir(path); err != nil {
+		return nil, fmt.Errorf("Failed to check if path escapes alloc directory: %v", err)
+	} else if escapes {
+		return nil, fmt.Errorf("Path escapes the alloc directory")
+	}
+
 	p := filepath.Join(d.AllocDir, path)
 	finfos, err := ioutil.ReadDir(p)
 	if err != nil {
@@ -479,6 +485,12 @@ func (d *AllocDir) List(path string) ([]*AllocFileInfo, error) {
 
 // Stat returns information about the file at a path relative to the alloc dir
 func (d *AllocDir) Stat(path string) (*AllocFileInfo, error) {
+	if escapes, err := structs.PathEscapesAllocDir(path); err != nil {
+		return nil, fmt.Errorf("Failed to check if path escapes alloc directory: %v", err)
+	} else if escapes {
+		return nil, fmt.Errorf("Path escapes the alloc directory")
+	}
+
 	p := filepath.Join(d.AllocDir, path)
 	info, err := os.Stat(p)
 	if err != nil {
@@ -496,6 +508,12 @@ func (d *AllocDir) Stat(path string) (*AllocFileInfo, error) {
 
 // ReadAt returns a reader for a file at the path relative to the alloc dir
 func (d *AllocDir) ReadAt(path string, offset int64) (io.ReadCloser, error) {
+	if escapes, err := structs.PathEscapesAllocDir(path); err != nil {
+		return nil, fmt.Errorf("Failed to check if path escapes alloc directory: %v", err)
+	} else if escapes {
+		return nil, fmt.Errorf("Path escapes the alloc directory")
+	}
+
 	p := filepath.Join(d.AllocDir, path)
 	f, err := os.Open(p)
 	if err != nil {
@@ -509,7 +527,13 @@ func (d *AllocDir) ReadAt(path string, offset int64) (io.ReadCloser, error) {
 
 // BlockUntilExists blocks until the passed file relative the allocation
 // directory exists. The block can be cancelled with the passed tomb.
-func (d *AllocDir) BlockUntilExists(path string, t *tomb.Tomb) chan error {
+func (d *AllocDir) BlockUntilExists(path string, t *tomb.Tomb) (chan error, error) {
+	if escapes, err := structs.PathEscapesAllocDir(path); err != nil {
+		return nil, fmt.Errorf("Failed to check if path escapes alloc directory: %v", err)
+	} else if escapes {
+		return nil, fmt.Errorf("Path escapes the alloc directory")
+	}
+
 	// Get the path relative to the alloc directory
 	p := filepath.Join(d.AllocDir, path)
 	watcher := getFileWatcher(p)
@@ -518,13 +542,19 @@ func (d *AllocDir) BlockUntilExists(path string, t *tomb.Tomb) chan error {
 		returnCh <- watcher.BlockUntilExists(t)
 		close(returnCh)
 	}()
-	return returnCh
+	return returnCh, nil
 }
 
 // ChangeEvents watches for changes to the passed path relative to the
 // allocation directory. The offset should be the last read offset. The tomb is
 // used to clean up the watch.
 func (d *AllocDir) ChangeEvents(path string, curOffset int64, t *tomb.Tomb) (*watch.FileChanges, error) {
+	if escapes, err := structs.PathEscapesAllocDir(path); err != nil {
+		return nil, fmt.Errorf("Failed to check if path escapes alloc directory: %v", err)
+	} else if escapes {
+		return nil, fmt.Errorf("Path escapes the alloc directory")
+	}
+
 	// Get the path relative to the alloc directory
 	p := filepath.Join(d.AllocDir, path)
 	watcher := getFileWatcher(p)

client/allocdir/alloc_dir_test.go

@@ -9,8 +9,11 @@ import (
 	"path/filepath"
 	"reflect"
 	"runtime"
+	"strings"
 	"testing"
 
+	tomb "gopkg.in/tomb.v1"
+
 	"github.com/hashicorp/nomad/client/testutil"
 	"github.com/hashicorp/nomad/nomad/structs"
 )
@@ -345,3 +348,45 @@ func TestAllocDir_Move(t *testing.T) {
 		t.Fatalf("task local dir was not moved")
 	}
 }
+
+func TestAllocDir_EscapeChecking(t *testing.T) {
+	tmp, err := ioutil.TempDir("", "AllocDir")
+	if err != nil {
+		t.Fatalf("Couldn't create temp dir: %v", err)
+	}
+	defer os.RemoveAll(tmp)
+
+	d := NewAllocDir(tmp, structs.DefaultResources().DiskMB)
+	defer d.Destroy()
+	tasks := []*structs.Task{t1, t2}
+	if err := d.Build(tasks); err != nil {
+		t.Fatalf("Build(%v) failed: %v", tasks, err)
+	}
+
+	// Check that issuing calls that escape the alloc dir returns errors
+	// List
+	if _, err := d.List(".."); err == nil || !strings.Contains(err.Error(), "escapes") {
+		t.Fatalf("List of escaping path didn't error: %v", err)
+	}
+
+	// Stat
+	if _, err := d.Stat("../foo"); err == nil || !strings.Contains(err.Error(), "escapes") {
+		t.Fatalf("Stat of escaping path didn't error: %v", err)
+	}
+
+	// ReadAt
+	if _, err := d.ReadAt("../foo", 0); err == nil || !strings.Contains(err.Error(), "escapes") {
+		t.Fatalf("ReadAt of escaping path didn't error: %v", err)
+	}
+
+	// BlockUntilExists
+	tomb := tomb.Tomb{}
+	if _, err := d.BlockUntilExists("../foo", &tomb); err == nil || !strings.Contains(err.Error(), "escapes") {
+		t.Fatalf("BlockUntilExists of escaping path didn't error: %v", err)
+	}
+
+	// ChangeEvents
+	if _, err := d.ChangeEvents("../foo", 0, &tomb); err == nil || !strings.Contains(err.Error(), "escapes") {
+		t.Fatalf("ChangeEvents of escaping path didn't error: %v", err)
+	}
+}

command/agent/fs_endpoint.go

@@ -856,7 +856,13 @@ func blockUntilNextLog(fs allocdir.AllocDirFS, t *tomb.Tomb, logPath, task, logT
 	next := make(chan error, 1)
 
 	go func() {
-		eofCancelCh := fs.BlockUntilExists(nextPath, t)
+		eofCancelCh, err := fs.BlockUntilExists(nextPath, t)
+		if err != nil {
+			next <- err
+			close(next)
+			return
+		}
+
 		scanCh := time.Tick(nextLogCheckRate)
 		for {
 			select {

nomad/structs/structs.go

@@ -2274,7 +2274,7 @@ func (t *Template) Validate() error {
 	}
 
 	// Verify the destination doesn't escape
-	escaped, err := pathEscapesAllocDir(t.DestPath)
+	escaped, err := PathEscapesAllocDir(t.DestPath)
 	if err != nil {
 		mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid destination path: %v", err))
 	} else if escaped {
@@ -2593,9 +2593,9 @@ func (ta *TaskArtifact) GoString() string {
 	return fmt.Sprintf("%+v", ta)
 }
 
-// pathEscapesAllocDir returns if the given path escapes the allocation
+// PathEscapesAllocDir returns if the given path escapes the allocation
 // directory
-func pathEscapesAllocDir(path string) (bool, error) {
+func PathEscapesAllocDir(path string) (bool, error) {
 	// Verify the destination doesn't escape the tasks directory
 	alloc, err := filepath.Abs(filepath.Join("/", "foo/", "bar/"))
 	if err != nil {
@@ -2620,7 +2620,7 @@ func (ta *TaskArtifact) Validate() error {
 		mErr.Errors = append(mErr.Errors, fmt.Errorf("source must be specified"))
 	}
 
-	escaped, err := pathEscapesAllocDir(ta.RelativeDest)
+	escaped, err := PathEscapesAllocDir(ta.RelativeDest)
 	if err != nil {
 		mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid destination path: %v", err))
 	} else if escaped {