client: check escaping of alloc dir using symlinks

This PR adds symlink resolution when doing validation of paths
to ensure they do not escape client allocation directories.

.changelog/12037.txt

@@ -0,0 +1,3 @@
+```release-note:security
+Resolve symlinks to prevent unauthorized access to files outside the allocation directory. [CVE-2022-24683](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-24683)
+```

client/allocdir/alloc_dir.go

@@ -6,17 +6,17 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"time"
 
-	"net/http"
-	"strings"
-
 	hclog "github.com/hashicorp/go-hclog"
 	multierror "github.com/hashicorp/go-multierror"
 	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/helper/escapingfs"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hpcloud/tail/watch"
 	tomb "gopkg.in/tomb.v1"
@@ -350,7 +350,7 @@ func (d *AllocDir) Build() error {
 
 // List returns the list of files at a path relative to the alloc dir
 func (d *AllocDir) List(path string) ([]*cstructs.AllocFileInfo, error) {
-	if escapes, err := structs.PathEscapesAllocDir("", path); err != nil {
+	if escapes, err := escapingfs.PathEscapesAllocDir(d.AllocDir, "", 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")
@@ -376,7 +376,7 @@ func (d *AllocDir) List(path string) ([]*cstructs.AllocFileInfo, error) {
 
 // Stat returns information about the file at a path relative to the alloc dir
 func (d *AllocDir) Stat(path string) (*cstructs.AllocFileInfo, error) {
-	if escapes, err := structs.PathEscapesAllocDir("", path); err != nil {
+	if escapes, err := escapingfs.PathEscapesAllocDir(d.AllocDir, "", 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")
@@ -426,7 +426,7 @@ func detectContentType(fileInfo os.FileInfo, path string) string {
 
 // 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 {
+	if escapes, err := escapingfs.PathEscapesAllocDir(d.AllocDir, "", 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")
@@ -457,7 +457,7 @@ 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 context.
 func (d *AllocDir) BlockUntilExists(ctx context.Context, path string) (chan error, error) {
-	if escapes, err := structs.PathEscapesAllocDir("", path); err != nil {
+	if escapes, err := escapingfs.PathEscapesAllocDir(d.AllocDir, "", 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")
@@ -483,7 +483,7 @@ func (d *AllocDir) BlockUntilExists(ctx context.Context, path string) (chan erro
 // allocation directory. The offset should be the last read offset. The context is
 // used to clean up the watch.
 func (d *AllocDir) ChangeEvents(ctx context.Context, path string, curOffset int64) (*watch.FileChanges, error) {
-	if escapes, err := structs.PathEscapesAllocDir("", path); err != nil {
+	if escapes, err := escapingfs.PathEscapesAllocDir(d.AllocDir, "", 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")

client/allocdir/alloc_dir_test.go

@@ -332,28 +332,30 @@ func TestAllocDir_EscapeChecking(t *testing.T) {
 
 // Test that `nomad fs` can't read secrets
 func TestAllocDir_ReadAt_SecretDir(t *testing.T) {
-	tmp, err := ioutil.TempDir("", "AllocDir")
-	if err != nil {
-		t.Fatalf("Couldn't create temp dir: %v", err)
-	}
-	defer os.RemoveAll(tmp)
+	tmp := t.TempDir()
 
 	d := NewAllocDir(testlog.HCLogger(t), tmp, "test")
-	if err := d.Build(); err != nil {
-		t.Fatalf("Build() failed: %v", err)
-	}
-	defer d.Destroy()
+	err := d.Build()
+	require.NoError(t, err)
+	defer func() {
+		_ = d.Destroy()
+	}()
 
 	td := d.NewTaskDir(t1.Name)
-	if err := td.Build(false, nil); err != nil {
-		t.Fatalf("TaskDir.Build() failed: %v", err)
-	}
+	err = td.Build(false, nil)
+	require.NoError(t, err)
 
-	// ReadAt of secret dir should fail
-	secret := filepath.Join(t1.Name, TaskSecrets, "test_file")
-	if _, err := d.ReadAt(secret, 0); err == nil || !strings.Contains(err.Error(), "secret file prohibited") {
-		t.Fatalf("ReadAt of secret file didn't error: %v", err)
-	}
+	// something to write and test reading
+	target := filepath.Join(t1.Name, TaskSecrets, "test_file")
+
+	// create target file in the task secrets dir
+	full := filepath.Join(d.AllocDir, target)
+	err = ioutil.WriteFile(full, []byte("hi"), 0600)
+	require.NoError(t, err)
+
+	// ReadAt of a file in the task secrets dir should fail
+	_, err = d.ReadAt(target, 0)
+	require.EqualError(t, err, "Reading secret file prohibited: web/secrets/test_file")
 }
 
 func TestAllocDir_SplitPath(t *testing.T) {

helper/escapingfs/escapes.go

@@ -0,0 +1,99 @@
+package escapingfs
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+// PathEscapesAllocViaRelative returns if the given path escapes the allocation
+// directory using relative paths.
+//
+// Only for use in server-side validation, where the real filesystem is not available.
+// For client-side validation use PathEscapesAllocDir, which includes symlink validation
+// as well.
+//
+// The prefix is joined to the path (e.g. "task/local"), and this function
+// checks if path escapes the alloc dir, NOT the prefix directory within the alloc dir.
+// With prefix="task/local", it will return false for "../secret", but
+// true for "../../../../../../root" path; only the latter escapes the alloc dir.
+func PathEscapesAllocViaRelative(prefix, path string) (bool, error) {
+	// Verify the destination does not escape the task's directory. The "alloc-dir"
+	// and "alloc-id" here are just placeholders; on a real filesystem they will
+	// have different names. The names are not important, but rather the number of levels
+	// in the path they represent.
+	alloc, err := filepath.Abs(filepath.Join("/", "alloc-dir/", "alloc-id/"))
+	if err != nil {
+		return false, err
+	}
+	abs, err := filepath.Abs(filepath.Join(alloc, prefix, path))
+	if err != nil {
+		return false, err
+	}
+	rel, err := filepath.Rel(alloc, abs)
+	if err != nil {
+		return false, err
+	}
+
+	return strings.HasPrefix(rel, ".."), nil
+}
+
+// pathEscapesBaseViaSymlink returns if path escapes dir, taking into account evaluation
+// of symlinks.
+//
+// The base directory must be an absolute path.
+func pathEscapesBaseViaSymlink(base, full string) (bool, error) {
+	resolveSym, err := filepath.EvalSymlinks(full)
+	if err != nil {
+		return false, err
+	}
+
+	rel, err := filepath.Rel(resolveSym, base)
+	if err != nil {
+		return true, nil
+	}
+
+	// note: this is not the same as !filesystem.IsAbs; we are asking if the relative
+	// path is descendent of the base path, indicating it does not escape.
+	isRelative := strings.HasPrefix(rel, "..") || rel == "."
+	escapes := !isRelative
+	return escapes, nil
+}
+
+// PathEscapesAllocDir returns true if base/prefix/path escapes the given base directory.
+//
+// Escaping a directory can be done with relative paths (e.g. ../../ etc.) or by
+// using symlinks. This checks both methods.
+//
+// The base directory must be an absolute path.
+func PathEscapesAllocDir(base, prefix, path string) (bool, error) {
+	full := filepath.Join(base, prefix, path)
+
+	// If base is not an absolute path, the caller passed in the wrong thing.
+	if !filepath.IsAbs(base) {
+		return false, errors.New("alloc dir must be absolute")
+	}
+
+	// Check path does not escape the alloc dir using relative paths.
+	if escapes, err := PathEscapesAllocViaRelative(prefix, path); err != nil {
+		return false, err
+	} else if escapes {
+		return true, nil
+	}
+
+	// Check path does not escape the alloc dir using symlinks.
+	if escapes, err := pathEscapesBaseViaSymlink(base, full); err != nil {
+		if os.IsNotExist(err) {
+			// Treat non-existent files as non-errors; perhaps not ideal but we
+			// have existing features (log-follow) that depend on this. Still safe,
+			// because we do the symlink check on every ReadAt call also.
+			return false, nil
+		}
+		return false, err
+	} else if escapes {
+		return true, nil
+	}
+
+	return false, nil
+}

helper/escapingfs/escapes_test.go

@@ -0,0 +1,162 @@
+package escapingfs
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func setup(t *testing.T) string {
+	p, err := ioutil.TempDir("", "escapist")
+	require.NoError(t, err)
+	return p
+}
+
+func cleanup(t *testing.T, root string) {
+	err := os.RemoveAll(root)
+	require.NoError(t, err)
+}
+
+func write(t *testing.T, file, data string) {
+	err := ioutil.WriteFile(file, []byte(data), 0600)
+	require.NoError(t, err)
+}
+
+func Test_PathEscapesAllocViaRelative(t *testing.T) {
+	for _, test := range []struct {
+		prefix string
+		path   string
+		exp    bool
+	}{
+		// directly under alloc-dir/alloc-id/
+		{prefix: "", path: "", exp: false},
+		{prefix: "", path: "/foo", exp: false},
+		{prefix: "", path: "./", exp: false},
+		{prefix: "", path: "../", exp: true}, // at alloc-id/
+
+		// under alloc-dir/alloc-id/<foo>/
+		{prefix: "foo", path: "", exp: false},
+		{prefix: "foo", path: "/foo", exp: false},
+		{prefix: "foo", path: "../", exp: false},   // at foo/
+		{prefix: "foo", path: "../../", exp: true}, // at alloc-id/
+
+		// under alloc-dir/alloc-id/foo/bar/
+		{prefix: "foo/bar", path: "", exp: false},
+		{prefix: "foo/bar", path: "/foo", exp: false},
+		{prefix: "foo/bar", path: "../", exp: false},      // at bar/
+		{prefix: "foo/bar", path: "../../", exp: false},   // at foo/
+		{prefix: "foo/bar", path: "../../../", exp: true}, // at alloc-id/
+	} {
+		result, err := PathEscapesAllocViaRelative(test.prefix, test.path)
+		require.NoError(t, err)
+		require.Equal(t, test.exp, result)
+	}
+}
+
+func Test_pathEscapesBaseViaSymlink(t *testing.T) {
+	t.Run("symlink-escape", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		// link from dir/link
+		link := filepath.Join(dir, "link")
+
+		// link to /tmp
+		target := filepath.Clean("/tmp")
+		err := os.Symlink(target, link)
+		require.NoError(t, err)
+
+		escape, err := pathEscapesBaseViaSymlink(dir, link)
+		require.NoError(t, err)
+		require.True(t, escape)
+	})
+
+	t.Run("symlink-noescape", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		// create a file within dir
+		target := filepath.Join(dir, "foo")
+		write(t, target, "hi")
+
+		// link to file within dir
+		link := filepath.Join(dir, "link")
+		err := os.Symlink(target, link)
+		require.NoError(t, err)
+
+		// link to file within dir does not escape dir
+		escape, err := pathEscapesBaseViaSymlink(dir, link)
+		require.NoError(t, err)
+		require.False(t, escape)
+	})
+}
+
+func Test_PathEscapesAllocDir(t *testing.T) {
+
+	t.Run("no-escape-root", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		escape, err := PathEscapesAllocDir(dir, "", "/")
+		require.NoError(t, err)
+		require.False(t, escape)
+	})
+
+	t.Run("no-escape", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		write(t, filepath.Join(dir, "foo"), "hi")
+
+		escape, err := PathEscapesAllocDir(dir, "", "/foo")
+		require.NoError(t, err)
+		require.False(t, escape)
+	})
+
+	t.Run("no-escape-no-exist", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		escape, err := PathEscapesAllocDir(dir, "", "/no-exist")
+		require.NoError(t, err)
+		require.False(t, escape)
+	})
+
+	t.Run("symlink-escape", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		// link from dir/link
+		link := filepath.Join(dir, "link")
+
+		// link to /tmp
+		target := filepath.Clean("/tmp")
+		err := os.Symlink(target, link)
+		require.NoError(t, err)
+
+		escape, err := PathEscapesAllocDir(dir, "", "/link")
+		require.NoError(t, err)
+		require.True(t, escape)
+	})
+
+	t.Run("relative-escape", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		escape, err := PathEscapesAllocDir(dir, "", "../../foo")
+		require.NoError(t, err)
+		require.True(t, escape)
+	})
+
+	t.Run("relative-escape-prefix", func(t *testing.T) {
+		dir := setup(t)
+		defer cleanup(t, dir)
+
+		escape, err := PathEscapesAllocDir(dir, "/foo/bar", "../../../foo")
+		require.NoError(t, err)
+		require.True(t, escape)
+	})
+}

nomad/structs/structs.go

@@ -17,7 +17,6 @@ import (
 	"math"
 	"net"
 	"os"
-	"path/filepath"
 	"reflect"
 	"regexp"
 	"sort"
@@ -25,6 +24,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/nomad/helper/escapingfs"
 	"golang.org/x/crypto/blake2b"
 
 	"github.com/hashicorp/cronexpr"
@@ -5316,7 +5316,7 @@ func (d *DispatchPayloadConfig) Copy() *DispatchPayloadConfig {
 
 func (d *DispatchPayloadConfig) Validate() error {
 	// Verify the destination doesn't escape
-	escaped, err := PathEscapesAllocDir("task/local/", d.File)
+	escaped, err := escapingfs.PathEscapesAllocViaRelative("task/local/", d.File)
 	if err != nil {
 		return fmt.Errorf("invalid destination path: %v", err)
 	} else if escaped {
@@ -7535,7 +7535,7 @@ func (t *Template) Validate() error {
 	}
 
 	// Verify the destination doesn't escape
-	escaped, err := PathEscapesAllocDir("task", t.DestPath)
+	escaped, err := escapingfs.PathEscapesAllocViaRelative("task", t.DestPath)
 	if err != nil {
 		mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid destination path: %v", err))
 	} else if escaped {
@@ -8333,31 +8333,6 @@ func (ta *TaskArtifact) Hash() string {
 	return base64.RawStdEncoding.EncodeToString(h.Sum(nil))
 }
 
-// PathEscapesAllocDir returns if the given path escapes the allocation
-// directory.
-//
-// The prefix is to joined to the path (e.g. "task/local"), and this function
-// checks if path escapes the alloc dir, NOT the prefix directory within the alloc dir.
-// With prefix="task/local", it will return false for "../secret", but
-// true for "../../../../../../root" path; only the latter escapes the alloc dir
-func PathEscapesAllocDir(prefix, path string) (bool, error) {
-	// Verify the destination doesn't escape the tasks directory
-	alloc, err := filepath.Abs(filepath.Join("/", "alloc-dir/", "alloc-id/"))
-	if err != nil {
-		return false, err
-	}
-	abs, err := filepath.Abs(filepath.Join(alloc, prefix, path))
-	if err != nil {
-		return false, err
-	}
-	rel, err := filepath.Rel(alloc, abs)
-	if err != nil {
-		return false, err
-	}
-
-	return strings.HasPrefix(rel, ".."), nil
-}
-
 func (ta *TaskArtifact) Validate() error {
 	// Verify the source
 	var mErr multierror.Error
@@ -8376,7 +8351,7 @@ func (ta *TaskArtifact) Validate() error {
 			ta.GetterMode, GetterModeAny, GetterModeFile, GetterModeDir))
 	}
 
-	escaped, err := PathEscapesAllocDir("task", ta.RelativeDest)
+	escaped, err := escapingfs.PathEscapesAllocViaRelative("task", ta.RelativeDest)
 	if err != nil {
 		mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid destination path: %v", err))
 	} else if escaped {