From 1b1a68e42f3868ab37f295cc0e47b7f104ec00bd Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 24 Aug 2022 09:56:42 -0500 Subject: [PATCH] cleanup: move fs helpers into escapingfs --- .../taskrunner/getter/getter_test.go | 6 +- client/taskenv/env.go | 3 +- command/agent/agent.go | 6 +- command/operator_debug.go | 5 +- helper/escapingfs/escapes.go | 22 +++++ helper/escapingfs/escapes_test.go | 86 +++++++++++++++++++ helper/funcs.go | 14 --- helper/funcs_test.go | 86 ------------------- helper/path.go | 15 ---- 9 files changed, 119 insertions(+), 124 deletions(-) delete mode 100644 helper/path.go diff --git a/client/allocrunner/taskrunner/getter/getter_test.go b/client/allocrunner/taskrunner/getter/getter_test.go index 6e23d49d0..ece5cbacd 100644 --- a/client/allocrunner/taskrunner/getter/getter_test.go +++ b/client/allocrunner/taskrunner/getter/getter_test.go @@ -19,7 +19,7 @@ import ( clientconfig "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/interfaces" "github.com/hashicorp/nomad/client/taskenv" - "github.com/hashicorp/nomad/helper" + "github.com/hashicorp/nomad/helper/escapingfs" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/stretchr/testify/require" @@ -31,11 +31,11 @@ type noopReplacer struct { } func clientPath(taskDir, path string, join bool) (string, bool) { - if !filepath.IsAbs(path) || (helper.PathEscapesSandbox(taskDir, path) && join) { + if !filepath.IsAbs(path) || (escapingfs.PathEscapesSandbox(taskDir, path) && join) { path = filepath.Join(taskDir, path) } path = filepath.Clean(path) - if taskDir != "" && !helper.PathEscapesSandbox(taskDir, path) { + if taskDir != "" && !escapingfs.PathEscapesSandbox(taskDir, path) { return path, false } return path, true diff --git a/client/taskenv/env.go b/client/taskenv/env.go index 7fb6a27cc..1ed6f5181 100644 --- a/client/taskenv/env.go +++ b/client/taskenv/env.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/nomad/helper" hargs "github.com/hashicorp/nomad/helper/args" + "github.com/hashicorp/nomad/helper/escapingfs" "github.com/hashicorp/nomad/lib/cpuset" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/plugins/drivers" @@ -341,7 +342,7 @@ func (t *TaskEnv) replaceEnvClient(arg string) string { // directory path fields of this TaskEnv func (t *TaskEnv) checkEscape(testPath string) bool { for _, p := range []string{t.clientTaskDir, t.clientSharedAllocDir} { - if p != "" && !helper.PathEscapesSandbox(p, testPath) { + if p != "" && !escapingfs.PathEscapesSandbox(p, testPath) { return false } } diff --git a/command/agent/agent.go b/command/agent/agent.go index 5c3aae4a8..835ae92d1 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -24,8 +24,8 @@ import ( "github.com/hashicorp/nomad/client/state" "github.com/hashicorp/nomad/command/agent/consul" "github.com/hashicorp/nomad/command/agent/event" - "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/bufconndialer" + "github.com/hashicorp/nomad/helper/escapingfs" "github.com/hashicorp/nomad/helper/pluginutils/loader" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/lib/cpuset" @@ -868,7 +868,7 @@ func (a *Agent) setupNodeID(config *nomad.Config) error { return err } // Persist this configured nodeID to our data directory - if err := helper.EnsurePath(fileID, false); err != nil { + if err := escapingfs.EnsurePath(fileID, false); err != nil { return err } if err := ioutil.WriteFile(fileID, []byte(config.NodeID), 0600); err != nil { @@ -880,7 +880,7 @@ func (a *Agent) setupNodeID(config *nomad.Config) error { // If we still don't have a valid node ID, make one. if config.NodeID == "" { id := uuid.Generate() - if err := helper.EnsurePath(fileID, false); err != nil { + if err := escapingfs.EnsurePath(fileID, false); err != nil { return err } if err := ioutil.WriteFile(fileID, []byte(id), 0600); err != nil { diff --git a/command/operator_debug.go b/command/operator_debug.go index d9b391e26..7966f7574 100644 --- a/command/operator_debug.go +++ b/command/operator_debug.go @@ -27,6 +27,7 @@ import ( "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/api/contexts" "github.com/hashicorp/nomad/helper" + "github.com/hashicorp/nomad/helper/escapingfs" "github.com/hashicorp/nomad/version" "github.com/posener/complete" ) @@ -729,7 +730,7 @@ func (c *OperatorDebugCommand) mkdir(paths ...string) error { joinedPath := c.path(paths...) // Ensure path doesn't escape the sandbox of the capture directory - escapes := helper.PathEscapesSandbox(c.collectDir, joinedPath) + escapes := escapingfs.PathEscapesSandbox(c.collectDir, joinedPath) if escapes { return fmt.Errorf("file path escapes capture directory") } @@ -1273,7 +1274,7 @@ func (c *OperatorDebugCommand) writeBytes(dir, file string, data []byte) error { } // Ensure filename doesn't escape the sandbox of the capture directory - escapes := helper.PathEscapesSandbox(c.collectDir, filePath) + escapes := escapingfs.PathEscapesSandbox(c.collectDir, filePath) if escapes { return fmt.Errorf("file path \"%s\" escapes capture directory \"%s\"", filePath, c.collectDir) } diff --git a/helper/escapingfs/escapes.go b/helper/escapingfs/escapes.go index 9296e7743..d8eff9467 100644 --- a/helper/escapingfs/escapes.go +++ b/helper/escapingfs/escapes.go @@ -97,3 +97,25 @@ func PathEscapesAllocDir(base, prefix, path string) (bool, error) { return false, nil } + +// PathEscapesSandbox returns whether previously cleaned path inside the +// sandbox directory (typically this will be the allocation directory) +// escapes. +func PathEscapesSandbox(sandboxDir, path string) bool { + rel, err := filepath.Rel(sandboxDir, path) + if err != nil { + return true + } + if strings.HasPrefix(rel, "..") { + return true + } + return false +} + +// EnsurePath is used to make sure a path exists +func EnsurePath(path string, dir bool) error { + if !dir { + path = filepath.Dir(path) + } + return os.MkdirAll(path, 0755) +} diff --git a/helper/escapingfs/escapes_test.go b/helper/escapingfs/escapes_test.go index 2c7e6254c..8885cfe05 100644 --- a/helper/escapingfs/escapes_test.go +++ b/helper/escapingfs/escapes_test.go @@ -1,6 +1,7 @@ package escapingfs import ( + "fmt" "io/ioutil" "os" "path/filepath" @@ -141,3 +142,88 @@ func Test_PathEscapesAllocDir(t *testing.T) { require.True(t, escape) }) } + +func TestPathEscapesSandbox(t *testing.T) { + cases := []struct { + name string + path string + dir string + expected bool + }{ + { + // this is the ${NOMAD_SECRETS_DIR} case + name: "ok joined absolute path inside sandbox", + path: filepath.Join("/alloc", "/secrets"), + dir: "/alloc", + expected: false, + }, + { + name: "fail unjoined absolute path outside sandbox", + path: "/secrets", + dir: "/alloc", + expected: true, + }, + { + name: "ok joined relative path inside sandbox", + path: filepath.Join("/alloc", "./safe"), + dir: "/alloc", + expected: false, + }, + { + name: "fail unjoined relative path outside sandbox", + path: "./safe", + dir: "/alloc", + expected: true, + }, + { + name: "ok relative path traversal constrained to sandbox", + path: filepath.Join("/alloc", "../../alloc/safe"), + dir: "/alloc", + expected: false, + }, + { + name: "ok unjoined absolute path traversal constrained to sandbox", + path: filepath.Join("/alloc", "/../alloc/safe"), + dir: "/alloc", + expected: false, + }, + { + name: "ok unjoined absolute path traversal constrained to sandbox", + path: "/../alloc/safe", + dir: "/alloc", + expected: false, + }, + { + name: "fail joined relative path traverses outside sandbox", + path: filepath.Join("/alloc", "../../../unsafe"), + dir: "/alloc", + expected: true, + }, + { + name: "fail unjoined relative path traverses outside sandbox", + path: "../../../unsafe", + dir: "/alloc", + expected: true, + }, + { + name: "fail joined absolute path tries to transverse outside sandbox", + path: filepath.Join("/alloc", "/alloc/../../unsafe"), + dir: "/alloc", + expected: true, + }, + { + name: "fail unjoined absolute path tries to transverse outside sandbox", + path: "/alloc/../../unsafe", + dir: "/alloc", + expected: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + caseMsg := fmt.Sprintf("path: %v\ndir: %v", tc.path, tc.dir) + escapes := PathEscapesSandbox(tc.dir, tc.path) + require.Equal(t, tc.expected, escapes, caseMsg) + }) + } +} diff --git a/helper/funcs.go b/helper/funcs.go index 93cb5b9b2..152e93c51 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -609,20 +609,6 @@ func CheckNamespaceScope(provided string, requested []string) []string { return nil } -// PathEscapesSandbox returns whether previously cleaned path inside the -// sandbox directory (typically this will be the allocation directory) -// escapes. -func PathEscapesSandbox(sandboxDir, path string) bool { - rel, err := filepath.Rel(sandboxDir, path) - if err != nil { - return true - } - if strings.HasPrefix(rel, "..") { - return true - } - return false -} - // StopFunc is used to stop a time.Timer created with NewSafeTimer type StopFunc func() diff --git a/helper/funcs_test.go b/helper/funcs_test.go index 5d882061f..3c5c64f15 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -2,7 +2,6 @@ package helper import ( "fmt" - "path/filepath" "reflect" "sort" "testing" @@ -449,91 +448,6 @@ func TestCheckNamespaceScope(t *testing.T) { } } -func TestPathEscapesSandbox(t *testing.T) { - cases := []struct { - name string - path string - dir string - expected bool - }{ - { - // this is the ${NOMAD_SECRETS_DIR} case - name: "ok joined absolute path inside sandbox", - path: filepath.Join("/alloc", "/secrets"), - dir: "/alloc", - expected: false, - }, - { - name: "fail unjoined absolute path outside sandbox", - path: "/secrets", - dir: "/alloc", - expected: true, - }, - { - name: "ok joined relative path inside sandbox", - path: filepath.Join("/alloc", "./safe"), - dir: "/alloc", - expected: false, - }, - { - name: "fail unjoined relative path outside sandbox", - path: "./safe", - dir: "/alloc", - expected: true, - }, - { - name: "ok relative path traversal constrained to sandbox", - path: filepath.Join("/alloc", "../../alloc/safe"), - dir: "/alloc", - expected: false, - }, - { - name: "ok unjoined absolute path traversal constrained to sandbox", - path: filepath.Join("/alloc", "/../alloc/safe"), - dir: "/alloc", - expected: false, - }, - { - name: "ok unjoined absolute path traversal constrained to sandbox", - path: "/../alloc/safe", - dir: "/alloc", - expected: false, - }, - { - name: "fail joined relative path traverses outside sandbox", - path: filepath.Join("/alloc", "../../../unsafe"), - dir: "/alloc", - expected: true, - }, - { - name: "fail unjoined relative path traverses outside sandbox", - path: "../../../unsafe", - dir: "/alloc", - expected: true, - }, - { - name: "fail joined absolute path tries to transverse outside sandbox", - path: filepath.Join("/alloc", "/alloc/../../unsafe"), - dir: "/alloc", - expected: true, - }, - { - name: "fail unjoined absolute path tries to transverse outside sandbox", - path: "/alloc/../../unsafe", - dir: "/alloc", - expected: true, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - caseMsg := fmt.Sprintf("path: %v\ndir: %v", tc.path, tc.dir) - escapes := PathEscapesSandbox(tc.dir, tc.path) - require.Equal(t, tc.expected, escapes, caseMsg) - }) - } -} - func Test_NewSafeTimer(t *testing.T) { t.Run("zero", func(t *testing.T) { timer, stop := NewSafeTimer(0) diff --git a/helper/path.go b/helper/path.go deleted file mode 100644 index a4684a29b..000000000 --- a/helper/path.go +++ /dev/null @@ -1,15 +0,0 @@ -// These functions are coming from consul/path.go -package helper - -import ( - "os" - "path/filepath" -) - -// EnsurePath is used to make sure a path exists -func EnsurePath(path string, dir bool) error { - if !dir { - path = filepath.Dir(path) - } - return os.MkdirAll(path, 0755) -}