Skip to content
Permalink
Browse files

Fix panic in kubectl cp command

  • Loading branch information...
soltysh committed Mar 5, 2019
1 parent b2539d5 commit 2f9ad66eaca587e516d5edd5edd070efc118b31f
Showing with 119 additions and 19 deletions.
  1. +30 −4 pkg/kubectl/cmd/cp.go
  2. +89 −15 pkg/kubectl/cmd/cp_test.go
@@ -296,12 +296,24 @@ func (o *CopyOptions) copyFromPod(src, dest fileSpec) error {
// remove extraneous path shortcuts - these could occur if a path contained extra "../"
// and attempted to navigate beyond "/" in a remote filesystem
prefix = stripPathShortcuts(prefix)
return untarAll(reader, dest.File, prefix)
return o.untarAll(reader, dest.File, prefix)
}

// stripPathShortcuts removes any leading or trailing "../" from a given path
func stripPathShortcuts(p string) string {
newPath := path.Clean(p)
trimmed := strings.TrimPrefix(newPath, "../")

for trimmed != newPath {
newPath = trimmed
trimmed = strings.TrimPrefix(newPath, "../")
}

// trim leftover {".", ".."}
if newPath == "." || newPath == ".." {
newPath = ""
}

if len(newPath) > 0 && string(newPath[0]) == "/" {
return newPath[1:]
}
@@ -389,7 +401,7 @@ func clean(fileName string) string {
return path.Clean(string(os.PathSeparator) + fileName)
}

func untarAll(reader io.Reader, destFile, prefix string) error {
func (o *CopyOptions) untarAll(reader io.Reader, destFile, prefix string) error {
entrySeq := -1

// TODO: use compression here?
@@ -404,6 +416,12 @@ func untarAll(reader io.Reader, destFile, prefix string) error {
}
entrySeq++
mode := header.FileInfo().Mode()
// 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 !strings.HasPrefix(header.Name, prefix) {
return fmt.Errorf("tar contents corrupted")
}
outFileName := path.Join(destFile, clean(header.Name[len(prefix):]))
baseName := path.Dir(outFileName)
if err := os.MkdirAll(baseName, 0755); err != nil {
@@ -428,8 +446,16 @@ func untarAll(reader io.Reader, destFile, prefix string) error {
}

if mode&os.ModeSymlink != 0 {
err := os.Symlink(header.Linkname, outFileName)
if err != nil {
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)
continue
}
if err := os.Symlink(linkname, outFileName); err != nil {
return err
}
} else {
@@ -128,26 +128,32 @@ func TestGetPrefix(t *testing.T) {
}
}

func TestTarUntar(t *testing.T) {
dir, err := ioutil.TempDir("", "input")
dir2, err2 := ioutil.TempDir("", "output")
if err != nil || err2 != nil {
t.Errorf("unexpected error: %v | %v", err, err2)
func checkErr(t *testing.T, err error) {
if err != nil {
t.Errorf("unexpected error: %v", err)
t.FailNow()
}
}

func TestTarUntar(t *testing.T) {
dir, err := ioutil.TempDir("", "input")
checkErr(t, err)
dir2, err := ioutil.TempDir("", "output")
checkErr(t, err)
dir3, err := ioutil.TempDir("", "dir")
checkErr(t, err)

dir = dir + "/"
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)
}
os.RemoveAll(dir)
os.RemoveAll(dir2)
os.RemoveAll(dir3)
}()

files := []struct {
name string
data string
omitted bool
fileType FileType
}{
{
@@ -172,7 +178,24 @@ func TestTarUntar(t *testing.T) {
},
{
name: "gakki",
data: "tmp/gakki",
fileType: SymLink,
},
{
name: "relative_to_dest",
data: path.Join(dir2, "foo"),
fileType: SymLink,
},
{
name: "tricky_relative",
data: path.Join(dir3, "xyz"),
omitted: true,
fileType: SymLink,
},
{
name: "absolute_path",
data: "/tmp/gakki",
omitted: true,
fileType: SymLink,
},
}
@@ -205,13 +228,15 @@ func TestTarUntar(t *testing.T) {

}

opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard())

writer := &bytes.Buffer{}
if err := makeTar(dir, dir, writer); err != nil {
t.Fatalf("unexpected error: %v", err)
}

reader := bytes.NewBuffer(writer.Bytes())
if err := untarAll(reader, dir2, ""); err != nil {
if err := opts.untarAll(reader, dir2, ""); err != nil {
t.Fatalf("unexpected error: %v", err)
}

@@ -238,7 +263,12 @@ func TestTarUntar(t *testing.T) {
}
} else if file.fileType == SymLink {
dest, err := os.Readlink(filePath)

if file.omitted {
if err != nil && strings.Contains(err.Error(), "no such file or directory") {
continue
}
t.Fatalf("expected to omit symlink for %s", filePath)
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
@@ -252,6 +282,48 @@ func TestTarUntar(t *testing.T) {
}
}

func TestTarUntarWrongPrefix(t *testing.T) {
dir, err := ioutil.TempDir("", "input")
checkErr(t, err)
dir2, err := ioutil.TempDir("", "output")
checkErr(t, err)

dir = dir + "/"
defer func() {
os.RemoveAll(dir)
os.RemoveAll(dir2)
}()

filepath := path.Join(dir, "foo")
if err := os.MkdirAll(path.Dir(filepath), 0755); err != nil {
t.Fatalf("unexpected error: %v", err)
}
f, err := os.Create(filepath)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
defer f.Close()
if _, err := io.Copy(f, bytes.NewBuffer([]byte("sample data"))); err != nil {
t.Fatalf("unexpected error: %v", err)
}
if err := f.Close(); err != nil {
t.Fatal(err)
}

opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard())

writer := &bytes.Buffer{}
if err := makeTar(dir, dir, writer); err != nil {
t.Fatalf("unexpected error: %v", err)
}

reader := bytes.NewBuffer(writer.Bytes())
err = opts.untarAll(reader, dir2, "verylongprefix-showing-the-tar-was-tempered-with")
if err == nil || !strings.Contains(err.Error(), "tar contents corrupted") {
t.Fatalf("unexpected error: %v", err)
}
}

// TestCopyToLocalFileOrDir tests untarAll in two cases :
// 1: copy pod file to local file
// 2: copy pod file into local directory
@@ -334,7 +406,8 @@ func TestCopyToLocalFileOrDir(t *testing.T) {
}
defer srcTarFile.Close()

if err := untarAll(srcTarFile, destPath, getPrefix(srcFilePath)); err != nil {
opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard())
if err := opts.untarAll(srcTarFile, destPath, getPrefix(srcFilePath)); err != nil {
t.Errorf("unexpected error: %v", err)
t.FailNow()
}
@@ -481,7 +554,8 @@ func TestBadTar(t *testing.T) {
t.FailNow()
}

if err := untarAll(&buf, dir, "/prefix"); err != nil {
opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard())
if err := opts.untarAll(&buf, dir, "/prefix"); err != nil {
t.Errorf("unexpected error: %v ", err)
t.FailNow()
}

0 comments on commit 2f9ad66

Please sign in to comment.
You can’t perform that action at this time.