CVE-2020-15185: Other (NVD-CWE-Other)
value: In Helm before versions 2.16.11 and 3.3.2, a Helm repository can contain duplicates of the same chart, with the last one always used. If a repository is compromised, this lowers the level of access that an attacker needs to inject a bad chart into a repository. To perform this attack, an attacker must have write access to the index file (which can occur during a MITM attack on a non-SSL connection). This issue has been patched in Helm 3.3.2 and 2.16.11. A possible workaround is to manually review the index file in the Helm repository cache before installing software.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 2.7
Published: 2020-09-17T22:15Z
Repository: https://github.com/helm/helm
Language: Go
Files changed: pkg/downloader/chart_downloader_test.go, pkg/repo/index.go, pkg/repo/index_test.go
Commit date: 2020-09-17 12:33:59 -0600
Commit message: Merge pull request from GHSA-jm56-5h66-w453

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
Diff: diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go
index b456143a..b9fd3bf8 100644
--- a/pkg/downloader/chart_downloader_test.go
+++ b/pkg/downloader/chart_downloader_test.go
@@ -69,11 +69,11 @@ func TestResolveChartRef(t *testing.T) {
 		u, err := c.ResolveChartVersion(tt.ref, tt.version)
 		if err != nil {
 			if tt.fail {
 				continue
 			}
-			t.Errorf("%s: failed with error %s", tt.name, err)
+			t.Errorf("%s: failed with error %q", tt.name, err)
 			continue
 		}
 		if got := u.String(); got != tt.expect {
 			t.Errorf("%s: expected %s, got %s", tt.name, tt.expect, got)
 		}
diff --git a/pkg/repo/index.go b/pkg/repo/index.go
index 6ef2cf8b..8b831029 100644
--- a/pkg/repo/index.go
+++ b/pkg/repo/index.go
@@ -226,10 +226,27 @@ type ChartVersion struct {
 	*chart.Metadata
 	URLs    []string  `json:"urls"`
 	Created time.Time `json:"created,omitempty"`
 	Removed bool      `json:"removed,omitempty"`
 	Digest  string    `json:"digest,omitempty"`
+
+	// ChecksumDeprecated is deprecated in Helm 3, and therefore ignored. Helm 3 replaced
+	// this with Digest. However, with a strict YAML parser enabled, a field must be
+	// present on the struct for backwards compatibility.
+	ChecksumDeprecated string `json:"checksum,omitempty"`
+
+	// EngineDeprecated is deprecated in Helm 3, and therefore ignored. However, with a strict
+	// YAML parser enabled, this field must be present.
+	EngineDeprecated string `json:"engine,omitempty"`
+
+	// TillerVersionDeprecated is deprecated in Helm 3, and therefore ignored. However, with a strict
+	// YAML parser enabled, this field must be present.
+	TillerVersionDeprecated string `json:"tillerVersion,omitempty"`
+
+	// URLDeprecated is deprectaed in Helm 3, superseded by URLs. It is ignored. However,
+	// with a strict YAML parser enabled, this must be present on the struct.
+	URLDeprecated string `json:"url,omitempty"`
 }
 
 // IndexDirectory reads a (flat) directory and generates an index.
 //
 // It indexes only charts that have been packaged (*.tgz).
@@ -279,11 +296,11 @@ func IndexDirectory(dir, baseURL string) (*IndexFile, error) {
 // loadIndex loads an index file and does minimal validity checking.
 //
 // This will fail if API Version is not set (ErrNoAPIVersion) or if the unmarshal fails.
 func loadIndex(data []byte) (*IndexFile, error) {
 	i := &IndexFile{}
-	if err := yaml.Unmarshal(data, i); err != nil {
+	if err := yaml.UnmarshalStrict(data, i); err != nil {
 		return i, err
 	}
 	i.SortEntries()
 	if i.APIVersion == "" {
 		return i, ErrNoAPIVersion
diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go
index 466a2c30..77b3a90a 100644
--- a/pkg/repo/index_test.go
+++ b/pkg/repo/index_test.go
@@ -93,10 +93,39 @@ func TestLoadIndex(t *testing.T) {
 		t.Fatal(err)
 	}
 	verifyLocalIndex(t, i)
 }
 
+const indexWithDuplicates = `
+apiVersion: v1
+entries:
+  nginx:
+    - urls:
+        - https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz
+      name: nginx
+      description: string
+      version: 0.2.0
+      home: https://github.com/something/else
+      digest: "sha256:1234567890abcdef"
+  nginx:
+    - urls:
+        - https://kubernetes-charts.storage.googleapis.com/alpine-1.0.0.tgz
+        - http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz
+      name: alpine
+      description: string
+      version: 1.0.0
+      home: https://github.com/something
+      digest: "sha256:1234567890abcdef"
+`
+
+// TestLoadIndex_Duplicates is a regression to make sure that we don't non-deterministically allow duplicate packages.
+func TestLoadIndex_Duplicates(t *testing.T) {
+	if _, err := loadIndex([]byte(indexWithDuplicates)); err == nil {
+		t.Errorf("Expected an error when duplicate entries are present")
+	}
+}
+
 func TestLoadIndexFile(t *testing.T) {
 	i, err := LoadIndexFile(testfile)
 	if err != nil {
 		t.Fatal(err)
 	}

Vulnerable code: 			t.Errorf("%s: failed with error %s", tt.name, err)
	if err := yaml.Unmarshal(data, i); err != nil {
Fixed code: 			t.Errorf("%s: failed with error %q", tt.name, err)

	// ChecksumDeprecated is deprecated in Helm 3, and therefore ignored. Helm 3 replaced
	// this with Digest. However, with a strict YAML parser enabled, a field must be
	// present on the struct for backwards compatibility.
	ChecksumDeprecated string `json:"checksum,omitempty"`

	// EngineDeprecated is deprecated in Helm 3, and therefore ignored. However, with a strict
	// YAML parser enabled, this field must be present.
	EngineDeprecated string `json:"engine,omitempty"`

	// TillerVersionDeprecated is deprecated in Helm 3, and therefore ignored. However, with a strict
	// YAML parser enabled, this field must be present.
	TillerVersionDeprecated string `json:"tillerVersion,omitempty"`

	// URLDeprecated is deprectaed in Helm 3, superseded by URLs. It is ignored. However,
	// with a strict YAML parser enabled, this must be present on the struct.
	URLDeprecated string `json:"url,omitempty"`
	if err := yaml.UnmarshalStrict(data, i); err != nil {
const indexWithDuplicates = `
apiVersion: v1
entries:
  nginx:
    - urls:
        - https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz
      name: nginx
      description: string
      version: 0.2.0
      home: https://github.com/something/else
      digest: "sha256:1234567890abcdef"
  nginx:
    - urls:
        - https://kubernetes-charts.storage.googleapis.com/alpine-1.0.0.tgz
        - http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz
      name: alpine
      description: string
      version: 1.0.0
      home: https://github.com/something
      digest: "sha256:1234567890abcdef"
`

// TestLoadIndex_Duplicates is a regression to make sure that we don't non-deterministically allow duplicate packages.
func TestLoadIndex_Duplicates(t *testing.T) {
	if _, err := loadIndex([]byte(indexWithDuplicates)); err == nil {
		t.Errorf("Expected an error when duplicate entries are present")
	}
}