diff --git a/pkg/kubectl/cmd/cp/BUILD b/pkg/kubectl/cmd/cp/BUILD index 77a19b4dc2..311b5bd7b7 100644 --- a/pkg/kubectl/cmd/cp/BUILD +++ b/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/assert:go_default_library", "//vendor/github.com/stretchr/testify/require:go_default_library", ], ) diff --git a/pkg/kubectl/cmd/cp/cp.go b/pkg/kubectl/cmd/cp/cp.go index 9b34422f38..6aae4131a2 100644 --- a/pkg/kubectl/cmd/cp/cp.go +++ b/pkg/kubectl/cmd/cp/cp.go @@ -441,9 +441,14 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error { // basic file information mode := header.FileInfo().Mode() - destFileName := path.Join(destDir, header.Name[len(prefix):]) - baseName := path.Dir(destFileName) + destFileName := filepath.Join(destDir, header.Name[len(prefix):]) + if !isDestRelative(destDir, destFileName) { + fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is outside target destination, skipping\n", destFileName) + continue + } + + baseName := filepath.Dir(destFileName) if err := os.MkdirAll(baseName, 0755); err != nil { return err } @@ -457,15 +462,14 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error { // 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) + evaledPath, err := filepath.EvalSymlinks(baseName) 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) + if !isDestRelative(destDir, filepath.Join(evaledPath, filepath.Base(destFileName))) { + fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is outside target destination, skipping\n", destFileName) continue } @@ -474,7 +478,11 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error { // 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)) { + linkTarget := linkname + if !filepath.IsAbs(linkname) { + linkTarget = filepath.Join(evaledPath, linkname) + } + if !isDestRelative(destDir, linkTarget) { fmt.Fprintf(o.IOStreams.ErrOut, "warning: link %q is pointing to %q which is outside target destination, skipping\n", destFileName, header.Linkname) continue } @@ -499,23 +507,10 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error { 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) + relative, err := filepath.Rel(base, dest) if err != nil { return false } diff --git a/pkg/kubectl/cmd/cp/cp_test.go b/pkg/kubectl/cmd/cp/cp_test.go index c7d75573d0..db76a6b341 100644 --- a/pkg/kubectl/cmd/cp/cp_test.go +++ b/pkg/kubectl/cmd/cp/cp_test.go @@ -30,6 +30,7 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/api/core/v1" @@ -200,12 +201,12 @@ func TestIsDestRelative(t *testing.T) { }{ { base: "/dir", - dest: "../link", + dest: "/dir/../link", relative: false, }, { base: "/dir", - dest: "../../link", + dest: "/dir/../../link", relative: false, }, { @@ -213,21 +214,6 @@ func TestIsDestRelative(t *testing.T) { 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", @@ -239,8 +225,18 @@ func TestIsDestRelative(t *testing.T) { relative: true, }, { - base: "/dir", - dest: "/dir/../../link", + base: "dir", + dest: "dir/link", + relative: true, + }, + { + base: "dir", + dest: "dir/int/../link", + relative: true, + }, + { + base: "dir", + dest: "dir/../../link", relative: false, }, } @@ -744,9 +740,7 @@ func TestUntar(t *testing.T) { defer os.RemoveAll(testdir) t.Logf("Test base: %s", testdir) - const ( - dest = "base" - ) + basedir := filepath.Join(testdir, "base") type file struct { path string @@ -755,26 +749,26 @@ func TestUntar(t *testing.T) { } files := []file{{ // Absolute file within dest - path: filepath.Join(testdir, dest, "abs"), - expected: filepath.Join(testdir, dest, testdir, dest, "abs"), + path: filepath.Join(basedir, "abs"), + expected: filepath.Join(basedir, basedir, "abs"), }, { // Absolute file outside dest path: filepath.Join(testdir, "abs-out"), - expected: filepath.Join(testdir, dest, testdir, "abs-out"), + expected: filepath.Join(basedir, testdir, "abs-out"), }, { // Absolute nested file within dest - path: filepath.Join(testdir, dest, "nested/nest-abs"), - expected: filepath.Join(testdir, dest, testdir, dest, "nested/nest-abs"), + path: filepath.Join(basedir, "nested/nest-abs"), + expected: filepath.Join(basedir, basedir, "nested/nest-abs"), }, { // Absolute nested file outside dest - path: filepath.Join(testdir, dest, "nested/../../nest-abs-out"), - expected: filepath.Join(testdir, dest, testdir, "nest-abs-out"), + path: filepath.Join(basedir, "nested/../../nest-abs-out"), + expected: filepath.Join(basedir, testdir, "nest-abs-out"), }, { // Relative file inside dest path: "relative", - expected: filepath.Join(testdir, dest, "relative"), + expected: filepath.Join(basedir, "relative"), }, { // Relative file outside dest path: "../unrelative", expected: "", }, { // Nested relative file inside dest path: "nested/nest-rel", - expected: filepath.Join(testdir, dest, "nested/nest-rel"), + expected: filepath.Join(basedir, "nested/nest-rel"), }, { // Nested relative file outside dest path: "nested/../../nest-unrelative", expected: "", @@ -786,9 +780,11 @@ 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 { + mkBacklinkExpectation := func(expected, suffix string) string { + // "resolve" the back link relative to the expectation + targetDir := filepath.Dir(filepath.Dir(expected)) + // If the "resolved" target is not nested in basedir, it is escaping. + if !filepath.HasPrefix(targetDir, basedir) { return "" } return expected + suffix @@ -801,17 +797,27 @@ func TestUntar(t *testing.T) { expected: mkExpectation(f.expected, "-innerlink"), }, file{ path: f.path + "-innerlink-abs", - linkTarget: filepath.Join(testdir, dest, "link-target"), + linkTarget: filepath.Join(basedir, "link-target"), expected: mkExpectation(f.expected, "-innerlink-abs"), }, file{ - path: f.path + "-outerlink", - linkTarget: filepath.Join(backtick(f.path), "link-target"), - expected: mkBacktickExpectation(f.expected, "-outerlink"), + path: f.path + "-backlink", + linkTarget: filepath.Join("..", "link-target"), + expected: mkBacklinkExpectation(f.expected, "-backlink"), }, file{ path: f.path + "-outerlink-abs", linkTarget: filepath.Join(testdir, "link-target"), expected: "", }) + + if f.expected != "" { + // outerlink is the number of backticks to escape to testdir + outerlink, _ := filepath.Rel(f.expected, testdir) + links = append(links, file{ + path: f.path + "outerlink", + linkTarget: filepath.Join(outerlink, "link-target"), + expected: "", + }) + } } files = append(files, links...) @@ -820,11 +826,11 @@ func TestUntar(t *testing.T) { file{ path: "nested/again/back-link", linkTarget: "../../nested", - expected: filepath.Join(testdir, dest, "nested/again/back-link"), + expected: filepath.Join(basedir, "nested/again/back-link"), }, file{ path: "nested/again/back-link/../../../back-link-file", - expected: filepath.Join(testdir, dest, "back-link-file"), + expected: filepath.Join(basedir, "back-link-file"), }) // Test chaining back-tick symlinks. @@ -832,20 +838,11 @@ func TestUntar(t *testing.T) { file{ path: "nested/back-link-first", linkTarget: "../", - expected: filepath.Join(testdir, dest, "nested/back-link-first"), + expected: filepath.Join(basedir, "nested/back-link-first"), }, file{ path: "nested/back-link-first/back-link-second", linkTarget: "../", - expected: filepath.Join(testdir, dest, "back-link-second"), - }, - file{ - // 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: "", }) @@ -885,9 +882,11 @@ func TestUntar(t *testing.T) { } tw.Close() - opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard()) + // Capture warnings to stderr for debugging. + output := (*testWriter)(t) + opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output}) - require.NoError(t, opts.untarAll(buf, filepath.Join(testdir, dest), "")) + require.NoError(t, opts.untarAll(buf, filepath.Join(basedir), "")) filepath.Walk(testdir, func(path string, info os.FileInfo, err error) error { if err != nil { @@ -910,10 +909,36 @@ func TestUntar(t *testing.T) { } } -// backtick returns a path to one directory up from the target -func backtick(target string) string { - rel, _ := filepath.Rel(filepath.Dir(target), "../") - return rel +func TestUntar_SingleFile(t *testing.T) { + testdir, err := ioutil.TempDir("", "test-untar") + require.NoError(t, err) + defer os.RemoveAll(testdir) + + dest := filepath.Join(testdir, "target") + + buf := &bytes.Buffer{} + tw := tar.NewWriter(buf) + + const ( + srcName = "source" + content = "file contents" + ) + hdr := &tar.Header{ + Name: srcName, + Mode: 0666, + Size: int64(len(content)), + } + require.NoError(t, tw.WriteHeader(hdr)) + _, err = tw.Write([]byte(content)) + require.NoError(t, err) + tw.Close() + + // Capture warnings to stderr for debugging. + output := (*testWriter)(t) + opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output}) + + require.NoError(t, opts.untarAll(buf, filepath.Join(dest), srcName)) + cmpFileData(t, dest, content) } func createTmpFile(t *testing.T, filepath, data string) { @@ -931,20 +956,14 @@ func createTmpFile(t *testing.T, filepath, data string) { } func cmpFileData(t *testing.T, filePath, data string) { - f, err := os.Open(filePath) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - defer f.Close() - buff := &bytes.Buffer{} - if _, err := io.Copy(buff, f); err != nil { - t.Fatal(err) - } - if err := f.Close(); err != nil { - t.Fatal(err) - } - if data != string(buff.Bytes()) { - t.Fatalf("expected: %s, saw: %s", data, string(buff.Bytes())) - } + actual, err := ioutil.ReadFile(filePath) + require.NoError(t, err) + assert.EqualValues(t, data, actual) +} + +type testWriter testing.T + +func (t *testWriter) Write(p []byte) (n int, err error) { + t.Logf(string(p)) + return len(p), nil }