Improper sanitization of plugin names in Helm

@@ -59,7 +59,7 @@ func loadPlugins(baseCmd *cobra.Command, out io.Writer) {
 
 	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
 	}
 

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"io"
 
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 
 	"helm.sh/helm/v3/cmd/helm/require"
@@ -81,7 +82,7 @@ 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 {

@@ -20,9 +20,11 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strings"
 
+	"github.com/pkg/errors"
 	"sigs.k8s.io/yaml"
 
 	"helm.sh/helm/v3/pkg/cli"
@@ -157,18 +159,51 @@ func (p *Plugin) PrepareCommand(extraArgs []string) (string, []string, error) {
 	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.
@@ -180,7 +215,7 @@ func LoadAll(basedir string) ([]*Plugin, error) {
 	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 {
@@ -195,7 +230,7 @@ func LoadAll(basedir string) ([]*Plugin, error) {
 		}
 		plugins = append(plugins, p)
 	}
-	return plugins, nil
+	return plugins, detectDuplicates(plugins)
 }
 
 // FindPlugins returns a list of YAML files that describe plugins.

@@ -16,6 +16,7 @@ limitations under the License.
 package plugin // import "helm.sh/helm/v3/pkg/plugin"
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -320,3 +321,51 @@ func TestSetupEnv(t *testing.T) {
 		}
 	}
 }
+
+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",
+	}
+}

@@ -48,7 +48,7 @@ func TestLocalInstaller(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	source := "../testdata/plugdir/echo"
+	source := "../testdata/plugdir/good/echo"
 	i, err := NewForSource(source, "", home)
 	if err != nil {
 		t.Errorf("unexpected error: %s", err)

@@ -61,7 +61,7 @@ func TestVCSInstaller(t *testing.T) {
 	}
 
 	source := "https://github.com/adamreese/helm-env"
-	testRepoPath, _ := filepath.Abs("../testdata/plugdir/echo")
+	testRepoPath, _ := filepath.Abs("../testdata/plugdir/good/echo")
 	repo := &testRepo{
 		local: testRepoPath,
 		tags:  []string{"0.1.0", "0.1.1"},

@@ -22,9 +22,10 @@ import (
 	"path/filepath"
 	"strings"
 
-	helm_env "k8s.io/helm/pkg/helm/environment"
-
 	"github.com/ghodss/yaml"
+	yaml2 "gopkg.in/yaml.v2"
+
+	helm_env "k8s.io/helm/pkg/helm/environment"
 )
 
 const pluginFileName = "plugin.yaml"
@@ -120,12 +121,20 @@ func LoadDir(dirname string) (*Plugin, error) {
 	}
 
 	plug := &Plugin{Dir: dirname}
+	if err := validateMeta(data); err != nil {
+		return nil, err
+	}
 	if err := yaml.Unmarshal(data, &plug.Metadata); err != nil {
 		return nil, err
 	}
 	return plug, nil
 }
 
+func validateMeta(data []byte) error {
+	// This is done ONLY for validation. We need to use ghodss/yaml for the actual parsing.
+	return yaml2.UnmarshalStrict(data, &Metadata{})
+}
+
 // LoadAll loads all plugins found beneath the base directory.
 //
 // This scans only one directory level.

@@ -64,7 +64,7 @@ func TestPrepareCommand(t *testing.T) {
 }
 
 func TestLoadDir(t *testing.T) {
-	dirname := "testdata/plugdir/hello"
+	dirname := "testdata/plugdir/good/hello"
 	plug, err := LoadDir(dirname)
 	if err != nil {
 		t.Fatalf("error loading Hello plugin: %s", err)
@@ -92,8 +92,15 @@ func TestLoadDir(t *testing.T) {
 	}
 }
 
+func TestLoadDirDuplicateEntries(t *testing.T) {
+	dirname := "testdata/plugdir/bad/duplicate-entries"
+	if _, err := LoadDir(dirname); err == nil {
+		t.Errorf("successfully loaded plugin with duplicate entries when it should've failed")
+	}
+}
+
 func TestDownloader(t *testing.T) {
-	dirname := "testdata/plugdir/downloader"
+	dirname := "testdata/plugdir/good/downloader"
 	plug, err := LoadDir(dirname)
 	if err != nil {
 		t.Fatalf("error loading Hello plugin: %s", err)
@@ -131,7 +138,7 @@ func TestLoadAll(t *testing.T) {
 		t.Fatalf("expected empty dir to have 0 plugins")
 	}
 
-	basedir := "testdata/plugdir"
+	basedir := "testdata/plugdir/good"
 	plugs, err := LoadAll(basedir)
 	if err != nil {
 		t.Fatalf("Could not load %q: %s", basedir, err)

@@ -0,0 +1,12 @@
+name: "duplicate-entries"
+version: "0.1.0"
+usage: "usage"
+description: |-
+  description
+command: "echo hello"
+useTunnel: true
+ignoreFlags: true
+hooks:
+  install: "echo installing..."
+hooks:
+  install: "echo installing something different"

@@ -6,6 +6,5 @@ description: |-
 command: "$HELM_PLUGIN_SELF/hello.sh"
 useTunnel: true
 ignoreFlags: true
-install: "echo installing..."
 hooks:
   install: "echo installing..."