diff --git a/pkg/chunked/dump/dump.go b/pkg/chunked/dump/dump.go index d98cee09de..7b58eb4835 100644 --- a/pkg/chunked/dump/dump.go +++ b/pkg/chunked/dump/dump.go @@ -13,6 +13,7 @@ import ( "time" "github.com/containers/storage/pkg/chunked/internal" + storagePath "github.com/containers/storage/pkg/chunked/internal/path" "golang.org/x/sys/unix" ) @@ -103,18 +104,8 @@ func getStMode(mode uint32, typ string) (uint32, error) { return mode, nil } -func sanitizeName(name string) string { - path := filepath.Clean(name) - if path == "." { - path = "/" - } else if path[0] != '/' { - path = "/" + path - } - return path -} - func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[string]int, verityDigests map[string]string, entry *internal.FileMetadata) error { - path := sanitizeName(entry.Name) + path := storagePath.CleanAbsPath(entry.Name) parent := filepath.Dir(path) if _, found := added[parent]; !found && path != "/" { @@ -172,7 +163,7 @@ func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[ if entry.Type == internal.TypeSymlink { payload = entry.Linkname } else { - payload = sanitizeName(entry.Linkname) + payload = storagePath.CleanAbsPath(entry.Linkname) } } else { if len(entry.Digest) > 10 { diff --git a/pkg/chunked/filesystem_linux.go b/pkg/chunked/filesystem_linux.go index e26e5e5c55..1552ca0f00 100644 --- a/pkg/chunked/filesystem_linux.go +++ b/pkg/chunked/filesystem_linux.go @@ -16,6 +16,7 @@ import ( driversCopy "github.com/containers/storage/drivers/copy" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chunked/internal" + storagePath "github.com/containers/storage/pkg/chunked/internal/path" securejoin "github.com/cyphar/filepath-securejoin" "github.com/vbatts/tar-split/archive/tar" "golang.org/x/sys/unix" @@ -49,10 +50,37 @@ type fileMetadata struct { skipSetAttrs bool } +// splitPath takes a file path as input and returns two components: dir and base. +// Differently than filepath.Split(), this function handles some edge cases. +// If the path refers to a file in the root directory, the returned dir is "/". +// The returned base value is never empty, it never contains any slash and the +// value "..". +func splitPath(path string) (string, string, error) { + path = storagePath.CleanAbsPath(path) + dir, base := filepath.Split(path) + if base == "" { + base = "." + } + // Remove trailing slashes from dir, but make sure that "/" is preserved. + dir = strings.TrimSuffix(dir, "/") + if dir == "" { + dir = "/" + } + + if strings.Contains(base, "/") { + // This should never happen, but be safe as the base is passed to *at syscalls. + return "", "", fmt.Errorf("internal error: splitPath(%q) contains a slash", path) + } + return dir, base, nil +} + func doHardLink(dirfd, srcFd int, destFile string) error { - destDir, destBase := filepath.Split(destFile) + destDir, destBase, err := splitPath(destFile) + if err != nil { + return err + } destDirFd := dirfd - if destDir != "" && destDir != "." { + if destDir != "/" { f, err := openOrCreateDirUnderRoot(dirfd, destDir, 0) if err != nil { return err @@ -72,7 +100,7 @@ func doHardLink(dirfd, srcFd int, destFile string) error { return nil } - err := doLink() + err = doLink() // if the destination exists, unlink it first and try again if err != nil && os.IsExist(err) { @@ -281,8 +309,11 @@ func openFileUnderRootFallback(dirfd int, name string, flags uint64, mode os.Fil // If O_NOFOLLOW is specified in the flags, then resolve only the parent directory and use the // last component as the path to openat(). if hasNoFollow { - dirName, baseName := filepath.Split(name) - if dirName != "" && dirName != "." { + dirName, baseName, err := splitPath(name) + if err != nil { + return -1, err + } + if dirName != "/" { newRoot, err := securejoin.SecureJoin(root, dirName) if err != nil { return -1, err @@ -409,7 +440,8 @@ func openOrCreateDirUnderRoot(dirfd int, name string, mode os.FileMode) (*os.Fil if errors.Is(err, unix.ENOENT) { parent := filepath.Dir(name) - if parent != "" { + // do not create the root directory, it should always exist + if parent != name { pDir, err2 := openOrCreateDirUnderRoot(dirfd, parent, mode) if err2 != nil { return nil, err @@ -448,9 +480,12 @@ func appendHole(fd int, name string, size int64) error { } func safeMkdir(dirfd int, mode os.FileMode, name string, metadata *fileMetadata, options *archive.TarOptions) error { - parent, base := filepath.Split(name) + parent, base, err := splitPath(name) + if err != nil { + return err + } parentFd := dirfd - if parent != "" && parent != "." { + if parent != "/" { parentFile, err := openOrCreateDirUnderRoot(dirfd, parent, 0) if err != nil { return err @@ -506,9 +541,12 @@ func safeLink(dirfd int, mode os.FileMode, metadata *fileMetadata, options *arch } func safeSymlink(dirfd int, metadata *fileMetadata) error { - destDir, destBase := filepath.Split(metadata.Name) + destDir, destBase, err := splitPath(metadata.Name) + if err != nil { + return err + } destDirFd := dirfd - if destDir != "" && destDir != "." { + if destDir != "/" { f, err := openOrCreateDirUnderRoot(dirfd, destDir, 0) if err != nil { return err @@ -542,9 +580,12 @@ func (d whiteoutHandler) Setxattr(path, name string, value []byte) error { } func (d whiteoutHandler) Mknod(path string, mode uint32, dev int) error { - dir, base := filepath.Split(path) + dir, base, err := splitPath(path) + if err != nil { + return err + } dirfd := d.Dirfd - if dir != "" && dir != "." { + if dir != "/" { dir, err := openOrCreateDirUnderRoot(d.Dirfd, dir, 0) if err != nil { return err diff --git a/pkg/chunked/filesystem_linux_test.go b/pkg/chunked/filesystem_linux_test.go index 7cb2cefb1f..3e3780793c 100644 --- a/pkg/chunked/filesystem_linux_test.go +++ b/pkg/chunked/filesystem_linux_test.go @@ -2,6 +2,7 @@ package chunked import ( "bytes" + "fmt" "io" "os" "path" @@ -123,11 +124,14 @@ func TestSafeMkdir(t *testing.T) { IgnoreChownErrors: true, } + err = safeMkdir(rootFd, 0o755, "/", &metadata, options) + require.NoError(t, err) + err = safeMkdir(rootFd, 0o755, dirName, &metadata, options) require.NoError(t, err) dir, err := openFileUnderRoot(rootFd, dirName, syscall.O_DIRECTORY|syscall.O_CLOEXEC, 0) - assert.NoError(t, err) + require.NoError(t, err) err = dir.Close() assert.NoError(t, err) } @@ -167,7 +171,7 @@ func TestSafeLink(t *testing.T) { // validate it was created newFile, err := openFileUnderRoot(rootFd, linkName, syscall.O_RDONLY, 0) - assert.NoError(t, err) + require.NoError(t, err) st := syscall.Stat_t{} err = syscall.Fstat(int(newFile.Fd()), &st) @@ -216,7 +220,7 @@ func TestSafeSymlink(t *testing.T) { // validate it was created newFile, err := openFileUnderRoot(rootFd, linkName, syscall.O_RDONLY, 0) - assert.NoError(t, err) + require.NoError(t, err) st2 := syscall.Stat_t{} err = syscall.Fstat(int(newFile.Fd()), &st2) @@ -334,3 +338,49 @@ func createTempFile(t *testing.T, dir, name string) *os.File { require.NoError(t, err) return tmpFile } + +func TestSplitPath(t *testing.T) { + tests := []struct { + path string + expectedDir string + expectedBase string + }{ + {"", "/", "."}, + {".", "/", "."}, + {"..", "/", "."}, + {"../..", "/", "."}, + {"../../..", "/", "."}, + {"../../../foo", "/", "foo"}, + {"../../../foo/..", "/", "."}, + {"../../../foo/./../foo/bar/baz", "/foo/bar", "baz"}, + {"../../../foo/bar", "/foo", "bar"}, + {"/", "/", "."}, + {"/.", "/", "."}, + {"/..", "/", "."}, + {"////foo////bar////", "/foo", "bar"}, + {"/foo", "/", "foo"}, + {"/foo/", "/", "foo"}, + {"/foo/bar", "/foo", "bar"}, + {"/foo/bar/", "/foo", "bar"}, + {"/foo/////bar/", "/foo", "bar"}, + {"/home/foo/file.txt", "/home/foo", "file.txt"}, + {"/home/foo////file.txt", "/home/foo", "file.txt"}, + {"file", "/", "file"}, + {"foo/", "/", "foo"}, + {"foo/.", "/", "foo"}, + {"foo/..", "/", "."}, + {"foo/../../bar", "/", "bar"}, + {"foo/bar/", "/foo", "bar"}, + {"foo/bar/..", "/", "foo"}, + {"foo/bar/../baz", "/foo", "baz"}, + {"foo/bar/baz/", "/foo/bar", "baz"}, + {"foo/file.txt", "/foo", "file.txt"}, + } + + for _, test := range tests { + dir, base, err := splitPath(test.path) + assert.NoError(t, err) + assert.Equal(t, test.expectedDir, dir, fmt.Sprintf("path %q: expected dir %q, got %q", test.path, test.expectedDir, dir)) + assert.Equal(t, test.expectedBase, base, fmt.Sprintf("path %q: expected base %q, got %q", test.path, test.expectedBase, base)) + } +} diff --git a/pkg/chunked/internal/path/path.go b/pkg/chunked/internal/path/path.go new file mode 100644 index 0000000000..50ca1def87 --- /dev/null +++ b/pkg/chunked/internal/path/path.go @@ -0,0 +1,12 @@ +package path + +import ( + "path/filepath" +) + +// CleanAbsPath removes any ".." and "." from the path +// and ensures it starts with a "/". If the path refers to the root +// directory, it returns "/". +func CleanAbsPath(path string) string { + return filepath.Clean("/" + path) +} diff --git a/pkg/chunked/internal/path/path_test.go b/pkg/chunked/internal/path/path_test.go new file mode 100644 index 0000000000..a410483e2b --- /dev/null +++ b/pkg/chunked/internal/path/path_test.go @@ -0,0 +1,48 @@ +package path + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCleanAbsPath(t *testing.T) { + tests := []struct { + path string + expected string + }{ + {"", "/"}, + {".", "/"}, + {"..", "/"}, + {"foo/../..", "/"}, + {"/foo/../..", "/"}, + {"./", "/"}, + {"../", "/"}, + {"/../", "/"}, + {"/./", "/"}, + {"foo", "/foo"}, + {"foo/bar", "/foo/bar"}, + {"/foo/bar/../baz", "/foo/baz"}, + {"/foo/./bar", "/foo/bar"}, + {"/foo/bar/../../baz", "/baz"}, + {"/././foo", "/foo"}, + {"../foo", "/foo"}, + {"./foo/bar/../..", "/"}, + {"foo/..", "/"}, + {"foo/../bar", "/bar"}, + {"//foo//bar", "/foo/bar"}, + {"foo/bar//baz/..", "/foo/bar"}, + {"../..", "/"}, + {".././..", "/"}, + {"../../.", "/"}, + {"/../../foo", "/foo"}, + {"../foo/bar/../baz", "/foo/baz"}, + {"../.././/.//../foo/./../bar/..", "/"}, + {"a/../.././/.//../foo/./../bar/..", "/"}, + } + + for _, test := range tests { + assert.Equal(t, test.expected, CleanAbsPath(test.path), fmt.Sprintf("path %q failed", test.path)) + } +} diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index 482c459199..d64a45b684 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -23,6 +23,7 @@ import ( "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/chunked/compressor" "github.com/containers/storage/pkg/chunked/internal" + path "github.com/containers/storage/pkg/chunked/internal/path" "github.com/containers/storage/pkg/chunked/toc" "github.com/containers/storage/pkg/fsverity" "github.com/containers/storage/pkg/idtools" @@ -1544,10 +1545,10 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff } } - r.Name = filepath.Clean(r.Name) + r.Name = path.CleanAbsPath(r.Name) // do not modify the value of symlinks if r.Linkname != "" && t != tar.TypeSymlink { - r.Linkname = filepath.Clean(r.Linkname) + r.Linkname = path.CleanAbsPath(r.Linkname) } if whiteoutConverter != nil { @@ -1595,7 +1596,7 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff } case tar.TypeDir: - if r.Name == "" || r.Name == "." { + if r.Name == "/" { output.RootDirMode = &mode } if err := safeMkdir(dirfd, mode, r.Name, r, options); err != nil {