CVE-2020-15186: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: In Helm before versions 2.16.11 and 3.3.2 plugin names are not sanitized properly. As a result, a malicious plugin author could use characters in a plugin name that would result in unexpected behavior, such as duplicating the name of another plugin or spoofing the output to `helm --help`. This issue has been patched in Helm 3.3.2. A possible workaround is to not install untrusted Helm plugins. Examine the `name` field in the `plugin.yaml` file for a plugin, looking for characters outside of the [a-zA-Z0-9._-] range.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 2.7
Published: 2020-09-17T22:15Z
Repository: https://github.com/helm/helm
Language: Go
Files changed: cmd/helm/load_plugins.go, cmd/helm/plugin_install.go, pkg/plugin/plugin.go, pkg/plugin/plugin_test.go
Commit date: 2020-09-17 12:35:10 -0600
Commit message: Merge pull request from GHSA-m54r-vrmv-hw33

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
Diff: diff --git a/cmd/helm/load_plugins.go b/cmd/helm/load_plugins.go
index e4aac6c0f..83590210a 100644
--- a/cmd/helm/load_plugins.go
+++ b/cmd/helm/load_plugins.go
@@ -57,11 +57,11 @@ func loadPlugins(baseCmd *cobra.Command, out io.Writer) {
 		return
 	}
 
 	found, err := plugin.FindPlugins(settings.PluginsDirectory)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "failed to load plugins: %s", err)
+		fmt.Fprintf(os.Stderr, "failed to load plugins: %s\n", err)
 		return
 	}
 
 	// Now we create commands for all of these.
 	for _, plug := range found {
diff --git a/cmd/helm/plugin_install.go b/cmd/helm/plugin_install.go
index 183d3dc57..4e8ee327b 100644
--- a/cmd/helm/plugin_install.go
+++ b/cmd/helm/plugin_install.go
@@ -17,10 +17,11 @@ package main
 
 import (
 	"fmt"
 	"io"
 
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 
 	"helm.sh/helm/v3/cmd/helm/require"
 	"helm.sh/helm/v3/pkg/plugin"
 	"helm.sh/helm/v3/pkg/plugin/installer"
@@ -79,11 +80,11 @@ func (o *pluginInstallOptions) run(out io.Writer) error {
 	}
 
 	debug("loading plugin from %s", i.Path())
 	p, err := plugin.LoadDir(i.Path())
 	if err != nil {
-		return err
+		return errors.Wrap(err, "plugin is installed but unusable")
 	}
 
 	if err := runHook(p, plugin.Install); err != nil {
 		return err
 	}
diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go
index caa34fbd3..9bac2244c 100644
--- a/pkg/plugin/plugin.go
+++ b/pkg/plugin/plugin.go
@@ -18,13 +18,15 @@ package plugin // import "helm.sh/helm/v3/pkg/plugin"
 import (
 	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strings"
 
+	"github.com/pkg/errors"
 	"sigs.k8s.io/yaml"
 
 	"helm.sh/helm/v3/pkg/cli"
 )
 
@@ -155,22 +157,55 @@ func (p *Plugin) PrepareCommand(extraArgs []string) (string, []string, error) {
 		baseArgs = append(baseArgs, extraArgs...)
 	}
 	return main, baseArgs, nil
 }
 
+// validPluginName is a regular expression that validates plugin names.
+//
+// Plugin names can only contain the ASCII characters a-z, A-Z, 0-9, ​_​ and ​-.
+var validPluginName = regexp.MustCompile("^[A-Za-z0-9_-]+$")
+
+// validatePluginData validates a plugin's YAML data.
+func validatePluginData(plug *Plugin, filepath string) error {
+	if !validPluginName.MatchString(plug.Metadata.Name) {
+		return fmt.Errorf("invalid plugin name at %q", filepath)
+	}
+	// We could also validate SemVer, executable, and other fields should we so choose.
+	return nil
+}
+
+func detectDuplicates(plugs []*Plugin) error {
+	names := map[string]string{}
+
+	for _, plug := range plugs {
+		if oldpath, ok := names[plug.Metadata.Name]; ok {
+			return fmt.Errorf(
+				"two plugins claim the name %q at %q and %q",
+				plug.Metadata.Name,
+				oldpath,
+				plug.Dir,
+			)
+		}
+		names[plug.Metadata.Name] = plug.Dir
+	}
+
+	return nil
+}
+
 // LoadDir loads a plugin from the given directory.
 func LoadDir(dirname string) (*Plugin, error) {
-	data, err := ioutil.ReadFile(filepath.Join(dirname, PluginFileName))
+	pluginfile := filepath.Join(dirname, PluginFileName)
+	data, err := ioutil.ReadFile(pluginfile)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrapf(err, "failed to read plugin at %q", pluginfile)
 	}
 
 	plug := &Plugin{Dir: dirname}
 	if err := yaml.Unmarshal(data, &plug.Metadata); err != nil {
-		return nil, err
+		return nil, errors.Wrapf(err, "failed to load plugin at %q", pluginfile)
 	}
-	return plug, nil
+	return plug, validatePluginData(plug, pluginfile)
 }
 
 // LoadAll loads all plugins found beneath the base directory.
 //
 // This scans only one directory level.
