chunked: improve creation of files under root

ihen the file name is the root directory, avoid using an empty string
or the ".." name to open the file.  The latter does not cause any
security issues or unexpected behavior, it is logically incorrect and
should be avoided.

Closes: #2191

Signed-off-by: Giuseppe Scrivano <[email protected]>

pkg/chunked/filesystem_linux.go

@@ -49,10 +49,33 @@ 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
+// empty string.
+// The returned base value is never empty, it never contains any slash and the
+// value "..".
+func splitPath(path string) (string, string, error) {
+	path = filepath.Clean(path)
+	dir, base := filepath.Split(path)
+	dir = cleanAbsDirectory(dir)
+	if base == "" || base == ".." {
+		base = "."
+	}
+	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 +95,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 +304,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
@@ -448,9 +474,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 +535,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 +574,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

pkg/chunked/filesystem_linux_test.go

@@ -2,6 +2,7 @@ package chunked
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"os"
 	"path"
@@ -123,6 +124,9 @@ 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)
 
@@ -334,3 +338,48 @@ 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/../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))
+	}
+}