Skip to content

Replace process-wide os.Chdir in pack/unpack/import with an explicit base path #1269

Description

@gorkem

Describe the problem you're trying to solve

Pack, unpack, and import each call os.Chdir and then rely on the process working directory as an implicit "resolve relative paths against this" parameter. os.Chdir is process-global, not goroutine- or call-scoped, so this makes the packing and unpacking internals unsafe to call from any long-lived or concurrent process, and it quietly makes the current working directory part of a security check.

The root cause is that a single string is used for two different jobs. In writeLayerToTar (pkg/lib/filesystem/tar.go:205) the walk is rooted at filepath.Walk(".") — literally the process cwd — and each resulting path is then used both as a filesystem path to read (os.Open(file), tar.go:279) and as the archive entry name (header.Name = name, tar.go:269). Walking from . while chdir'd into the context directory is what makes those entries correctly relative. The same trick runs in reverse on extract: extractTar sets outPath := header.Name and only joins a base when one is non-empty (unpack/core.go:394-396), and per the comment at core.go:360-362 the base is empty for every ModelKit created since v0.5.0. So modern artifacts extract relative to whatever the cwd happens to be.

This is not cruft. It dates to the original kit buildkit pack rename (f3b4425, March 2024) and both call sites document the intent — pack's comment calls it "the equivalent of using the -C parameter for tar". Relative entry names are a real requirement of the artifact format: absolute host paths must not leak into a ModelKit, and they are an extraction hazard. When kit was strictly a single-shot CLI, process-global state was effectively function-local and chdir was a cheap, correct way to get tar -C semantics. That assumption no longer holds.

Concrete consequences today:

  • The internals cannot be called concurrently in one process. Two goroutines packing or unpacking will move the cwd out from under each other, and the deferred restore in unpack (core.go:70) and import (kitimport/util.go:96) can fire while another operation is mid-flight. pack never restores it at all (pkg/cmd/pack/cmd.go:101), which is fine only because the CLI exits immediately after.
  • The cwd is load-bearing for path-traversal containment. extractTar (core.go:399) and extractRawLayer (core.go:460) call filesystem.VerifySubpath("", …), and VerifySubpath resolves an empty context via filepath.Abs("") (pkg/lib/filesystem/paths.go:43), which is the cwd. Anyone removing the chdir without fixing this silently reanchors the containment check to the wrong directory.
  • It has already forced unrelated workarounds. Import restores the cwd specifically because Windows will not delete a temp directory that is some process's working directory (kitimport/util.go:85-99).
  • It is a latent trap for callers. opts.UnpackDir is reused at core.go:112, :197 and :267 after the chdir into that same directory. Both current callers pass an absolute path (pkg/cmd/unpack/cmd.go:149-153, and dev via constants.ExtractedDevModelPath), so it is correct today, but a library caller passing a relative directory would resolve to <orig>/out/out/….

Describe the solution you'd like

Make the base directory an explicit parameter and delete all three chdir sites.

Pack side — walk the absolute context directory instead of ., then split the overloaded string: filepath.Rel(contextDir, file) for header.Name, the absolute path for os.Open. os.Stat(basePath), sameDirTree, and ignorePaths.Matches (tar.go:183, :213, :238) need the same treatment. The plumbing is shallow — saveContentLayersaveContentLayerAsTar/saveContentLayerAsRawpackLayerToTarwriteLayerToTar — and SaveModelOptions (local-storage.go:43-47) is already a struct, so a ContextDir field there avoids a signature cascade through SaveModel and saveKitfileLayers.

Unpack side — pass a real base to extractTar and extractRawLayer and retire the extractDir == "" convention, giving VerifySubpath a genuine context instead of the empty string. Then drop the chdir blocks at core.go:66 and :278, pack/cmd.go:101, and kitimport/util.go:92-99.

Suggested as two PRs:

  1. Make the base path explicit, preserving all current behavior.
  2. Convert the file operations to *os.Root (os.OpenRoot). The repo is on go 1.25.11, so the full method set is available, and the standard library documents Root as safe for simultaneous use from multiple goroutines. This gets kernel-enforced containment via the *at syscalls, removes the TOCTOU window in VerifySubpath's check-then-act (EvalSymlinks at paths.go:48 and :58, then the caller acts), and largely dissolves VerifySubpath itself.

Splitting them keeps the behavior-preserving change reviewable on its own and leaves the semantic change — os.Root rejects absolute symlinks and out-of-root links outright, which could reject artifacts that extract today — as a separate, deliberate decision.

Behavior for end users should not change. This is an internal refactor.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions