Merge pull request from GHSA-m9g4-7496-9p6f

Kubectl plugins distributed on Krew have to be packaged as tar or zip archive files.
A bug in krew’s handling of archive files allowed a hand-crafted tar/zip archive with file entries that contain relative or absolute paths in the filenames allowed the file to be written outside the desired extraction directory, hence giving the bad actor to write files to the rest of the user’s filesystem upon installing a plugin.

All Krew versions before v0.3.2 are known to be affected.

This is a low-severity vulnerability since:

- Plugins widely distributed with Krew are hosted in krew-index repository, which is controlled and approved by Krew maintainers.
- Manual validation of the plugin archive files in krew-index reveal no exploitation of this bug.
- Krew validates archive files downloaded with their checksum listed in plugin manifest file, which doesn't allow plugin authors to silently change the underlying archive files without going through a manifest update in the krew-index repository.

Krew will now refuse to unpack an archive if one of the following conditions applies to any archive file name:
- it starts with '/'
- it starts with '\'
- it contains '..'

Author: corneliusweig

pkg/download/downloader.go

@@ -59,6 +59,10 @@ func extractZIP(targetDir string, read io.ReaderAt, size int64) error {
 	}
 
 	for _, f := range zipReader.File {
+		if err := suspiciousPath(f.Name); err != nil {
+			return err
+		}
+
 		path := filepath.Join(targetDir, filepath.FromSlash(f.Name))
 		if f.FileInfo().IsDir() {
 			if err := os.MkdirAll(path, f.Mode()); err != nil {
@@ -119,6 +123,10 @@ func extractTARGZ(targetDir string, at io.ReaderAt, size int64) error {
 			continue
 		}
 
+		if err := suspiciousPath(hdr.Name); err != nil {
+			return err
+		}
+
 		path := filepath.Join(targetDir, filepath.FromSlash(hdr.Name))
 		switch hdr.Typeflag {
 		case tar.TypeDir:
@@ -150,6 +158,18 @@ func extractTARGZ(targetDir string, at io.ReaderAt, size int64) error {
 	return nil
 }
 
+func suspiciousPath(path string) error {
+	if strings.Contains(path, "..") {
+		return errors.Errorf("refusing to unpack archive with suspicious entry %q", path)
+	}
+
+	if strings.HasPrefix(path, `/`) || strings.HasPrefix(path, `\`) {
+		return errors.Errorf("refusing to unpack archive with absolute entry %q", path)
+	}
+
+	return nil
+}
+
 func detectMIMEType(at io.ReaderAt) (string, error) {
 	buf := make([]byte, 512)
 	n, err := at.ReadAt(buf, 0)

pkg/download/downloader_test.go

@@ -15,7 +15,10 @@
 package download
 
 import (
+	"archive/tar"
+	"archive/zip"
 	"bytes"
+	"compress/gzip"
 	"io"
 	"io/ioutil"
 	"os"
@@ -452,3 +455,182 @@ func Test_extractArchive(t *testing.T) {
 		})
 	}
 }
+
+func Test_suspiciousPath(t *testing.T) {
+	tests := []struct {
+		path      string
+		shouldErr bool
+	}{
+		{
+			path:      `/foo`,
+			shouldErr: true,
+		},
+		{
+			path:      `\foo`,
+			shouldErr: true,
+		},
+		{
+			path:      `//foo`,
+			shouldErr: true,
+		},
+		{
+			path:      `/\foo`,
+			shouldErr: true,
+		},
+		{
+			path:      `\\foo`,
+			shouldErr: true,
+		},
+		{
+			path: `./foo`,
+		},
+		{
+			path: `././foo`,
+		},
+		{
+			path: `.//foo`,
+		},
+		{
+			path:      `../foo`,
+			shouldErr: true,
+		},
+		{
+			path:      `a/../foo`,
+			shouldErr: true,
+		},
+		{
+			path: `a/././foo`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.path, func(t *testing.T) {
+			err := suspiciousPath(tt.path)
+			if tt.shouldErr && err == nil {
+				t.Errorf("Expected suspiciousPath to fail")
+			}
+			if !tt.shouldErr && err != nil {
+				t.Errorf("Expected suspiciousPath not to fail, got %s", err)
+			}
+		})
+	}
+}
+
+func Test_extractMaliciousArchive(t *testing.T) {
+	const testContent = "some file content"
+
+	tests := []struct {
+		name string
+		path string
+	}{
+		{
+			name: "absolute file",
+			path: "/foo",
+		},
+		{
+			name: "contains ..",
+			path: "a/../foo",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run("tar.gz  "+tt.name, func(t *testing.T) {
+			tmpDir, cleanup := testutil.NewTempDir(t)
+			defer cleanup()
+
+			// do not use filepath.Join here, because it calls filepath.Clean on the result
+			reader, err := tarGZArchiveForTesting(map[string]string{tt.path: testContent})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			err = extractTARGZ(tmpDir.Root(), reader, reader.Size())
+			if err == nil {
+				t.Errorf("Expected extractTARGZ to fail")
+			} else if !strings.HasPrefix(err.Error(), "refusing to unpack archive") {
+				t.Errorf("Found the wrong error: %s", err)
+			}
+		})
+	}
+
+	for _, tt := range tests {
+		t.Run("zip  "+tt.name, func(t *testing.T) {
+			tmpDir, cleanup := testutil.NewTempDir(t)
+			defer cleanup()
+
+			// do not use filepath.Join here, because it calls filepath.Clean on the result
+			reader, err := zipArchiveReaderForTesting(map[string]string{tt.path: testContent})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			err = extractZIP(tmpDir.Root(), reader, reader.Size())
+			if err == nil {
+				t.Errorf("Expected extractZIP to fail")
+			} else if !strings.HasPrefix(err.Error(), "refusing to unpack archive") {
+				t.Errorf("Found the wrong error: %s", err)
+			}
+		})
+	}
+}
+
+// tarGZArchiveForTesting creates an in-memory zip archive with entries from
+// the files map, where keys are the paths and values are the contents.
+// For example, to create an empty file `a` and another file `b/c`:
+//  tarGZArchiveForTesting(map[string]string{
+//     "a": "",
+//     "b/c": "nested content",
+//  })
+func tarGZArchiveForTesting(files map[string]string) (*bytes.Reader, error) {
+	archiveBuffer := &bytes.Buffer{}
+	gzArchiveBuffer := gzip.NewWriter(archiveBuffer)
+	tw := tar.NewWriter(gzArchiveBuffer)
+	for path, content := range files {
+		header := &tar.Header{
+			Name: path,
+			Size: int64(len(content)),
+			Mode: 0600,
+		}
+		if err := tw.WriteHeader(header); err != nil {
+			return nil, err
+		}
+		if _, err := tw.Write([]byte(content)); err != nil {
+			return nil, err
+		}
+
+	}
+	if err := tw.Close(); err != nil {
+		return nil, err
+	}
+	if err := gzArchiveBuffer.Close(); err != nil {
+		return nil, err
+	}
+	return bytes.NewReader(archiveBuffer.Bytes()), nil
+}
+
+// zipArchiveReaderForTesting creates an in-memory zip archive with entries from
+// the files map, where keys are the paths and values are the contents. Note that
+// entries with empty content just create a directory. The zip spec requires that
+// parent directories are explicitly listed in the archive, so this must be done
+// for nested entries. For example, to create a file at `a/b/c`, you must pass:
+//  map[string]string{"a": "", "a/b": "", "a/b/c": "nested content"}
+func zipArchiveReaderForTesting(files map[string]string) (*bytes.Reader, error) {
+	archiveBuffer := &bytes.Buffer{}
+	zw := zip.NewWriter(archiveBuffer)
+	for path, content := range files {
+		f, err := zw.Create(path)
+		if err != nil {
+			return nil, err
+		}
+		if content == "" {
+			continue
+		}
+		if _, err := f.Write([]byte(content)); err != nil {
+			return nil, err
+		}
+	}
+	if err := zw.Close(); err != nil {
+		return nil, err
+	}
+	return bytes.NewReader(archiveBuffer.Bytes()), nil
+}