diff --git a/TODO.md b/TODO.md index c8cb9f2..50eb1ee 100644 --- a/TODO.md +++ b/TODO.md @@ -24,6 +24,10 @@ only thing left of the `chore/align-repo-policies` branch is the list below. # Completed Steps +- 2026-10-03: `fetch` refuses any manifest path that runs through a symlink + already in the destination directory, checked before each of its writes + (directories, temp file, rename), so such a symlink cannot send a write + outside it (#86) - 2026-10-02: a plain `docker build .` of a clone now stamps the tag or short commit into `mfer version` instead of nothing: `.dockerignore` sends `.git` (not `.git/config`), and the build stage takes the `VERSION` build argument, diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 8254a4c..7983e97 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -53,6 +53,9 @@ var ( // errPathTraversal indicates a manifest path escaping the target // directory. errPathTraversal = errors.New("path traversal not allowed") + // errSymlinkInPath indicates a manifest path running through a + // symlink that already exists in the target directory. + errSymlinkInPath = errors.New("symlink in path not allowed") // errSizeMismatch indicates a downloaded file with an unexpected // size. errSizeMismatch = errors.New("size mismatch") @@ -274,6 +277,35 @@ func sanitizePath(p string) (string, error) { return cleaned, nil } +// checkNoSymlinks returns an error if any part of the relative path p +// already exists as a symlink. sanitizePath checks p only as text, so +// without this a symlink inside the target directory could send a write +// to p outside of it. Parts that do not exist yet are fine: fetch creates +// them as plain directories and files. Call it immediately before each +// write: a symlink created after it returns is not caught. +func checkNoSymlinks(p string) error { + current := "" + + for _, part := range strings.Split(p, string(filepath.Separator)) { + current = filepath.Join(current, part) + + info, err := os.Lstat(current) + if errors.Is(err, os.ErrNotExist) { + return nil + } + + if err != nil { + return fmt.Errorf("failed to check %s for a symlink: %w", current, err) + } + + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("%w: %s", errSymlinkInPath, current) + } + } + + return nil +} + // resolveManifestURL takes a URL and returns the manifest URL. // If the URL already ends with .mf, it's returned as-is. // Otherwise, index.mf is appended. @@ -419,7 +451,12 @@ func downloadFile( // Create parent directories if needed dir := filepath.Dir(localPath) if dir != "" && dir != "." { - err := os.MkdirAll(dir, dirPerms) + err = checkNoSymlinks(dir) + if err != nil { + return err + } + + err = os.MkdirAll(dir, dirPerms) if err != nil { return fmt.Errorf("failed to create directory %s: %w", dir, err) } @@ -447,14 +484,16 @@ func downloadFile( totalBytes = expectedSize } + err = checkNoSymlinks(tmpPath) + if err != nil { + return err + } + // Create temp file. // - // G304: tmpPath is derived from localPath, which sanitizePath above - // constrains lexically to a relative path that does not escape the - // destination directory. That is a purely lexical guarantee: it does - // not resolve symlinks, so a pre-existing symlink inside the - // destination tree can still redirect this write outside of it - // (tracked in issue #86). + // G304: tmpPath is a relative path that sanitizePath keeps inside the + // target directory as text, and checkNoSymlinks just found no symlink + // in it. out, err := os.Create(tmpPath) //nolint:gosec // G304: see comment above if err != nil { return fmt.Errorf("failed to create temp file: %w", err) @@ -519,6 +558,11 @@ func finishDownload( return err } + err = checkNoSymlinks(localPath) + if err != nil { + return err + } + // Rename temp file to final path err = os.Rename(tmpPath, localPath) if err != nil { diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index dd7a3e8..6aaa82d 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -440,3 +440,52 @@ func TestFetchProgress(t *testing.T) { require.NoError(t, err) assert.Equal(t, content, downloaded) } + +// TestFetchRefusesSymlinks runs fetch into a destination directory that +// holds a symlink pointing outside it, in each of the three places fetch +// writes: a parent directory, the temp file, and the file itself, which +// the temp file is renamed onto; and once as a directory inside a plain +// directory. The fetch must fail and nothing outside may change. +// +//nolint:paralleltest // changes the process-global working directory +func TestFetchRefusesSymlinks(t *testing.T) { + tests := []struct { + name string + entry string // the manifest's only file + link string // symlink placed in the destination directory + target string // what link points to, relative to the outside directory + }{ + {"parent directory", "sub/deeper/file.txt", "sub", "."}, + {"directory inside a plain directory", "docs/data/passwd", "docs/data", "."}, + {"temp file", testFileTxt, ".file.txt.tmp", "new.txt"}, + {"file", testFileTxt, testFileTxt, "new.txt"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + content := []byte("fetched") + sourceFs := afero.NewMemMapFs() + require.NoError(t, sourceFs.MkdirAll(filepath.Dir("/"+tt.entry), 0o755)) + require.NoError(t, afero.WriteFile(sourceFs, "/"+tt.entry, content, 0o644)) + + server := httptest.NewServer(fetchTestHandler( + scanToManifest(t, sourceFs), map[string][]byte{tt.entry: content})) + defer server.Close() + + outside := t.TempDir() + + chdirTemp(t) + require.NoError(t, os.MkdirAll(filepath.Dir(tt.link), 0o750)) + require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), tt.link)) + + opts := testOpts([]string{testApp, "fetch", "-q", server.URL}, afero.NewOsFs()) + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), "failed to download "+tt.entry+ + ": symlink in path not allowed: "+tt.link) + + written, err := os.ReadDir(outside) + require.NoError(t, err) + assert.Empty(t, written, "fetch wrote outside the destination") + }) + } +}