-
Notifications
You must be signed in to change notification settings - Fork 2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.
- Loading branch information
Showing
6 changed files
with
295 additions
and
54 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| ``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
| }) | ||
| } |
Oops, something went wrong.