From a37339d63233633d807e57fe5ce32d6990413559 Mon Sep 17 00:00:00 2001 From: Sebastian Florek Date: Tue, 21 Jul 2026 16:33:17 +0200 Subject: [PATCH 1/2] fix(Untar): ensure files are extracted inside the destination directory - Verify target paths to prevent extraction outside the destination - Add unit tests for validation of target path and content extraction within destination directory --- pkg/utils/tar.go | 11 ++++++++- pkg/utils/tar_test.go | 56 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 pkg/utils/tar_test.go diff --git a/pkg/utils/tar.go b/pkg/utils/tar.go index 5286074c3..9d96294e0 100644 --- a/pkg/utils/tar.go +++ b/pkg/utils/tar.go @@ -6,10 +6,16 @@ import ( "io" "os" "path/filepath" + "strings" "sync" ) func Untar(dst string, r io.Reader) error { + root, err := filepath.Abs(dst) + if err != nil { + return err + } + tr := tar.NewReader(r) madeDir := map[string]bool{} for { @@ -29,7 +35,10 @@ func Untar(dst string, r io.Reader) error { } // the target location where the dir/file should be created - target := filepath.Join(dst, header.Name) + target := filepath.Join(root, header.Name) + if target != root && !strings.HasPrefix(target, root+string(filepath.Separator)) { + return fmt.Errorf("tar entry %q resolves outside destination", header.Name) + } // the following switch could also be done using fi.Mode(), not sure if there // a benefit of using one vs. the other. // fi := header.FileInfo() diff --git a/pkg/utils/tar_test.go b/pkg/utils/tar_test.go new file mode 100644 index 000000000..459350b89 --- /dev/null +++ b/pkg/utils/tar_test.go @@ -0,0 +1,56 @@ +package utils_test + +import ( + "archive/tar" + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/pluralsh/plural-cli/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestUntar(t *testing.T) { + t.Run("extracts files inside destination", func(t *testing.T) { + archive := createTar(t, "nested/file.txt", "content") + dst := t.TempDir() + + require.NoError(t, utils.Untar(dst, archive)) + + content, err := os.ReadFile(filepath.Join(dst, "nested", "file.txt")) + require.NoError(t, err) + assert.Equal(t, "content", string(content)) + }) + + t.Run("rejects files outside destination", func(t *testing.T) { + archive := createTar(t, "../outside.txt", "content") + parent := t.TempDir() + dst := filepath.Join(parent, "destination") + + err := utils.Untar(dst, archive) + + require.Error(t, err) + assert.ErrorContains(t, err, "resolves outside destination") + _, err = os.Stat(filepath.Join(parent, "outside.txt")) + assert.ErrorIs(t, err, os.ErrNotExist) + }) +} + +func createTar(t *testing.T, name, content string) *bytes.Reader { + t.Helper() + + var archive bytes.Buffer + w := tar.NewWriter(&archive) + require.NoError(t, w.WriteHeader(&tar.Header{ + Name: name, + Mode: 0o600, + Size: int64(len(content)), + })) + _, err := w.Write([]byte(content)) + require.NoError(t, err) + require.NoError(t, w.Close()) + + return bytes.NewReader(archive.Bytes()) +} From 5c849f55941fc891730c0b8d96454e49502dd429 Mon Sep 17 00:00:00 2001 From: Sebastian Florek Date: Tue, 21 Jul 2026 16:55:05 +0200 Subject: [PATCH 2/2] fix(Untar): enhance path security and validation - Use `os.OpenRoot` and consolidate directory creation - Check local path using `filepath.IsLocal` - Refactor `makeDir` to use `root.MkdirAll` - Improve error messaging for file extraction - Add test to reject symlinks resolving outside the destination --- pkg/utils/tar.go | 33 +++++++++++++++++++-------------- pkg/utils/tar_test.go | 13 +++++++++++++ 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/pkg/utils/tar.go b/pkg/utils/tar.go index 9d96294e0..02d6cbd00 100644 --- a/pkg/utils/tar.go +++ b/pkg/utils/tar.go @@ -6,7 +6,6 @@ import ( "io" "os" "path/filepath" - "strings" "sync" ) @@ -15,6 +14,14 @@ func Untar(dst string, r io.Reader) error { if err != nil { return err } + if err := os.MkdirAll(root, 0755); err != nil { + return err + } + rootFS, err := os.OpenRoot(root) + if err != nil { + return err + } + defer rootFS.Close() tr := tar.NewReader(r) madeDir := map[string]bool{} @@ -34,11 +41,12 @@ func Untar(dst string, r io.Reader) error { continue } - // the target location where the dir/file should be created - target := filepath.Join(root, header.Name) - if target != root && !strings.HasPrefix(target, root+string(filepath.Separator)) { + if !filepath.IsLocal(header.Name) { return fmt.Errorf("tar entry %q resolves outside destination", header.Name) } + + // the target location where the dir/file should be created + target := filepath.Clean(header.Name) // the following switch could also be done using fi.Mode(), not sure if there // a benefit of using one vs. the other. // fi := header.FileInfo() @@ -47,20 +55,19 @@ func Untar(dst string, r io.Reader) error { switch header.Typeflag { // if its a dir and it doesn't exist create it case tar.TypeDir: - if err := makeDir(target, madeDir); err != nil { + if err := makeDir(rootFS, target, madeDir); err != nil { return err } // if it's a file create it case tar.TypeReg: - if err := makeDir(filepath.Dir(target), madeDir); err != nil { + if err := makeDir(rootFS, filepath.Dir(target), madeDir); err != nil { return err } - f, err := os.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) + f, err := rootFS.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) if err != nil { - fmt.Println("could not open file") - return err + return fmt.Errorf("extract tar entry %q: %w", header.Name, err) } // copy over contents @@ -75,15 +82,13 @@ func Untar(dst string, r io.Reader) error { } } -func makeDir(target string, made map[string]bool) error { +func makeDir(root *os.Root, target string, made map[string]bool) error { if made[target] { return nil } - if _, err := os.Stat(target); err != nil { - if err := os.MkdirAll(target, 0755); err != nil { - return err - } + if err := root.MkdirAll(target, 0755); err != nil { + return err } made[target] = true diff --git a/pkg/utils/tar_test.go b/pkg/utils/tar_test.go index 459350b89..8faf272f9 100644 --- a/pkg/utils/tar_test.go +++ b/pkg/utils/tar_test.go @@ -36,6 +36,19 @@ func TestUntar(t *testing.T) { _, err = os.Stat(filepath.Join(parent, "outside.txt")) assert.ErrorIs(t, err, os.ErrNotExist) }) + + t.Run("rejects symlinks outside destination", func(t *testing.T) { + archive := createTar(t, "link/file.txt", "content") + dst := t.TempDir() + outside := t.TempDir() + require.NoError(t, os.Symlink(outside, filepath.Join(dst, "link"))) + + err := utils.Untar(dst, archive) + + require.Error(t, err) + _, err = os.Stat(filepath.Join(outside, "file.txt")) + assert.ErrorIs(t, err, os.ErrNotExist) + }) } func createTar(t *testing.T, name, content string) *bytes.Reader {