@@ -178,11 +213,11 @@ func LoadAll(basedir string) ([]*Plugin, error) {
 	plugins := []*Plugin{}
 	// We want basedir/*/plugin.yaml
 	scanpath := filepath.Join(basedir, "*", PluginFileName)
 	matches, err := filepath.Glob(scanpath)
 	if err != nil {
-		return plugins, err
+		return plugins, errors.Wrapf(err, "failed to find plugins in %q", scanpath)
 	}
 
 	if matches == nil {
 		return plugins, nil
 	}
@@ -193,11 +228,11 @@ func LoadAll(basedir string) ([]*Plugin, error) {
 		if err != nil {
 			return plugins, err
 		}
 		plugins = append(plugins, p)
 	}
-	return plugins, nil
+	return plugins, detectDuplicates(plugins)
 }
 
 // FindPlugins returns a list of YAML files that describe plugins.
 func FindPlugins(plugdirs string) ([]*Plugin, error) {
 	found := []*Plugin{}
diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go
index af0b61846..88add037d 100644
--- a/pkg/plugin/plugin_test.go
+++ b/pkg/plugin/plugin_test.go
@@ -14,10 +14,11 @@ limitations under the License.
 */
 
 package plugin // import "helm.sh/helm/v3/pkg/plugin"
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"reflect"
 	"runtime"
 	"testing"
@@ -318,5 +319,53 @@ func TestSetupEnv(t *testing.T) {
 		if got := os.Getenv(tt.name); got != tt.expect {
 			t.Errorf("Expected $%s=%q, got %q", tt.name, tt.expect, got)
 		}
 	}
 }
+
+func TestValidatePluginData(t *testing.T) {
+	for i, item := range []struct {
+		pass bool
+		plug *Plugin
+	}{
+		{true, mockPlugin("abcdefghijklmnopqrstuvwxyz0123456789_-ABC")},
+		{true, mockPlugin("foo-bar-FOO-BAR_1234")},
+		{false, mockPlugin("foo -bar")},
+		{false, mockPlugin("$foo -bar")}, // Test leading chars
+		{false, mockPlugin("foo -bar ")}, // Test trailing chars
+		{false, mockPlugin("foo\nbar")},  // Test newline
+	} {
+		err := validatePluginData(item.plug, fmt.Sprintf("test-%d", i))
+		if item.pass && err != nil {
+			t.Errorf("failed to validate case %d: %s", i, err)
+		} else if !item.pass && err == nil {
+			t.Errorf("expected case %d to fail", i)
+		}
+	}
+}
+
+func TestDetectDuplicates(t *testing.T) {
+	plugs := []*Plugin{
+		mockPlugin("foo"),
+		mockPlugin("bar"),
+	}
+	if err := detectDuplicates(plugs); err != nil {
+		t.Error("no duplicates in the first set")
+	}
+	plugs = append(plugs, mockPlugin("foo"))
+	if err := detectDuplicates(plugs); err == nil {
+		t.Error("duplicates in the second set")
+	}
+}
+
+func mockPlugin(name string) *Plugin {
+	return &Plugin{
+		Metadata: &Metadata{
+			Name:        name,
+			Version:     "v0.1.2",
+			Usage:       "Mock plugin",
+			Description: "Mock plugin for testing",
+			Command:     "echo mock plugin",
+		},
+		Dir: "no-such-dir",
+	}
+}

