backported archive improvements from v3 (#8318)

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
(cherry picked from commit def975f5)

pkg/plugin/installer/http_installer.go

@@ -19,14 +19,16 @@ import (
 	"archive/tar"
 	"bytes"
 	"compress/gzip"
+	"errors"
 	"fmt"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"regexp"
 	"strings"
 
-	fp "github.com/cyphar/filepath-securejoin"
+	securejoin "github.com/cyphar/filepath-securejoin"
 
 	"k8s.io/helm/pkg/getter"
 	"k8s.io/helm/pkg/helm/environment"
@@ -184,7 +186,7 @@ func (g *TarGzExtractor) Extract(buffer *bytes.Buffer, targetDir string) error {
 			return err
 		}
 
-		path, err := fp.SecureJoin(targetDir, header.Name)
+		path, err := cleanJoin(targetDir, header.Name)
 		if err != nil {
 			return err
 		}
@@ -212,3 +214,55 @@ func (g *TarGzExtractor) Extract(buffer *bytes.Buffer, targetDir string) error {
 	return nil
 
 }
+
+// CleanJoin resolves dest as a subpath of root.
+//
+// This function runs several security checks on the path, generating an error if
+// the supplied `dest` looks suspicious or would result in dubious behavior on the
+// filesystem.
+//
+// CleanJoin assumes that any attempt by `dest` to break out of the CWD is an attempt
+// to be malicious. (If you don't care about this, use the securejoin-filepath library.)
+// It will emit an error if it detects paths that _look_ malicious, operating on the
+// assumption that we don't actually want to do anything with files that already
+// appear to be nefarious.
+//
+//   - The character `:` is considered illegal because it is a separator on UNIX and a
+//     drive designator on Windows.
+//   - The path component `..` is considered suspicions, and therefore illegal
+//   - The character \ (backslash) is treated as a path separator and is converted to /.
+//   - Beginning a path with a path separator is illegal
+//   - Rudimentary symlink protects are offered by SecureJoin.
+func cleanJoin(root, dest string) (string, error) {
+
+	// On Windows, this is a drive separator. On UNIX-like, this is the path list separator.
+	// In neither case do we want to trust a TAR that contains these.
+	if strings.Contains(dest, ":") {
+		return "", errors.New("path contains ':', which is illegal")
+	}
+
+	// The Go tar library does not convert separators for us.
+	// We assume here, as we do elsewhere, that `\\` means a Windows path.
+	dest = strings.ReplaceAll(dest, "\\", "/")
+
+	// We want to alert the user that something bad was attempted. Cleaning it
+	// is not a good practice.
+	for _, part := range strings.Split(dest, "/") {
+		if part == ".." {
+			return "", errors.New("path contains '..', which is illegal")
+		}
+	}
+
+	// If a path is absolute, the creator of the TAR is doing something shady.
+	if path.IsAbs(dest) {
+		return "", errors.New("path is absolute, which is illegal")
+	}
+
+	// SecureJoin will do some cleaning, as well as some rudimentary checking of symlinks.
+	newpath, err := securejoin.SecureJoin(root, dest)
+	if err != nil {
+		return "", err
+	}
+
+	return filepath.ToSlash(newpath), nil
+}

pkg/plugin/installer/http_installer_test.go

@@ -22,11 +22,12 @@ import (
 	"encoding/base64"
 	"fmt"
 	"io/ioutil"
-	"k8s.io/helm/pkg/helm/helmpath"
 	"os"
 	"path/filepath"
 	"syscall"
 	"testing"
+
+	"k8s.io/helm/pkg/helm/helmpath"
 )
 
 var _ Installer = new(HTTPInstaller)
@@ -222,7 +223,7 @@ func TestExtract(t *testing.T) {
 		Name, Body string
 		Mode       int64
 	}{
-		{"../../plugin.yaml", "sneaky plugin metadata", 0600},
+		{"./plugin.yaml", "sneaky plugin metadata", 0600},
 		{"README.md", "some text", 0777},
 	}
 	for _, file := range files {
@@ -283,3 +284,98 @@ func TestExtract(t *testing.T) {
 	}
 
 }
+
+func TestExtractBadPlugin(t *testing.T) {
+	//create a temp home
+	hh, err := ioutil.TempDir("", "helm-home-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(hh)
+
+	home := helmpath.Home(hh)
+	if err := os.MkdirAll(home.Plugins(), 0755); err != nil {
+		t.Fatalf("Could not create %s: %s", home.Plugins(), err)
+	}
+
+	cacheDir := filepath.Join(home.Cache(), "plugins", "plugin-key")
+	if err := os.MkdirAll(cacheDir, 0755); err != nil {
+		t.Fatalf("Could not create %s: %s", cacheDir, err)
+	}
+
+	syscall.Umask(0000)
+
+	var tarbuf bytes.Buffer
+	tw := tar.NewWriter(&tarbuf)
+	var files = []struct {
+		Name, Body string
+		Mode       int64
+	}{
+		{"../../plugin.yaml", "sneaky plugin metadata", 0600},
+		{"README.md", "some text", 0777},
+	}
+	for _, file := range files {
+		hdr := &tar.Header{
+			Name:     file.Name,
+			Typeflag: tar.TypeReg,
+			Mode:     file.Mode,
+			Size:     int64(len(file.Body)),
+		}
+		if err := tw.WriteHeader(hdr); err != nil {
+			t.Fatal(err)
+		}
+		if _, err := tw.Write([]byte(file.Body)); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if err := tw.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	var buf bytes.Buffer
+	gz := gzip.NewWriter(&buf)
+	if _, err := gz.Write(tarbuf.Bytes()); err != nil {
+		t.Fatal(err)
+	}
+	gz.Close()
+
+	source := "https://repo.localdomain/plugins/fake-plugin-0.0.1.tgz"
+	extr, err := NewExtractor(source)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err = extr.Extract(&buf, cacheDir); err == nil {
+		t.Error("Should have failed because of bad plugin.yaml path")
+	}
+}
+
+func TestCleanJoin(t *testing.T) {
+	for i, fixture := range []struct {
+		path        string
+		expect      string
+		expectError bool
+	}{
+		{"foo/bar.txt", "/tmp/foo/bar.txt", false},
+		{"/foo/bar.txt", "", true},
+		{"./foo/bar.txt", "/tmp/foo/bar.txt", false},
+		{"./././././foo/bar.txt", "/tmp/foo/bar.txt", false},
+		{"../../../../foo/bar.txt", "", true},
+		{"foo/../../../../bar.txt", "", true},
+		{"c:/foo/bar.txt", "/tmp/c:/foo/bar.txt", true},
+		{"foo\\bar.txt", "/tmp/foo/bar.txt", false},
+		{"c:\\foo\\bar.txt", "", true},
+	} {
+		out, err := cleanJoin("/tmp", fixture.path)
+		if err != nil {
+			if !fixture.expectError {
+				t.Errorf("Test %d: Path was not cleaned: %s", i, err)
+			}
+			continue
+		}
+		if fixture.expect != out {
+			t.Errorf("Test %d: Expected %q but got %q", i, fixture.expect, out)
+		}
+	}
+
+}