Improve the extractor and add tests (#8317)

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

pkg/plugin/installer/http_installer.go

@@ -21,10 +21,12 @@ import (
 	"compress/gzip"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"regexp"
 	"strings"
 
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 
 	"helm.sh/helm/v3/internal/third_party/dep/fs"
@@ -118,7 +120,7 @@ func (i *HTTPInstaller) Install() error {
 	}
 
 	if err := i.extractor.Extract(pluginData, i.CacheDir); err != nil {
-		return err
+		return errors.Wrap(err, "extracting files from archive")
 	}
 
 	if !isPlugin(i.CacheDir) {
@@ -148,6 +150,58 @@ func (i HTTPInstaller) Path() string {
 	return helmpath.DataPath("plugins", i.PluginName)
 }
 
+// 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
+}
+
 // Extract extracts compressed archives
 //
 // Implements Extractor.
@@ -171,7 +225,10 @@ func (g *TarGzExtractor) Extract(buffer *bytes.Buffer, targetDir string) error {
 			return err
 		}
 
-		path := filepath.Join(targetDir, header.Name)
+		path, err := cleanJoin(targetDir, header.Name)
+		if err != nil {
+			return err
+		}
 
 		switch header.Typeflag {
 		case tar.TypeDir:

pkg/plugin/installer/http_installer_test.go

@@ -277,3 +277,33 @@ func TestExtract(t *testing.T) {
 	}
 
 }
+
+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)
+		}
+	}
+
+}