Vulnerable code: 		fmt.Fprintf(os.Stderr, "failed to load plugins: %s", err)
		return err
	data, err := ioutil.ReadFile(filepath.Join(dirname, PluginFileName))
		return nil, err
		return nil, err
	return plug, nil
		return plugins, err
	return plugins, nil
Fixed code: 		fmt.Fprintf(os.Stderr, "failed to load plugins: %s\n", err)
	"github.com/pkg/errors"
		return errors.Wrap(err, "plugin is installed but unusable")
	"regexp"
	"github.com/pkg/errors"
// validPluginName is a regular expression that validates plugin names.
//
// Plugin names can only contain the ASCII characters a-z, A-Z, 0-9, ​_​ and ​-.
var validPluginName = regexp.MustCompile("^[A-Za-z0-9_-]+$")

// validatePluginData validates a plugin's YAML data.
func validatePluginData(plug *Plugin, filepath string) error {
	if !validPluginName.MatchString(plug.Metadata.Name) {
		return fmt.Errorf("invalid plugin name at %q", filepath)
	}
	// We could also validate SemVer, executable, and other fields should we so choose.
	return nil
}

func detectDuplicates(plugs []*Plugin) error {
	names := map[string]string{}

	for _, plug := range plugs {
		if oldpath, ok := names[plug.Metadata.Name]; ok {
			return fmt.Errorf(
				"two plugins claim the name %q at %q and %q",
				plug.Metadata.Name,
				oldpath,
				plug.Dir,
			)
		}
		names[plug.Metadata.Name] = plug.Dir
	}

	return nil
}

	pluginfile := filepath.Join(dirname, PluginFileName)
	data, err := ioutil.ReadFile(pluginfile)
		return nil, errors.Wrapf(err, "failed to read plugin at %q", pluginfile)
		return nil, errors.Wrapf(err, "failed to load plugin at %q", pluginfile)
	return plug, validatePluginData(plug, pluginfile)
		return plugins, errors.Wrapf(err, "failed to find plugins in %q", scanpath)
	return plugins, detectDuplicates(plugins)
	"fmt"

func TestValidatePluginData(t *testing.T) {
	for i, item := range []struct {
		pass bool
		plug *Plugin
	}{
		{true, mockPlugin("abcdefghijklmnopqrstuvwxyz0123456789_-ABC")},
		{true, mockPlugin("foo-bar-FOO-BAR_1234")},
		{false, mockPlugin("foo -bar")},
		{false, mockPlugin("$foo -bar")}, // Test leading chars
		{false, mockPlugin("foo -bar ")}, // Test trailing chars
		{false, mockPlugin("foo\nbar")},  // Test newline
	} {
		err := validatePluginData(item.plug, fmt.Sprintf("test-%d", i))
		if item.pass && err != nil {
			t.Errorf("failed to validate case %d: %s", i, err)
		} else if !item.pass && err == nil {
			t.Errorf("expected case %d to fail", i)
		}
	}
}

func TestDetectDuplicates(t *testing.T) {
	plugs := []*Plugin{
		mockPlugin("foo"),
		mockPlugin("bar"),
	}
	if err := detectDuplicates(plugs); err != nil {
		t.Error("no duplicates in the first set")
	}
	plugs = append(plugs, mockPlugin("foo"))
	if err := detectDuplicates(plugs); err == nil {
		t.Error("duplicates in the second set")
	}
}

func mockPlugin(name string) *Plugin {
	return &Plugin{
		Metadata: &Metadata{
			Name:        name,
			Version:     "v0.1.2",
			Usage:       "Mock plugin",
			Description: "Mock plugin for testing",
			Command:     "echo mock plugin",
		},
		Dir: "no-such-dir",
	}
}