[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

@@ -0,0 +1,3 @@
+```release-note:security
+client: inspect artifacts for sandbox escape when landlock is unavailable
+```

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):

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",

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,

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
+}

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
 }

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
+}

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

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

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

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

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,

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),
 

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