From 2abf6c4f03e8d25f451893105723f33e15c1afa7 Mon Sep 17 00:00:00 2001 From: fsabiu Date: Sun, 3 May 2026 21:39:58 +0200 Subject: [PATCH] Constrain tar driver paths to image root --- internal/pkgutil/root_path.go | 92 +++++++++++++++ internal/pkgutil/tar_utils.go | 57 +++++++-- internal/pkgutil/tar_utils_test.go | 178 +++++++++++++++++++++++++++++ pkg/drivers/tar_driver.go | 25 ++-- pkg/drivers/tar_driver_test.go | 122 ++++++++++++++++++++ 5 files changed, 459 insertions(+), 15 deletions(-) create mode 100644 internal/pkgutil/root_path.go create mode 100644 internal/pkgutil/tar_utils_test.go create mode 100644 pkg/drivers/tar_driver_test.go diff --git a/internal/pkgutil/root_path.go b/internal/pkgutil/root_path.go new file mode 100644 index 00000000..6b917c31 --- /dev/null +++ b/internal/pkgutil/root_path.go @@ -0,0 +1,92 @@ +package pkgutil + +import ( + "fmt" + "os" + pathpkg "path" + "path/filepath" + "strings" +) + +const maxRootPathSymlinks = 255 + +// ResolvePathInRoot resolves imagePath below root. Absolute paths are treated as +// absolute within root, and symlink targets are resolved using the same image +// root instead of the host root. +func ResolvePathInRoot(root, imagePath string, followFinalSymlink bool) (string, error) { + absRoot, err := filepath.Abs(root) + if err != nil { + return "", err + } + absRoot = filepath.Clean(absRoot) + + parts, err := resolveRootPathParts(nil, imagePath) + if err != nil { + return "", err + } + + resolved := []string{} + symlinkCount := 0 + for i := 0; i < len(parts); i++ { + part := parts[i] + candidate := joinRootPath(absRoot, append(resolved, part)) + isFinal := i == len(parts)-1 + if !isFinal || followFinalSymlink { + info, err := os.Lstat(candidate) + if err != nil && !os.IsNotExist(err) { + return "", err + } + if err == nil && info.Mode()&os.ModeSymlink != 0 { + symlinkCount++ + if symlinkCount > maxRootPathSymlinks { + return "", fmt.Errorf("too many symlinks resolving path %q", imagePath) + } + linkTarget, err := os.Readlink(candidate) + if err != nil { + return "", err + } + base := resolved + if pathpkg.IsAbs(filepath.ToSlash(linkTarget)) { + base = nil + } + linkParts, err := resolveRootPathParts(base, linkTarget) + if err != nil { + return "", err + } + parts = append(linkParts, parts[i+1:]...) + resolved = nil + i = -1 + continue + } + } + resolved = append(resolved, part) + } + return joinRootPath(absRoot, resolved), nil +} + +func resolveRootPathParts(base []string, imagePath string) ([]string, error) { + cleaned := filepath.ToSlash(imagePath) + if pathpkg.IsAbs(cleaned) { + base = nil + } + + parts := append([]string{}, base...) + for _, part := range strings.Split(pathpkg.Clean(cleaned), "/") { + switch part { + case "", ".": + continue + case "..": + if len(parts) == 0 { + return nil, fmt.Errorf("path %q escapes root", imagePath) + } + parts = parts[:len(parts)-1] + default: + parts = append(parts, part) + } + } + return parts, nil +} + +func joinRootPath(root string, parts []string) string { + return filepath.Join(append([]string{root}, parts...)...) +} diff --git a/internal/pkgutil/tar_utils.go b/internal/pkgutil/tar_utils.go index 9d0075cb..943d78c3 100644 --- a/internal/pkgutil/tar_utils.go +++ b/internal/pkgutil/tar_utils.go @@ -21,6 +21,7 @@ import ( "fmt" "io" "os" + pathpkg "path" "path/filepath" "strings" @@ -50,7 +51,10 @@ func unpackTar(tr *tar.Reader, path string, whitelist []string) error { if err != nil { return errors.Wrap(err, "Error getting next tar header") } - target := filepath.Clean(filepath.Join(path, header.Name)) + target, err := ResolvePathInRoot(path, header.Name, false) + if err != nil { + return errors.Wrap(err, "resolving tar entry path") + } // Make sure the target isn't part of the whitelist if checkWhitelist(target, whitelist) { continue @@ -60,7 +64,7 @@ func unpackTar(tr *tar.Reader, path string, whitelist []string) error { // if its a dir and it doesn't exist create it case tar.TypeDir: - if _, err := os.Stat(target); os.IsNotExist(err) { + if _, err := os.Lstat(target); os.IsNotExist(err) { if mode.Perm()&(1<<(uint(7))) == 0 { logrus.Debugf("Write permission bit not set on %s by default; setting manually", target) originalMode := mode @@ -93,7 +97,7 @@ func unpackTar(tr *tar.Reader, path string, whitelist []string) error { } // It's possible we end up creating files that can't be overwritten based on their permissions. // Explicitly delete an existing file before continuing. - if _, err := os.Stat(target); !os.IsNotExist(err) { + if _, err := os.Lstat(target); !os.IsNotExist(err) { logrus.Debugf("Removing %s for overwrite", target) if err := os.Remove(target); err != nil { logrus.Errorf("error removing file %s", target) @@ -118,9 +122,12 @@ func unpackTar(tr *tar.Reader, path string, whitelist []string) error { } currFile.Close() case tar.TypeSymlink: + if _, err := ResolvePathInRoot(path, tarLinkTarget(header.Name, header.Linkname), true); err != nil { + return errors.Wrap(err, "resolving tar symlink path") + } // It's possible we end up creating files that can't be overwritten based on their permissions. // Explicitly delete an existing file before continuing. - if _, err := os.Stat(target); !os.IsNotExist(err) { + if _, err := os.Lstat(target); !os.IsNotExist(err) { logrus.Debugf("Removing %s to create symlink", target) if err := os.RemoveAll(target); err != nil { logrus.Debugf("Unable to remove %s: %s", target, err) @@ -131,11 +138,16 @@ func unpackTar(tr *tar.Reader, path string, whitelist []string) error { logrus.Errorf("Failed to create symlink between %s and %s: %s", header.Linkname, target, err) } case tar.TypeLink: - linkname := filepath.Clean(filepath.Join(path, header.Linkname)) + linkname, err := ResolvePathInRoot(path, header.Linkname, false) + if err != nil { + return errors.Wrap(err, "resolving tar hard link path") + } // Check if the linkname already exists if _, err := os.Stat(linkname); !os.IsNotExist(err) { // If it exists, create the hard link - resolveHardlink(linkname, target) + if err := resolveHardlink(path, linkname, target); err != nil { + return errors.Wrap(err, fmt.Sprintf("Unable to create hard link from %s to %s", linkname, target)) + } } else { hardlinks.Store(target, linkname) } @@ -148,7 +160,7 @@ func unpackTar(tr *tar.Reader, path string, whitelist []string) error { logrus.Info("Resolving hard links") if _, err := os.Stat(linkname); !os.IsNotExist(err) { // If it exists, create the hard link - if err := resolveHardlink(linkname, target); err != nil { + if err := resolveHardlink(path, linkname, target); err != nil { resolveError.Store(errors.Wrap(err, fmt.Sprintf("Unable to create hard link from %s to %s", linkname, target))) return false } @@ -168,7 +180,36 @@ func unpackTar(tr *tar.Reader, path string, whitelist []string) error { return nil } -func resolveHardlink(linkname, target string) error { +func tarLinkTarget(name, linkname string) string { + if pathpkg.IsAbs(filepath.ToSlash(linkname)) { + return linkname + } + return pathpkg.Join(pathpkg.Dir(filepath.ToSlash(name)), filepath.ToSlash(linkname)) +} + +func resolveHardlink(root, linkname, target string) error { + info, err := os.Lstat(linkname) + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + linkTarget, err := os.Readlink(linkname) + if err != nil { + return err + } + relLink, err := filepath.Rel(root, linkname) + if err != nil { + return err + } + if _, err := ResolvePathInRoot(root, tarLinkTarget(relLink, linkTarget), true); err != nil { + return err + } + if err := os.Symlink(linkTarget, target); err != nil { + return err + } + logrus.Debugf("Created symlink from %s to %s for hard link source %s", linkTarget, target, linkname) + return nil + } if err := os.Link(linkname, target); err != nil { return err } diff --git a/internal/pkgutil/tar_utils_test.go b/internal/pkgutil/tar_utils_test.go new file mode 100644 index 00000000..e378f865 --- /dev/null +++ b/internal/pkgutil/tar_utils_test.go @@ -0,0 +1,178 @@ +package pkgutil + +import ( + "archive/tar" + "bytes" + "os" + "path/filepath" + "testing" +) + +func testTarReader(t *testing.T, entries ...func(*tar.Writer) error) *tar.Reader { + t.Helper() + + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + for _, entry := range entries { + if err := entry(tw); err != nil { + t.Fatal(err) + } + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + return tar.NewReader(bytes.NewReader(buf.Bytes())) +} + +func testTarFile(name, body string) func(*tar.Writer) error { + return func(tw *tar.Writer) error { + if err := tw.WriteHeader(&tar.Header{ + Name: name, + Mode: 0644, + Size: int64(len(body)), + }); err != nil { + return err + } + _, err := tw.Write([]byte(body)) + return err + } +} + +func testTarDir(name string) func(*tar.Writer) error { + return func(tw *tar.Writer) error { + return tw.WriteHeader(&tar.Header{ + Name: name, + Mode: 0755, + Typeflag: tar.TypeDir, + }) + } +} + +func testTarSymlink(name, linkname string) func(*tar.Writer) error { + return func(tw *tar.Writer) error { + return tw.WriteHeader(&tar.Header{ + Name: name, + Linkname: linkname, + Typeflag: tar.TypeSymlink, + }) + } +} + +func testTarHardlink(name, linkname string) func(*tar.Writer) error { + return func(tw *tar.Writer) error { + return tw.WriteHeader(&tar.Header{ + Name: name, + Linkname: linkname, + Typeflag: tar.TypeLink, + }) + } +} + +func TestUnpackTarRejectsEntriesOutsideRoot(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "root") + if err := os.MkdirAll(root, 0755); err != nil { + t.Fatal(err) + } + + err := unpackTar(testTarReader(t, testTarFile("../outside.txt", "outside-data")), root, nil) + if err == nil { + t.Fatal("unpackTar allowed an entry outside the root") + } + if _, err := os.Stat(filepath.Join(parent, "outside.txt")); !os.IsNotExist(err) { + t.Fatalf("outside path exists after rejected unpack: %v", err) + } +} + +func TestUnpackTarRejectsSymlinksOutsideRoot(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "root") + if err := os.MkdirAll(root, 0755); err != nil { + t.Fatal(err) + } + + err := unpackTar(testTarReader(t, testTarSymlink("link", "..")), root, nil) + if err == nil { + t.Fatal("unpackTar allowed a symlink outside the root") + } +} + +func TestUnpackTarRejectsHardlinksOutsideRoot(t *testing.T) { + root := t.TempDir() + + err := unpackTar(testTarReader(t, testTarHardlink("link", "../outside.txt")), root, nil) + if err == nil { + t.Fatal("unpackTar allowed a hard link outside the root") + } +} + +func TestUnpackTarHardlinkToSymlinkDoesNotFollowTarget(t *testing.T) { + root := t.TempDir() + + err := unpackTar(testTarReader(t, + testTarFile("file.txt", "image-data"), + testTarSymlink("sym", "file.txt"), + testTarHardlink("hard", "sym"), + ), root, nil) + if err != nil { + t.Fatal(err) + } + + info, err := os.Lstat(filepath.Join(root, "hard")) + if err != nil { + t.Fatal(err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("hard link to symlink should remain a symlink, got mode %s", info.Mode()) + } + target, err := os.Readlink(filepath.Join(root, "hard")) + if err != nil { + t.Fatal(err) + } + if target != "file.txt" { + t.Fatalf("hard link to symlink target = %q", target) + } +} + +func TestUnpackTarRejectsHardlinkSourceSymlinkOutsideRoot(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "root") + if err := os.MkdirAll(root, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(parent, "outside.txt"), []byte("outside-data"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../outside.txt", filepath.Join(root, "sym")); err != nil { + t.Fatal(err) + } + + err := unpackTar(testTarReader(t, testTarHardlink("hard", "sym")), root, nil) + if err == nil { + t.Fatal("unpackTar allowed a hard link through a symlink outside the root") + } + if _, err := os.Lstat(filepath.Join(root, "hard")); !os.IsNotExist(err) { + t.Fatalf("hard link path exists after rejected unpack: %v", err) + } +} + +func TestUnpackTarResolvesSymlinksWithinRoot(t *testing.T) { + root := t.TempDir() + + err := unpackTar(testTarReader(t, + testTarDir("etc"), + testTarSymlink("link", "/etc"), + testTarFile("link/generated.txt", "image-data"), + ), root, nil) + if err != nil { + t.Fatal(err) + } + + got, err := os.ReadFile(filepath.Join(root, "etc", "generated.txt")) + if err != nil { + t.Fatal(err) + } + if string(got) != "image-data" { + t.Fatalf("unpacked file through image symlink = %q", got) + } +} diff --git a/pkg/drivers/tar_driver.go b/pkg/drivers/tar_driver.go index af0a2021..bc255517 100644 --- a/pkg/drivers/tar_driver.go +++ b/pkg/drivers/tar_driver.go @@ -17,7 +17,6 @@ package drivers import ( "io/fs" "os" - "path/filepath" "strings" "github.com/pkg/errors" @@ -138,16 +137,28 @@ func (d *TarDriver) ProcessCommand(_ []unversioned.EnvVar, _ []string) (string, return "", "", -1, errors.New("Tar driver is unable to process commands, please use a different driver") } -func (d *TarDriver) StatFile(path string) (os.FileInfo, error) { - return os.Lstat(filepath.Join(d.Image.FSPath, path)) +func (d *TarDriver) StatFile(imagePath string) (os.FileInfo, error) { + path, err := pkgutil.ResolvePathInRoot(d.Image.FSPath, imagePath, false) + if err != nil { + return nil, err + } + return os.Lstat(path) } -func (d *TarDriver) ReadFile(path string) ([]byte, error) { - return os.ReadFile(filepath.Join(d.Image.FSPath, path)) +func (d *TarDriver) ReadFile(imagePath string) ([]byte, error) { + path, err := pkgutil.ResolvePathInRoot(d.Image.FSPath, imagePath, true) + if err != nil { + return nil, err + } + return os.ReadFile(path) } -func (d *TarDriver) ReadDir(path string) ([]os.FileInfo, error) { - entries, err := os.ReadDir(filepath.Join(d.Image.FSPath, path)) +func (d *TarDriver) ReadDir(imagePath string) ([]os.FileInfo, error) { + path, err := pkgutil.ResolvePathInRoot(d.Image.FSPath, imagePath, true) + if err != nil { + return nil, err + } + entries, err := os.ReadDir(path) if err != nil { return nil, err } diff --git a/pkg/drivers/tar_driver_test.go b/pkg/drivers/tar_driver_test.go new file mode 100644 index 00000000..84e86eb9 --- /dev/null +++ b/pkg/drivers/tar_driver_test.go @@ -0,0 +1,122 @@ +package drivers + +import ( + "os" + "path/filepath" + "testing" + + "github.com/GoogleContainerTools/container-structure-test/internal/pkgutil" +) + +func TestTarDriverFileOperationsStayWithinImageRoot(t *testing.T) { + parent := t.TempDir() + imageRoot := filepath.Join(parent, "image-root") + if err := os.MkdirAll(filepath.Join(imageRoot, "etc"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(imageRoot, "etc", "config.txt"), []byte("image-data"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(parent, "outside.txt"), []byte("outside-data"), 0644); err != nil { + t.Fatal(err) + } + + driver := &TarDriver{Image: pkgutil.Image{FSPath: imageRoot}} + + if got, err := driver.ReadFile("/etc/config.txt"); err != nil || string(got) != "image-data" { + t.Fatalf("ReadFile inside image root = %q, %v", got, err) + } + if _, err := driver.StatFile("/etc/config.txt"); err != nil { + t.Fatalf("StatFile inside image root: %v", err) + } + if _, err := driver.ReadDir("/etc"); err != nil { + t.Fatalf("ReadDir inside image root: %v", err) + } + + for name, op := range map[string]func() error{ + "ReadFile": func() error { + _, err := driver.ReadFile("../outside.txt") + return err + }, + "StatFile": func() error { + _, err := driver.StatFile("../outside.txt") + return err + }, + "ReadDir": func() error { + _, err := driver.ReadDir("..") + return err + }, + } { + if err := op(); err == nil { + t.Fatalf("%s allowed a path outside the image root", name) + } + } +} + +func TestTarDriverResolvesSymlinksWithinImageRoot(t *testing.T) { + imageRoot := t.TempDir() + if err := os.MkdirAll(filepath.Join(imageRoot, "etc"), 0755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(imageRoot, "bin"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(imageRoot, "etc", "config.txt"), []byte("image-data"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("/etc/config.txt", filepath.Join(imageRoot, "absolute-link")); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../etc/config.txt", filepath.Join(imageRoot, "bin", "relative-link")); err != nil { + t.Fatal(err) + } + + driver := &TarDriver{Image: pkgutil.Image{FSPath: imageRoot}} + + for _, imagePath := range []string{"/absolute-link", "/bin/relative-link"} { + got, err := driver.ReadFile(imagePath) + if err != nil { + t.Fatalf("ReadFile(%q): %v", imagePath, err) + } + if string(got) != "image-data" { + t.Fatalf("ReadFile(%q) = %q", imagePath, got) + } + } + + info, err := driver.StatFile("/absolute-link") + if err != nil { + t.Fatalf("StatFile on symlink: %v", err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("StatFile should return the final symlink itself, got mode %s", info.Mode()) + } +} + +func TestTarDriverRejectsSymlinksOutsideImageRoot(t *testing.T) { + parent := t.TempDir() + imageRoot := filepath.Join(parent, "image-root") + if err := os.MkdirAll(imageRoot, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(parent, "outside.txt"), []byte("outside-data"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../outside.txt", filepath.Join(imageRoot, "file-link")); err != nil { + t.Fatal(err) + } + if err := os.Symlink("..", filepath.Join(imageRoot, "dir-link")); err != nil { + t.Fatal(err) + } + + driver := &TarDriver{Image: pkgutil.Image{FSPath: imageRoot}} + + if _, err := driver.ReadFile("/file-link"); err == nil { + t.Fatal("ReadFile allowed a symlink outside the image root") + } + if _, err := driver.ReadDir("/dir-link"); err == nil { + t.Fatal("ReadDir allowed a symlink outside the image root") + } + if _, err := driver.StatFile("/dir-link/outside.txt"); err == nil { + t.Fatal("StatFile allowed an intermediate symlink outside the image root") + } +}