From 8db8423eea628d3696157cf8d3fbc261781a4302 Mon Sep 17 00:00:00 2001 From: Maciej Szulik Date: Tue, 16 Apr 2019 15:49:16 +0200 Subject: [PATCH] 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,