Properly handle links in tar

pkg/kubectl/cmd/cp/BUILD

@@ -32,6 +32,7 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/cli-runtime/pkg/genericclioptions:go_default_library",
         "//staging/src/k8s.io/client-go/rest/fake:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
     ],
 )
 

pkg/kubectl/cmd/cp/cp.go

@@ -418,9 +418,7 @@ func clean(fileName string) string {
 	return path.Clean(string(os.PathSeparator) + fileName)
 }
 
-func (o *CopyOptions) untarAll(reader io.Reader, destFile, prefix string) error {
-	entrySeq := -1
-
+func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error {
 	// TODO: use compression here?
 	tarReader := tar.NewReader(reader)
 	for {
@@ -431,52 +429,60 @@ func (o *CopyOptions) untarAll(reader io.Reader, destFile, prefix string) error
 			}
 			break
 		}
-		entrySeq++
-		mode := header.FileInfo().Mode()
-		// all the files will start with the prefix, which is the directory where
+
+		// All the files will start with the prefix, which is the directory where
 		// they were located on the pod, we need to strip down that prefix, but
-		// if the prefix is missing it means the tar was tempered with
+		// if the prefix is missing it means the tar was tempered with.
+		// For the case where prefix is empty we need to ensure that the path
+		// is not absolute, which also indicates the tar file was tempered with.
 		if !strings.HasPrefix(header.Name, prefix) {
 			return fmt.Errorf("tar contents corrupted")
 		}
-		outFileName := path.Join(destFile, clean(header.Name[len(prefix):]))
-		baseName := path.Dir(outFileName)
+
+		// basic file information
+		mode := header.FileInfo().Mode()
+		destFileName := path.Join(destDir, header.Name[len(prefix):])
+		baseName := path.Dir(destFileName)
+
 		if err := os.MkdirAll(baseName, 0755); err != nil {
 			return err
 		}
 		if header.FileInfo().IsDir() {
-			if err := os.MkdirAll(outFileName, 0755); err != nil {
+			if err := os.MkdirAll(destFileName, 0755); err != nil {
 				return err
 			}
 			continue
 		}
 
-		// handle coping remote file into local directory
-		if entrySeq == 0 && !header.FileInfo().IsDir() {
-			exists, err := dirExists(outFileName)
-			if err != nil {
-				return err
-			}
-			if exists {
-				outFileName = filepath.Join(outFileName, path.Base(clean(header.Name)))
-			}
+		// We need to ensure that the destination file is always within boundries
+		// of the destination directory. This prevents any kind of path traversal
+		// from within tar archive.
+		dir, file := filepath.Split(destFileName)
+		evaledPath, err := filepath.EvalSymlinks(dir)
+		if err != nil {
+			return err
+		}
+		// For scrutiny we verify both the actual destination as well as we follow
+		// all the links that might lead outside of the destination directory.
+		if !isDestRelative(destDir, destFileName) || !isDestRelative(destDir, filepath.Join(evaledPath, file)) {
+			fmt.Fprintf(o.IOStreams.ErrOut, "warning: link %q is pointing to %q which is outside target destination, skipping\n", destFileName, header.Linkname)
+			continue
 		}
 
 		if mode&os.ModeSymlink != 0 {
 			linkname := header.Linkname
-			// error is returned if linkname can't be made relative to destFile,
-			// but relative can end up being ../dir that's why we also need to
-			// verify if relative path is the same after Clean-ing
-			relative, err := filepath.Rel(destFile, linkname)
-			if path.IsAbs(linkname) && (err != nil || relative != stripPathShortcuts(relative)) {
-				fmt.Fprintf(o.IOStreams.ErrOut, "warning: link %q is pointing to %q which is outside target destination, skipping\n", outFileName, header.Linkname)
+			// We need to ensure that the link destination is always within boundries
+			// of the destination directory. This prevents any kind of path traversal
+			// from within tar archive.
+			if !isDestRelative(destDir, linkJoin(destFileName, linkname)) {
+				fmt.Fprintf(o.IOStreams.ErrOut, "warning: link %q is pointing to %q which is outside target destination, skipping\n", destFileName, header.Linkname)
 				continue
 			}
-			if err := os.Symlink(linkname, outFileName); err != nil {
+			if err := os.Symlink(linkname, destFileName); err != nil {
 				return err
 			}
 		} else {
-			outFile, err := os.Create(outFileName)
+			outFile, err := os.Create(destFileName)
 			if err != nil {
 				return err
 			}
@@ -490,14 +496,32 @@ func (o *CopyOptions) untarAll(reader io.Reader, destFile, prefix string) error
 		}
 	}
 
-	if entrySeq == -1 {
-		//if no file was copied
-		errInfo := fmt.Sprintf("error: %s no such file or directory", prefix)
-		return errors.New(errInfo)
-	}
 	return nil
 }
 
+// linkJoin joins base and link to get the final path to be created.
+// It will consider whether link is an absolute path or not when returning result.
+func linkJoin(base, link string) string {
+	if filepath.IsAbs(link) {
+		return link
+	}
+	return filepath.Join(base, link)
+}
+
+// isDestRelative returns true if dest is pointing outside the base directory,
+// false otherwise.
+func isDestRelative(base, dest string) bool {
+	fullPath := dest
+	if !filepath.IsAbs(dest) {
+		fullPath = filepath.Join(base, dest)
+	}
+	relative, err := filepath.Rel(base, fullPath)
+	if err != nil {
+		return false
+	}
+	return relative == "." || relative == stripPathShortcuts(relative)
+}
+
 func getPrefix(file string) string {
 	// tar strips the leading '/' if it's there, so we will too
 	return strings.TrimLeft(file, "/")
@@ -524,15 +548,3 @@ func (o *CopyOptions) execute(options *exec.ExecOptions) error {
 	}
 	return nil
 }
-
-// dirExists checks if a path exists and is a directory.
-func dirExists(path string) (bool, error) {
-	fi, err := os.Stat(path)
-	if err == nil && fi.IsDir() {
-		return true, nil
-	}
-	if os.IsNotExist(err) {
-		return false, nil
-	}
-	return false, err
-}

pkg/kubectl/cmd/cp/cp_test.go

@@ -19,11 +19,11 @@ package cp
 import (
 	"archive/tar"
 	"bytes"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"os"
-	"os/exec"
 	"path"
 	"path/filepath"
 	"reflect"
@@ -31,6 +31,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -191,6 +192,67 @@ func TestStripPathShortcuts(t *testing.T) {
 		}
 	}
 }
+func TestIsDestRelative(t *testing.T) {
+	tests := []struct {
+		base     string
+		dest     string
+		relative bool
+	}{
+		{
+			base:     "/dir",
+			dest:     "../link",
+			relative: false,
+		},
+		{
+			base:     "/dir",
+			dest:     "../../link",
+			relative: false,
+		},
+		{
+			base:     "/dir",
+			dest:     "/link",
+			relative: false,
+		},
+		{
+			base:     "/dir",
+			dest:     "link",
+			relative: true,
+		},
+		{
+			base:     "/dir",
+			dest:     "int/file/link",
+			relative: true,
+		},
+		{
+			base:     "/dir",
+			dest:     "int/../link",
+			relative: true,
+		},
+		{
+			base:     "/dir",
+			dest:     "/dir/link",
+			relative: true,
+		},
+		{
+			base:     "/dir",
+			dest:     "/dir/int/../link",
+			relative: true,
+		},
+		{
+			base:     "/dir",
+			dest:     "/dir/../../link",
+			relative: false,
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			if test.relative != isDestRelative(test.base, test.dest) {
+				t.Errorf("unexpected result for: base %q, dest %q", test.base, test.dest)
+			}
+		})
+	}
+}
 
 func checkErr(t *testing.T, err error) {
 	if err != nil {
@@ -367,97 +429,6 @@ func TestTarUntarWrongPrefix(t *testing.T) {
 	}
 }
 
-// TestCopyToLocalFileOrDir tests untarAll in two cases :
-// 1: copy pod file to local file
-// 2: copy pod file into local directory
-func TestCopyToLocalFileOrDir(t *testing.T) {
-	dir, err := ioutil.TempDir(os.TempDir(), "input")
-	dir2, err2 := ioutil.TempDir(os.TempDir(), "output")
-	if err != nil || err2 != nil {
-		t.Errorf("unexpected error: %v | %v", err, err2)
-		t.FailNow()
-	}
-	defer func() {
-		if err := os.RemoveAll(dir); err != nil {
-			t.Errorf("Unexpected error cleaning up: %v", err)
-		}
-		if err := os.RemoveAll(dir2); err != nil {
-			t.Errorf("Unexpected error cleaning up: %v", err)
-		}
-	}()
-
-	files := []struct {
-		name          string
-		data          string
-		dest          string
-		destDirExists bool
-	}{
-		{
-			name:          "foo",
-			data:          "foobarbaz",
-			dest:          "path/to/dest",
-			destDirExists: false,
-		},
-		{
-			name:          "dir/blah",
-			data:          "bazblahfoo",
-			dest:          "dest/file/path",
-			destDirExists: true,
-		},
-	}
-
-	for _, file := range files {
-		func() {
-			// setup
-			srcFilePath := filepath.Join(dir, file.name)
-			destPath := filepath.Join(dir2, file.dest)
-			if err := os.MkdirAll(filepath.Dir(srcFilePath), 0755); err != nil {
-				t.Errorf("unexpected error: %v", err)
-				t.FailNow()
-			}
-			createTmpFile(t, srcFilePath, file.data)
-			if file.destDirExists {
-				if err := os.MkdirAll(destPath, 0755); err != nil {
-					t.Errorf("unexpected error: %v", err)
-					t.FailNow()
-				}
-			}
-
-			// start tests
-			srcTarFilePath := filepath.Join(dir, file.name+".tar")
-			// here use tar command to create tar file instead of calling makeTar func
-			// because makeTar func can not generate correct header name
-			err = exec.Command("tar", "cf", srcTarFilePath, srcFilePath).Run()
-			if err != nil {
-				t.Errorf("unexpected error: %v", err)
-				t.FailNow()
-			}
-			srcTarFile, err := os.Open(srcTarFilePath)
-			if err != nil {
-				t.Errorf("unexpected error: %v", err)
-				t.FailNow()
-			}
-			defer srcTarFile.Close()
-
-			opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard())
-			if err := opts.untarAll(srcTarFile, destPath, getPrefix(srcFilePath)); err != nil {
-				t.Errorf("unexpected error: %v", err)
-				t.FailNow()
-			}
-			actualDestFilePath := destPath
-			if file.destDirExists {
-				actualDestFilePath = filepath.Join(destPath, filepath.Base(srcFilePath))
-			}
-			_, err = os.Stat(actualDestFilePath)
-			if err != nil && os.IsNotExist(err) {
-				t.Errorf("expecting %s exists, but actually it's missing", actualDestFilePath)
-			}
-			cmpFileData(t, actualDestFilePath, file.data)
-		}()
-	}
-
-}
-
 func TestTarDestinationName(t *testing.T) {
 	dir, err := ioutil.TempDir(os.TempDir(), "input")
 	dir2, err2 := ioutil.TempDir(os.TempDir(), "output")
@@ -544,7 +515,6 @@ func TestBadTar(t *testing.T) {
 		name string
 		body string
 	}{
-		{"/prefix/../../../tmp/foo", "Up to temp"},
 		{"/prefix/foo/bar/../../home/bburns/names.txt", "Down and back"},
 	}
 	for _, file := range files {
@@ -801,13 +771,13 @@ func TestUntar(t *testing.T) {
 		expected: filepath.Join(testdir, dest, "relative"),
 	}, { // Relative file outside dest
 		path:     "../unrelative",
-		expected: filepath.Join(testdir, dest, "unrelative"),
+		expected: "",
 	}, { // Nested relative file inside dest
 		path:     "nested/nest-rel",
 		expected: filepath.Join(testdir, dest, "nested/nest-rel"),
 	}, { // Nested relative file outside dest
 		path:     "nested/../../nest-unrelative",
-		expected: filepath.Join(testdir, dest, "nest-unrelative"),
+		expected: "",
 	}}
 
 	mkExpectation := func(expected, suffix string) string {
@@ -816,6 +786,13 @@ func TestUntar(t *testing.T) {
 		}
 		return expected + suffix
 	}
+	mkBacktickExpectation := func(expected, suffix string) string {
+		dir, _ := filepath.Split(filepath.Clean(expected))
+		if len(strings.Split(dir, string(os.PathSeparator))) <= 1 {
+			return ""
+		}
+		return expected + suffix
+	}
 	links := []file{}
 	for _, f := range files {
 		links = append(links, file{
@@ -825,11 +802,11 @@ func TestUntar(t *testing.T) {
 		}, file{
 			path:       f.path + "-innerlink-abs",
 			linkTarget: filepath.Join(testdir, dest, "link-target"),
-			expected:   "",
+			expected:   mkExpectation(f.expected, "-innerlink-abs"),
 		}, file{
 			path:       f.path + "-outerlink",
 			linkTarget: filepath.Join(backtick(f.path), "link-target"),
-			expected:   "",
+			expected:   mkBacktickExpectation(f.expected, "-outerlink"),
 		}, file{
 			path:       f.path + "-outerlink-abs",
 			linkTarget: filepath.Join(testdir, "link-target"),
@@ -863,7 +840,19 @@ func TestUntar(t *testing.T) {
 			expected:   filepath.Join(testdir, dest, "back-link-second"),
 		},
 		file{
-			path: "nested/back-link-first/back-link-second/back-link-term",
+			// This case is chaining together symlinks that step back, so that
+			// if you just look at the target relative to the path it appears
+			// inside the destination directory, but if you actually follow each
+			// step of the path you end up outside the destination directory.
+			path:       "nested/back-link-first/back-link-second/back-link-term",
+			linkTarget: "",
+			expected:   "",
+		})
+
+	files = append(files,
+		file{ // Relative directory path with terminating /
+			path:     "direct/dir/",
+			expected: "",
 		})
 
 	buf := &bytes.Buffer{}
@@ -880,8 +869,10 @@ func TestUntar(t *testing.T) {
 				Size: int64(len(f.path)),
 			}
 			require.NoError(t, tw.WriteHeader(hdr), f.path)
-			_, err := tw.Write([]byte(f.path))
-			require.NoError(t, err, f.path)
+			if !strings.HasSuffix(f.path, "/") {
+				_, err := tw.Write([]byte(f.path))
+				require.NoError(t, err, f.path)
+			}
 		} else {
 			hdr := &tar.Header{
 				Name:     f.path,