From fd1e40537cf56a91541f8674eb298d9ffd11c028 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Wed, 27 Aug 2025 10:37:34 -0700 Subject: [PATCH] [artifact] add artifact inspection after download (#26608) This adds artifact inspection after download to detect any issues with the content fetched. Currently this means checking for any symlinks within the artifact that resolve outside the task or allocation directories. On platforms where lockdown is available (some Linux) this inspection is not performed. The inspection can be disabled with the DisableArtifactInspection option. A dedicated option for disabling this behavior allows the DisableFilesystemIsolation option to be enabled but still have artifacts inspected after download. --- .changelog/26608.txt | 3 + .../allocrunner/taskrunner/getter/params.go | 3 + .../taskrunner/getter/params_test.go | 1 + .../allocrunner/taskrunner/getter/sandbox.go | 1 + .../taskrunner/getter/sandbox_test.go | 126 ++++++++++++++++++ .../allocrunner/taskrunner/getter/testing.go | 4 + client/allocrunner/taskrunner/getter/util.go | 122 +++++++++++++++++ .../taskrunner/getter/util_default.go | 5 + .../taskrunner/getter/util_linux.go | 6 + .../taskrunner/getter/util_test.go | 47 +++++++ .../taskrunner/getter/util_windows.go | 5 + client/config/artifact.go | 2 + nomad/structs/config/artifact.go | 17 +++ website/content/docs/configuration/client.mdx | 5 + 14 files changed, 347 insertions(+) create mode 100644 .changelog/26608.txt diff --git a/.changelog/26608.txt b/.changelog/26608.txt new file mode 100644 index 000000000..49d10e9b9 --- /dev/null +++ b/.changelog/26608.txt @@ -0,0 +1,3 @@ +```release-note:security +client: inspect artifacts for sandbox escape when landlock is unavailable +``` diff --git a/client/allocrunner/taskrunner/getter/params.go b/client/allocrunner/taskrunner/getter/params.go index 7fd60b349..2e7d6ea05 100644 --- a/client/allocrunner/taskrunner/getter/params.go +++ b/client/allocrunner/taskrunner/getter/params.go @@ -31,6 +31,7 @@ type parameters struct { S3Timeout time.Duration `json:"s3_timeout"` DecompressionLimitFileCount int `json:"decompression_limit_file_count"` DecompressionLimitSize int64 `json:"decompression_limit_size"` + DisableArtifactInspection bool `json:"disable_artifact_inspection"` DisableFilesystemIsolation bool `json:"disable_filesystem_isolation"` FilesystemIsolationExtraPaths []string `json:"filesystem_isolation_extra_paths"` SetEnvironmentVariables string `json:"set_environment_variables"` @@ -100,6 +101,8 @@ func (p *parameters) Equal(o *parameters) bool { return false case p.DecompressionLimitSize != o.DecompressionLimitSize: return false + case p.DisableArtifactInspection != o.DisableArtifactInspection: + return false case p.DisableFilesystemIsolation != o.DisableFilesystemIsolation: return false case !helper.SliceSetEq(p.FilesystemIsolationExtraPaths, o.FilesystemIsolationExtraPaths): diff --git a/client/allocrunner/taskrunner/getter/params_test.go b/client/allocrunner/taskrunner/getter/params_test.go index a7320dd5c..94432078a 100644 --- a/client/allocrunner/taskrunner/getter/params_test.go +++ b/client/allocrunner/taskrunner/getter/params_test.go @@ -24,6 +24,7 @@ const paramsAsJSON = ` "s3_timeout": 5000000000, "decompression_limit_file_count": 3, "decompression_limit_size": 98765, + "disable_artifact_inspection": false, "disable_filesystem_isolation": true, "filesystem_isolation_extra_paths": [ "f:r:/dev/urandom", diff --git a/client/allocrunner/taskrunner/getter/sandbox.go b/client/allocrunner/taskrunner/getter/sandbox.go index 2d6a1c1c3..2bc272424 100644 --- a/client/allocrunner/taskrunner/getter/sandbox.go +++ b/client/allocrunner/taskrunner/getter/sandbox.go @@ -52,6 +52,7 @@ func (s *Sandbox) Get(env interfaces.EnvReplacer, artifact *structs.TaskArtifact S3Timeout: s.ac.S3Timeout, DecompressionLimitFileCount: s.ac.DecompressionLimitFileCount, DecompressionLimitSize: s.ac.DecompressionLimitSize, + DisableArtifactInspection: s.ac.DisableArtifactInspection, DisableFilesystemIsolation: s.ac.DisableFilesystemIsolation, FilesystemIsolationExtraPaths: s.ac.FilesystemIsolationExtraPaths, SetEnvironmentVariables: s.ac.SetEnvironmentVariables, diff --git a/client/allocrunner/taskrunner/getter/sandbox_test.go b/client/allocrunner/taskrunner/getter/sandbox_test.go index 1b09118b7..63c7ee778 100644 --- a/client/allocrunner/taskrunner/getter/sandbox_test.go +++ b/client/allocrunner/taskrunner/getter/sandbox_test.go @@ -4,9 +4,12 @@ package getter import ( + "fmt" "net/http" + "net/http/cgi" "net/http/httptest" "os" + "os/exec" "path/filepath" "syscall" "testing" @@ -109,3 +112,126 @@ func TestSandbox_Get_chown(t *testing.T) { uid := info.Sys().(*syscall.Stat_t).Uid must.Eq(t, 65534, uid) // nobody's conventional uid } + +func TestSandbox_Get_inspection(t *testing.T) { + // These tests disable filesystem isolation as the + // artifact inspection is what is being tested. + testutil.RequireRoot(t) + logger := testlog.HCLogger(t) + + // Create a temporary directory directly so the repos + // don't end up being found improperly + tdir, err := os.MkdirTemp("", "nomad-test") + must.NoError(t, err, must.Sprint("failed to create top level local repo directory")) + + t.Run("symlink escaped sandbox", func(t *testing.T) { + dir, err := os.MkdirTemp(tdir, "fake-repo") + must.NoError(t, err, must.Sprint("failed to create local repo directory")) + must.NoError(t, os.Symlink("/", filepath.Join(dir, "bad-file")), must.Sprint("could not create symlink in local repo")) + srv := makeAndServeGitRepo(t, dir) + + artifact := &structs.TaskArtifact{ + RelativeDest: "local/symlink", + GetterSource: fmt.Sprintf("git::%s/%s", srv.URL, filepath.Base(dir)), + } + + t.Run("default", func(t *testing.T) { + ac := artifactConfig(10 * time.Second) + sbox := New(ac, logger) + + _, taskDir := SetupDir(t) + env := noopTaskEnv(taskDir) + sbox.ac.DisableFilesystemIsolation = true + + err := sbox.Get(env, artifact, "nobody") + must.ErrorIs(t, err, ErrSandboxEscape) + }) + + t.Run("DisableArtifactInspection", func(t *testing.T) { + ac := artifactConfig(10 * time.Second) + sbox := New(ac, logger) + + _, taskDir := SetupDir(t) + env := noopTaskEnv(taskDir) + sbox.ac.DisableFilesystemIsolation = true + sbox.ac.DisableArtifactInspection = true + + err := sbox.Get(env, artifact, "nobody") + must.NoError(t, err) + }) + }) + + t.Run("symlink within sandbox", func(t *testing.T) { + dir, err := os.MkdirTemp(tdir, "fake-repo") + must.NoError(t, err, must.Sprint("failed to create local repo")) + // create a file to link to + f, err := os.Create(filepath.Join(dir, "test-file")) + must.NoError(t, err, must.Sprint("could not create test file in local repo")) + f.Close() + // move into local repo to create relative link + wd, err := os.Getwd() + must.NoError(t, err, must.Sprint("cannot determine working directory")) + must.NoError(t, os.Chdir(dir)) + must.NoError(t, os.Symlink(filepath.Base(f.Name()), "good-file"), must.Sprint("could not create symlink in local repo")) + must.NoError(t, os.Chdir(wd)) + + // now serve the repo + srv := makeAndServeGitRepo(t, dir) + + artifact := &structs.TaskArtifact{ + RelativeDest: "local/symlink", + GetterSource: fmt.Sprintf("git::%s/%s", srv.URL, filepath.Base(dir)), + } + + ac := artifactConfig(10 * time.Second) + sbox := New(ac, logger) + + _, taskDir := SetupDir(t) + env := noopTaskEnv(taskDir) + sbox.ac.DisableFilesystemIsolation = true + + err = sbox.Get(env, artifact, "nobody") + must.NoError(t, err) + }) +} + +func makeAndServeGitRepo(t *testing.T, repoPath string) *httptest.Server { + t.Helper() + + wd, err := os.Getwd() + must.NoError(t, err, must.Sprint("could not determine working directory")) + must.NoError(t, os.Chdir(repoPath), must.Sprint("failed to change into repository directory")) + defer func() { must.NoError(t, os.Chdir(wd), must.Sprint("failed to return to working directory")) }() + + git, err := exec.LookPath("git") + must.NoError(t, err, must.Sprint("could not locate git executable")) + + cmd := exec.Command("git", "init", ".") + must.NoError(t, cmd.Run(), must.Sprint("cannot init git repository")) + + cmd = exec.Command("git", "config", "user.email", "user@example.com") + must.NoError(t, cmd.Run(), must.Sprint("cannot configure git repository")) + + cmd = exec.Command("git", "config", "user.name", "test user") + must.NoError(t, cmd.Run(), must.Sprint("cannot configure git repository")) + + cmd = exec.Command("git", "add", "--all") + must.NoError(t, cmd.Run(), must.Sprint("could not add files to git repository")) + + cmd = exec.Command("git", "commit", "-m", "test commit") + must.NoError(t, cmd.Run(), must.Sprint("cannot commit git repository content")) + + handler := &cgi.Handler{ + Path: git, + Args: []string{"http-backend"}, + Env: []string{ + "GIT_HTTP_EXPORT_ALL=true", + fmt.Sprintf("GIT_PROJECT_ROOT=%s", filepath.Dir(repoPath)), + }, + } + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + return srv +} diff --git a/client/allocrunner/taskrunner/getter/testing.go b/client/allocrunner/taskrunner/getter/testing.go index 8cde9da16..d65f440e3 100644 --- a/client/allocrunner/taskrunner/getter/testing.go +++ b/client/allocrunner/taskrunner/getter/testing.go @@ -33,6 +33,7 @@ func TestSandbox(t *testing.T) *Sandbox { func SetupDir(t *testing.T) (string, string) { allocDir := t.TempDir() taskDir := filepath.Join(allocDir, "local") + tmpDir := filepath.Join(taskDir, "tmp") topDir := filepath.Dir(allocDir) must.NoError(t, os.Chmod(topDir, 0o755)) @@ -41,5 +42,8 @@ func SetupDir(t *testing.T) (string, string) { must.NoError(t, os.Mkdir(taskDir, 0o755)) must.NoError(t, os.Chmod(taskDir, 0o755)) + must.NoError(t, os.Mkdir(tmpDir, 0o755)) + must.NoError(t, os.Chmod(tmpDir, 0o755)) + return allocDir, taskDir } diff --git a/client/allocrunner/taskrunner/getter/util.go b/client/allocrunner/taskrunner/getter/util.go index a2a0cd255..4e7f4ca1a 100644 --- a/client/allocrunner/taskrunner/getter/util.go +++ b/client/allocrunner/taskrunner/getter/util.go @@ -5,7 +5,9 @@ package getter import ( "bytes" + "errors" "fmt" + "io/fs" "net/http" "net/url" "os" @@ -28,6 +30,8 @@ const ( githubPrefixSSH = "git@github.com:" ) +var ErrSandboxEscape = errors.New("artifact includes symlink that resolves outside of sandbox") + func getURL(taskEnv interfaces.EnvReplacer, artifact *structs.TaskArtifact) (string, error) { source := taskEnv.ReplaceEnv(artifact.GetterSource) @@ -184,5 +188,123 @@ func (s *Sandbox) runCmd(env *parameters) error { } } subproc.Log(output, s.logger.Debug) + + // if filesystem isolation was not disabled and lockdown + // is available on this platform, do not continue to inspection + if !env.DisableFilesystemIsolation && lockdownAvailable() { + return nil + } + + // if artifact inspection is disabled, do not continue to inspection + if env.DisableArtifactInspection { + return nil + } + + // inspect the writable directories. start with inspecting the + // alloc directory + allocInspector, err := genWalkInspector(env.AllocDir) + if err != nil { + return err + } + + if err := filepath.WalkDir(env.AllocDir, allocInspector); err != nil { + return err + } + + // the task directory is within the alloc directory. however, if + // that ever changes for some reason, make sure it is checked as well + isWithin, err := isPathWithin(env.AllocDir, env.TaskDir) + if err != nil { + return err + } + + if !isWithin { + taskInspector, err := genWalkInspector(env.TaskDir) + if err != nil { + return err + } + + if err := filepath.WalkDir(env.TaskDir, taskInspector); err != nil { + return err + } + } + return nil } + +// generateWalkInspector creates a walk function to check for symlinks +// that resolve outside of the rootDir. +func genWalkInspector(rootDir string) (fs.WalkDirFunc, error) { + rootDir, err := filepath.Abs(rootDir) + if err != nil { + return nil, err + } + + var walkFn fs.WalkDirFunc + + walkFn = func(path string, entry fs.DirEntry, err error) error { + // argument error means an error was encountered reading + // a directory or getting file info so stop here + if err != nil { + return err + } + + info, err := entry.Info() + if err != nil { + return err + } + + // Only care about symlinks + if info.Mode()&fs.ModeSymlink != fs.ModeSymlink { + return nil + } + + // Build up the actual path + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return err + } + + toCheck, err := filepath.Abs(resolved) + if err != nil { + return err + } + + // Check that entry is still within sandbox + isWithin, err := isPathWithin(rootDir, toCheck) + if err != nil { + return err + } + + if !isWithin { + return ErrSandboxEscape + } + + return nil + } + return walkFn, nil +} + +// isPathWithin checks if the toCheckPath is within the rootPath. It +// uses the os.SameFile function to perform the path check so paths +// are compared appropriately based on the filesystem. +func isPathWithin(rootPath, toCheckPath string) (bool, error) { + rootPath = filepath.Clean(rootPath) + toCheckPath = filepath.Clean(toCheckPath) + + if len(rootPath) > len(toCheckPath) { + return false, nil + } + + rootStat, err := os.Stat(rootPath) + if err != nil { + return false, err + } + + checkStat, err := os.Stat(toCheckPath[0:len(rootPath)]) + if err != nil { + return false, err + } + + return os.SameFile(rootStat, checkStat), nil +} diff --git a/client/allocrunner/taskrunner/getter/util_default.go b/client/allocrunner/taskrunner/getter/util_default.go index 8dbdde4f2..f5247da41 100644 --- a/client/allocrunner/taskrunner/getter/util_default.go +++ b/client/allocrunner/taskrunner/getter/util_default.go @@ -9,6 +9,11 @@ import ( "path/filepath" ) +// lockdown is not available by default +func lockdownAvailable() bool { + return false +} + // lockdown is not implemented by default func lockdown(string, string, []string) error { return nil diff --git a/client/allocrunner/taskrunner/getter/util_linux.go b/client/allocrunner/taskrunner/getter/util_linux.go index 6934f81f7..b0f97951b 100644 --- a/client/allocrunner/taskrunner/getter/util_linux.go +++ b/client/allocrunner/taskrunner/getter/util_linux.go @@ -46,6 +46,12 @@ func defaultEnvironment(taskDir string) map[string]string { } } +// lockdownAvailable returns if lockdown is implemented for +// the current platform. +func lockdownAvailable() bool { + return landlock.Available() +} + // lockdown isolates this process to only be able to write and // create files in the task's task directory. // dir - the task directory diff --git a/client/allocrunner/taskrunner/getter/util_test.go b/client/allocrunner/taskrunner/getter/util_test.go index 978eea690..925770eef 100644 --- a/client/allocrunner/taskrunner/getter/util_test.go +++ b/client/allocrunner/taskrunner/getter/util_test.go @@ -6,6 +6,8 @@ package getter import ( "errors" "fmt" + "os" + "path/filepath" "testing" "github.com/hashicorp/go-getter" @@ -240,3 +242,48 @@ func TestUtil_environment(t *testing.T) { }, result) }) } + +func TestUtil_isPathWithin(t *testing.T) { + tdir := t.TempDir() + pathFn := func(parent string) string { + dir, err := os.MkdirTemp(parent, "testing-path") + must.NoError(t, err, must.Sprint("failed to create temporary directory")) + return dir + } + + t.Run("when path not within root", func(t *testing.T) { + root := pathFn(tdir) + check := pathFn(tdir) + result, err := isPathWithin(root, check) + + must.NoError(t, err) + must.False(t, result) + }) + + t.Run("when path within root", func(t *testing.T) { + root := pathFn(tdir) + check := pathFn(root) + result, err := isPathWithin(root, check) + + must.NoError(t, err) + must.True(t, result) + }) + + t.Run("when root within path", func(t *testing.T) { + check := pathFn(tdir) + root := pathFn(check) + result, err := isPathWithin(root, check) + + must.NoError(t, err) + must.False(t, result) + }) + + t.Run("when path does not exist", func(t *testing.T) { + root := filepath.Join(tdir, "missing") + check := filepath.Join(root, "unknown") + result, err := isPathWithin(root, check) + + must.ErrorContains(t, err, "no such file or directory") + must.False(t, result) + }) +} diff --git a/client/allocrunner/taskrunner/getter/util_windows.go b/client/allocrunner/taskrunner/getter/util_windows.go index b171e6134..d9384120d 100644 --- a/client/allocrunner/taskrunner/getter/util_windows.go +++ b/client/allocrunner/taskrunner/getter/util_windows.go @@ -10,6 +10,11 @@ import ( "path/filepath" ) +// lockdown is not implemented on Windows +func lockdownAvailable() bool { + return false +} + // lockdown is not implemented on Windows func lockdown(string, string, []string) error { return nil diff --git a/client/config/artifact.go b/client/config/artifact.go index aeb6de6ef..4159b879c 100644 --- a/client/config/artifact.go +++ b/client/config/artifact.go @@ -26,6 +26,7 @@ type ArtifactConfig struct { DecompressionLimitFileCount int DecompressionLimitSize int64 + DisableArtifactInspection bool DisableFilesystemIsolation bool FilesystemIsolationExtraPaths []string SetEnvironmentVariables string @@ -78,6 +79,7 @@ func ArtifactConfigFromAgent(c *config.ArtifactConfig) (*ArtifactConfig, error) S3Timeout: s3Timeout, DecompressionLimitFileCount: *c.DecompressionFileCountLimit, DecompressionLimitSize: int64(decompressionSizeLimit), + DisableArtifactInspection: *c.DisableArtifactInspection, DisableFilesystemIsolation: *c.DisableFilesystemIsolation, FilesystemIsolationExtraPaths: slices.Clone(c.FilesystemIsolationExtraPaths), SetEnvironmentVariables: *c.SetEnvironmentVariables, diff --git a/nomad/structs/config/artifact.go b/nomad/structs/config/artifact.go index b10096722..21f3c1123 100644 --- a/nomad/structs/config/artifact.go +++ b/nomad/structs/config/artifact.go @@ -53,6 +53,12 @@ type ArtifactConfig struct { // Default is 100GB. DecompressionSizeLimit *string `hcl:"decompression_size_limit"` + // DisableArtifactInspection will turn off artifact inspection which checks + // the artifact contents for potential sandbox escapes. If the platform supports + // filesystem isolation, and it is not disabled, the check will not be run + // regardless of this value. + DisableArtifactInspection *bool `hcl:"disable_artifact_inspection"` + // DisableFilesystemIsolation will turn off the security feature where the // artifact downloader can write only to the task sandbox directory, and can // read only from specific locations on the host filesystem. @@ -81,6 +87,7 @@ func (a *ArtifactConfig) Copy() *ArtifactConfig { S3Timeout: pointer.Copy(a.S3Timeout), DecompressionFileCountLimit: pointer.Copy(a.DecompressionFileCountLimit), DecompressionSizeLimit: pointer.Copy(a.DecompressionSizeLimit), + DisableArtifactInspection: pointer.Copy(a.DisableArtifactInspection), DisableFilesystemIsolation: pointer.Copy(a.DisableFilesystemIsolation), FilesystemIsolationExtraPaths: slices.Clone(a.FilesystemIsolationExtraPaths), SetEnvironmentVariables: pointer.Copy(a.SetEnvironmentVariables), @@ -103,6 +110,7 @@ func (a *ArtifactConfig) Merge(o *ArtifactConfig) *ArtifactConfig { S3Timeout: pointer.Merge(a.S3Timeout, o.S3Timeout), DecompressionFileCountLimit: pointer.Merge(a.DecompressionFileCountLimit, o.DecompressionFileCountLimit), DecompressionSizeLimit: pointer.Merge(a.DecompressionSizeLimit, o.DecompressionSizeLimit), + DisableArtifactInspection: pointer.Merge(a.DisableArtifactInspection, o.DisableArtifactInspection), DisableFilesystemIsolation: pointer.Merge(a.DisableFilesystemIsolation, o.DisableFilesystemIsolation), SetEnvironmentVariables: pointer.Merge(a.SetEnvironmentVariables, o.SetEnvironmentVariables), } @@ -138,6 +146,8 @@ func (a *ArtifactConfig) Equal(o *ArtifactConfig) bool { return false case !pointer.Eq(a.DecompressionSizeLimit, o.DecompressionSizeLimit): return false + case !pointer.Eq(a.DisableArtifactInspection, o.DisableArtifactInspection): + return false case !pointer.Eq(a.DisableFilesystemIsolation, o.DisableFilesystemIsolation): return false case !helper.SliceSetEq(a.FilesystemIsolationExtraPaths, o.FilesystemIsolationExtraPaths): @@ -223,6 +233,10 @@ func (a *ArtifactConfig) Validate() error { return fmt.Errorf("decompression_size_limit must be < %d but found %d", int64(math.MaxInt64), v) } + if a.DisableArtifactInspection == nil { + return fmt.Errorf("disable_artifact_inspection must be set") + } + if a.DisableFilesystemIsolation == nil { return fmt.Errorf("disable_filesystem_isolation must be set") } @@ -275,6 +289,9 @@ func DefaultArtifactConfig() *ArtifactConfig { // a single artifact. Must be large enough to accommodate large payloads. DecompressionSizeLimit: pointer.Of("100GB"), + // Toggle for disabling artifact inspection + DisableArtifactInspection: pointer.Of(false), + // Toggle for disabling filesystem isolation, where available. DisableFilesystemIsolation: pointer.Of(false), diff --git a/website/content/docs/configuration/client.mdx b/website/content/docs/configuration/client.mdx index a41b0c101..030416fc6 100644 --- a/website/content/docs/configuration/client.mdx +++ b/website/content/docs/configuration/client.mdx @@ -488,6 +488,11 @@ refer to the [drivers documentation](/nomad/docs/job-declare/task-driver). of files that will be decompressed before triggering an error and cancelling the operation. Set to `0` to not enforce a limit. +- `disable_artifact_inspection` `(bool: false)` - Specifies whether to disable + artifact inspection for sandbox escapes. If the platform supports filesystem + isolation, and it is not disabled, artifact inspection will not be performed + regardless of this value. + - `disable_filesystem_isolation` `(bool: false)` - Specifies whether filesystem isolation should be disabled for artifact downloads. Applies only to systems where filesystem isolation via [landlock] is possible (Linux kernel 5.13+).