Skip to content

Use os.Root for copy operations in vminitd#168

Open
dmcgowan wants to merge 1 commit intocontainerd:mainfrom
dmcgowan:use-os-root
Open

Use os.Root for copy operations in vminitd#168
dmcgowan wants to merge 1 commit intocontainerd:mainfrom
dmcgowan:use-os-root

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan commented May 1, 2026

Updates the transfer in vminitd to use os.Root rather than path resolution into the destination path

Copilot AI review requested due to automatic review settings May 1, 2026 20:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the container filesystem transfer logic in vminitd to anchor copy/export/import operations using os.Root, aiming to prevent path traversal and symlink-escape issues by avoiding destination-path resolution.

Changes:

  • Introduces rootRel and migrates writePath to use os.OpenRoot + root.FS() walking and *os.Root file operations.
  • Migrates readPath extraction to enforce destination boundaries via a sub-*os.Root, removing prior lexical prefix checks.
  • Adds comprehensive tests covering tar path traversal, symlink/hardlink attacks, and round-trip export/import behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
internal/transfer/containerfs.go Switches export/import implementation to *os.Root-anchored operations and adds rootRel helper.
internal/transfer/containerfs_test.go Adds security- and correctness-focused tests for export/import behavior (symlinks, traversal, round-trips).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/transfer/containerfs_test.go Outdated
Comment thread internal/transfer/containerfs_test.go Outdated
Comment thread internal/transfer/containerfs.go
Comment thread internal/transfer/containerfs.go
Comment thread internal/transfer/containerfs_test.go Outdated
Copilot AI review requested due to automatic review settings May 1, 2026 21:32
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/transfer/containerfs_test.go
Comment thread internal/transfer/containerfs.go
Signed-off-by: Derek McGowan <derek@mcg.dev>
// walkPath is always slash-separated (fs.FS contract) and
// rooted at relPath. Strip the prefix to get the entry's
// path within the walk.
rel := strings.TrimPrefix(strings.TrimPrefix(walkPath, relPath), "/")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential issue for dotfiles at rootfs root:

// TestWritePathExportRootDotfilesPreserved verifies that files whose
// names begin with "." (dotfiles) at the rootfs root are exported with
// their full name intact when src is "/". The double-TrimPrefix in the
// walk loop incorrectly strips the leading "." because relPath is also
// "." — exposing this regression.
func TestWritePathExportRootDotfilesPreserved(t *testing.T) {
	_, rootfs, _ := makeRootfs(t)

	if err := os.WriteFile(filepath.Join(rootfs, ".bashrc"), []byte("rc"), 0644); err != nil {
		t.Fatal(err)
	}
	if err := os.WriteFile(filepath.Join(rootfs, "plain"), []byte("plain"), 0644); err != nil {
		t.Fatal(err)
	}

	buf := &bytes.Buffer{}
	if err := writePath(rootfs, "/", buf, mediaTypeTar, false); err != nil {
		t.Fatalf("writePath: %v", err)
	}

	entries := readTar(t, buf)
	if _, ok := entries[".bashrc"]; !ok {
		t.Errorf("dotfile '.bashrc' missing from tar; got entries: %v", keys(entries))
	}
	if _, ok := entries["bashrc"]; ok {
		t.Errorf("dotfile was renamed to 'bashrc' (leading dot stripped by TrimPrefix bug)")
	}
}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants