From fe44df8bd4ef4a97c0ebdc722d07657cd4935e6c Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Fri, 12 Apr 2019 18:37:53 -0700 Subject: [PATCH 1/2] Test kubectl cp escape --- pkg/kubectl/cmd/cp/cp_test.go | 159 +++++++++++++++++++++++++++++++++- 1 file changed, 158 insertions(+), 1 deletion(-) diff --git a/pkg/kubectl/cmd/cp/cp_test.go b/pkg/kubectl/cmd/cp/cp_test.go index 3881714552..5aad2359aa 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/require" "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -578,7 +579,6 @@ func TestBadTar(t *testing.T) { t.Errorf("Error finding file: %v", err) } } - } func TestClean(t *testing.T) { @@ -768,6 +768,163 @@ func TestValidate(t *testing.T) { } } +func TestUntar(t *testing.T) { + testdir, err := ioutil.TempDir("", "test-untar") + require.NoError(t, err) + defer os.RemoveAll(testdir) + t.Logf("Test base: %s", testdir) + + const ( + dest = "base" + ) + + type file struct { + path string + linkTarget string // For link types + expected string // Expect to find the file here (or not, if empty) + } + files := []file{{ + // Absolute file within dest + path: filepath.Join(testdir, dest, "abs"), + expected: filepath.Join(testdir, dest, testdir, dest, "abs"), + }, { // Absolute file outside dest + path: filepath.Join(testdir, "abs-out"), + expected: filepath.Join(testdir, dest, 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"), + }, { // Absolute nested file outside dest + path: filepath.Join(testdir, dest, "nested/../../nest-abs-out"), + expected: filepath.Join(testdir, dest, testdir, "nest-abs-out"), + }, { // Relative file inside dest + path: "relative", + expected: filepath.Join(testdir, dest, "relative"), + }, { // Relative file outside dest + path: "../unrelative", + expected: filepath.Join(testdir, dest, "unrelative"), + }, { // 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"), + }} + + mkExpectation := func(expected, suffix string) string { + if expected == "" { + return "" + } + return expected + suffix + } + links := []file{} + for _, f := range files { + links = append(links, file{ + path: f.path + "-innerlink", + linkTarget: "link-target", + expected: mkExpectation(f.expected, "-innerlink"), + }, file{ + path: f.path + "-innerlink-abs", + linkTarget: filepath.Join(testdir, dest, "link-target"), + expected: "", + }, file{ + path: f.path + "-outerlink", + linkTarget: filepath.Join(backtick(f.path), "link-target"), + expected: "", + }, file{ + path: f.path + "-outerlink-abs", + linkTarget: filepath.Join(testdir, "link-target"), + expected: "", + }) + } + files = append(files, links...) + + // Test back-tick escaping through a symlink. + files = append(files, + file{ + path: "nested/again/back-link", + linkTarget: "../../nested", + expected: filepath.Join(testdir, dest, "nested/again/back-link"), + }, + file{ + path: "nested/again/back-link/../../../back-link-file", + expected: filepath.Join(testdir, dest, "back-link-file"), + }) + + // Test chaining back-tick symlinks. + files = append(files, + file{ + path: "nested/back-link-first", + linkTarget: "../", + expected: filepath.Join(testdir, dest, "nested/back-link-first"), + }, + file{ + path: "nested/back-link-first/back-link-second", + linkTarget: "../", + expected: filepath.Join(testdir, dest, "back-link-second"), + }, + file{ + path: "nested/back-link-first/back-link-second/back-link-term", + }) + + buf := &bytes.Buffer{} + tw := tar.NewWriter(buf) + expectations := map[string]bool{} + for _, f := range files { + if f.expected != "" { + expectations[f.expected] = false + } + if f.linkTarget == "" { + hdr := &tar.Header{ + Name: f.path, + Mode: 0666, + 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) + } else { + hdr := &tar.Header{ + Name: f.path, + Mode: int64(0777 | os.ModeSymlink), + Typeflag: tar.TypeSymlink, + Linkname: f.linkTarget, + } + require.NoError(t, tw.WriteHeader(hdr), f.path) + } + } + tw.Close() + + opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard()) + + require.NoError(t, opts.untarAll(buf, filepath.Join(testdir, dest), "")) + + filepath.Walk(testdir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + return nil // Ignore directories. + } + if _, ok := expectations[path]; !ok { + t.Errorf("Unexpected file at %s", path) + } else { + expectations[path] = true + } + return nil + }) + for path, found := range expectations { + if !found { + t.Errorf("Missing expected file %s", path) + } + } +} + +// 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 createTmpFile(t *testing.T, filepath, data string) { f, err := os.Create(filepath) if err != nil { From 8db8423eea628d3696157cf8d3fbc261781a4302 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Tue, 16 Apr 2019 15:49:16 +0200 Subject: [PATCH 2/2] Properly handle links in tar --- pkg/kubectl/cmd/cp/BUILD | 1 + pkg/kubectl/cmd/cp/cp.go | 100 ++++++++++-------- pkg/kubectl/cmd/cp/cp_test.go | 191 ++++++++++++++++------------------ 3 files changed, 148 insertions(+), 144 deletions(-) diff --git a/pkg/kubectl/cmd/cp/BUILD b/pkg/kubectl/cmd/cp/BUILD index 160688835e..77a19b4dc2 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/require:go_default_library", ], ) diff --git a/pkg/kubectl/cmd/cp/cp.go b/pkg/kubectl/cmd/cp/cp.go index e0a624a290..9b34422f38 100644 --- a/pkg/kubectl/cmd/cp/cp.go +++ b/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 -} diff --git a/pkg/kubectl/cmd/cp/cp_test.go b/pkg/kubectl/cmd/cp/cp_test.go index 5aad2359aa..c7d75573d0 100644 --- a/pkg/kubectl/cmd/cp/cp_test.go +++ b/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,