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+).