From 8649b4d9ebad4e3b6ae01619585237ec5863b92b 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/BUILD | 1 + pkg/kubectl/cmd/cp.go | 100 ++++++++------ pkg/kubectl/cmd/cp_test.go | 273 +++++++++++++++++++++---------------- 3 files changed, 209 insertions(+), 165 deletions(-) diff --git a/pkg/kubectl/cmd/BUILD b/pkg/kubectl/cmd/BUILD index 885485458dc3..7d1727447f9b 100644 --- a/pkg/kubectl/cmd/BUILD +++ b/pkg/kubectl/cmd/BUILD @@ -234,6 +234,7 @@ go_test( "//staging/src/k8s.io/metrics/pkg/client/clientset/versioned/fake:go_default_library", "//vendor/github.com/googleapis/gnostic/OpenAPIv2:go_default_library", "//vendor/github.com/spf13/cobra:go_default_library", + "//vendor/github.com/stretchr/testify/require:go_default_library", "//vendor/gopkg.in/yaml.v2:go_default_library", "//vendor/k8s.io/utils/exec:go_default_library", ], diff --git a/pkg/kubectl/cmd/cp.go b/pkg/kubectl/cmd/cp.go index 768c6b71d679..f4c945499e86 100644 --- a/pkg/kubectl/cmd/cp.go +++ b/pkg/kubectl/cmd/cp.go @@ -401,9 +401,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 { @@ -414,52 +412,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 } @@ -473,14 +479,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, "/") @@ -507,15 +531,3 @@ func (o *CopyOptions) execute(options *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_test.go b/pkg/kubectl/cmd/cp_test.go index 4e414654341b..0615163bf4ad 100644 --- a/pkg/kubectl/cmd/cp_test.go +++ b/pkg/kubectl/cmd/cp_test.go @@ -19,17 +19,18 @@ package cmd import ( "archive/tar" "bytes" + "fmt" "io" "io/ioutil" "net/http" "os" - "os/exec" "path" "path/filepath" "strings" "testing" "github.com/stretchr/testify/require" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -128,6 +129,128 @@ func TestGetPrefix(t *testing.T) { } } +func TestStripPathShortcuts(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "test single path shortcut prefix", + input: "../foo/bar", + expected: "foo/bar", + }, + { + name: "test multiple path shortcuts", + input: "../../foo/bar", + expected: "foo/bar", + }, + { + name: "test multiple path shortcuts with absolute path", + input: "/tmp/one/two/../../foo/bar", + expected: "tmp/foo/bar", + }, + { + name: "test multiple path shortcuts with no named directory", + input: "../../", + expected: "", + }, + { + name: "test multiple path shortcuts with no named directory and no trailing slash", + input: "../..", + expected: "", + }, + { + name: "test multiple path shortcuts with absolute path and filename containing leading dots", + input: "/tmp/one/two/../../foo/..bar", + expected: "tmp/foo/..bar", + }, + { + name: "test multiple path shortcuts with no named directory and filename containing leading dots", + input: "../...foo", + expected: "...foo", + }, + { + name: "test filename containing leading dots", + input: "...foo", + expected: "...foo", + }, + { + name: "test root directory", + input: "/", + expected: "", + }, + } + + for _, test := range tests { + out := stripPathShortcuts(test.input) + if out != test.expected { + t.Errorf("expected: %s, saw: %s", test.expected, out) + } + } +} +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 { t.Errorf("unexpected error: %v", err) @@ -324,118 +447,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() - } - srcFile, err := os.Create(srcFilePath) - if err != nil { - t.Errorf("unexpected error: %v", err) - t.FailNow() - } - defer srcFile.Close() - - if _, err := io.Copy(srcFile, bytes.NewBuffer([]byte(file.data))); err != nil { - t.Errorf("unexpected error: %v", err) - t.FailNow() - } - 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) - } - destFile, err := os.Open(actualDestFilePath) - if err != nil { - t.Errorf("unexpected error: %v", err) - t.FailNow() - } - defer destFile.Close() - buff := &bytes.Buffer{} - io.Copy(buff, destFile) - if file.data != string(buff.Bytes()) { - t.Errorf("expected: %s, actual: %s", file.data, string(buff.Bytes())) - } - }() - } - -} - func TestTarDestinationName(t *testing.T) { dir, err := ioutil.TempDir(os.TempDir(), "input") dir2, err2 := ioutil.TempDir(os.TempDir(), "output") @@ -531,7 +542,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 { @@ -726,13 +736,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 { @@ -741,6 +751,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{ @@ -750,11 +767,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"), @@ -788,7 +805,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{} @@ -805,8 +834,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,