From 437bb4b86d8f5e5e9aebabe06d308cf8c4d29dfb Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Thu, 27 Jan 2022 12:46:56 -0600 Subject: [PATCH] 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 | 3 + client/allocdir/alloc_dir.go | 16 +-- client/allocdir/alloc_dir_test.go | 36 +++---- helper/escapingfs/escapes.go | 99 ++++++++++++++++++ helper/escapingfs/escapes_test.go | 162 ++++++++++++++++++++++++++++++ nomad/structs/structs.go | 33 +----- 6 files changed, 295 insertions(+), 54 deletions(-) create mode 100644 .changelog/12037.txt create mode 100644 helper/escapingfs/escapes.go create mode 100644 helper/escapingfs/escapes_test.go diff --git a/.changelog/12037.txt b/.changelog/12037.txt new file mode 100644 index 000000000..7b6a06bf3 --- /dev/null +++ b/.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) +``` diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go index 0dc52029c..da05aacb3 100644 --- a/client/allocdir/alloc_dir.go +++ b/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") diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index 86cd8d917..4a876c57b 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/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) { diff --git a/helper/escapingfs/escapes.go b/helper/escapingfs/escapes.go new file mode 100644 index 000000000..9296e7743 --- /dev/null +++ b/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 +} diff --git a/helper/escapingfs/escapes_test.go b/helper/escapingfs/escapes_test.go new file mode 100644 index 000000000..59e74ccb3 --- /dev/null +++ b/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// + {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) + }) +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 2c20932a2..2aa0fd883 100644 --- a/nomad/structs/structs.go +++ b/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 {