Merge pull request #6822 from bacongobbler/security-audit

fix(loader): error out when loading irregular files

internal/sympath/walk.go

@@ -21,6 +21,7 @@ limitations under the License.
 package sympath
 
 import (
+	"log"
 	"os"
 	"path/filepath"
 	"sort"
@@ -70,6 +71,7 @@ func symwalk(path string, info os.FileInfo, walkFn filepath.WalkFunc) error {
 		if err != nil {
 			return errors.Wrapf(err, "error evaluating symlink %s", path)
 		}
+		log.Printf("found symbolic link in path: %s resolves to %s", path, resolved)
 		if info, err = os.Lstat(resolved); err != nil {
 			return err
 		}

pkg/chart/loader/directory.go

@@ -17,6 +17,7 @@ limitations under the License.
 package loader
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -91,6 +92,13 @@ func LoadDir(dir string) (*chart.Chart, error) {
 			return nil
 		}
 
+		// Irregular files include devices, sockets, and other uses of files that
+		// are not regular files. In Go they have a file mode type bit set.
+		// See https://golang.org/pkg/os/#FileMode for examples.
+		if !fi.Mode().IsRegular() {
+			return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", name)
+		}
+
 		data, err := ioutil.ReadFile(name)
 		if err != nil {
 			return errors.Wrapf(err, "error reading %s", n)

pkg/chart/loader/load_test.go

@@ -23,6 +23,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -45,6 +46,45 @@ func TestLoadDir(t *testing.T) {
 	verifyDependenciesLock(t, c)
 }
 
+func TestLoadDirWithDevNull(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("test only works on unix systems with /dev/null present")
+	}
+
+	l, err := Loader("testdata/frobnitz_with_dev_null")
+	if err != nil {
+		t.Fatalf("Failed to load testdata: %s", err)
+	}
+	if _, err := l.Load(); err == nil {
+		t.Errorf("packages with an irregular file (/dev/null) should not load")
+	}
+}
+
+func TestLoadDirWithSymlink(t *testing.T) {
+	sym := filepath.Join("..", "LICENSE")
+	link := filepath.Join("testdata", "frobnitz_with_symlink", "LICENSE")
+
+	if err := os.Symlink(sym, link); err != nil {
+		t.Fatal(err)
+	}
+
+	defer os.Remove(link)
+
+	l, err := Loader("testdata/frobnitz_with_symlink")
+	if err != nil {
+		t.Fatalf("Failed to load testdata: %s", err)
+	}
+
+	c, err := l.Load()
+	if err != nil {
+		t.Fatalf("Failed to load testdata: %s", err)
+	}
+	verifyFrobnitz(t, c)
+	verifyChart(t, c)
+	verifyDependencies(t, c)
+	verifyDependenciesLock(t, c)
+}
+
 func TestLoadV1(t *testing.T) {
 	l, err := Loader("testdata/frobnitz.v1")
 	if err != nil {

pkg/chart/loader/testdata/LICENSE

@@ -0,0 +1 @@
+LICENSE placeholder.

pkg/chart/loader/testdata/frobnitz_with_dev_null/.helmignore

@@ -0,0 +1 @@
+ignore/

pkg/chart/loader/testdata/frobnitz_with_dev_null/Chart.lock

@@ -0,0 +1,8 @@
+dependencies:
+  - name: alpine
+    version: "0.1.0"
+    repository: https://example.com/charts
+  - name: mariner
+    version: "4.3.2"
+    repository: https://example.com/charts
+digest: invalid

pkg/chart/loader/testdata/frobnitz_with_dev_null/Chart.yaml

@@ -0,0 +1,27 @@
+apiVersion: v1
+name: frobnitz
+description: This is a frobnitz.
+version: "1.2.3"
+keywords:
+  - frobnitz
+  - sprocket
+  - dodad
+maintainers:
+  - name: The Helm Team
+    email: helm@example.com
+  - name: Someone Else
+    email: nobody@example.com
+sources:
+  - https://example.com/foo/bar
+home: http://example.com
+icon: https://example.com/64x64.png
+annotations:
+  extrakey: extravalue
+  anotherkey: anothervalue
+dependencies:
+  - name: alpine
+    version: "0.1.0"
+    repository: https://example.com/charts
+  - name: mariner
+    version: "4.3.2"
+    repository: https://example.com/charts

pkg/chart/loader/testdata/frobnitz_with_dev_null/INSTALL.txt

@@ -0,0 +1 @@
+This is an install document. The client may display this.

pkg/chart/loader/testdata/frobnitz_with_dev_null/LICENSE

@@ -0,0 +1 @@
+LICENSE placeholder.

pkg/chart/loader/testdata/frobnitz_with_dev_null/README.md

@@ -0,0 +1,11 @@
+# Frobnitz
+
+This is an example chart.
+
+## Usage
+
+This is an example. It has no usage.
+
+## Development
+
+For developer info, see the top-level repository.

pkg/chart/loader/testdata/frobnitz_with_dev_null/charts/_ignore_me

@@ -0,0 +1 @@
+This should be ignored by the loader, but may be included in a chart.

pkg/chart/loader/testdata/frobnitz_with_dev_null/charts/alpine/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+name: alpine
+description: Deploy a basic Alpine Linux pod
+version: 0.1.0
+home: https://helm.sh/helm

pkg/chart/loader/testdata/frobnitz_with_dev_null/charts/alpine/README.md

@@ -0,0 +1,9 @@
+This example was generated using the command `helm create alpine`.
+
+The `templates/` directory contains a very simple pod resource with a
+couple of parameters.
+
+The `values.toml` file contains the default values for the
+`alpine-pod.yaml` template.
+
+You can install this example using `helm install ./alpine`.

pkg/chart/loader/testdata/frobnitz_with_dev_null/charts/alpine/charts/mast1/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+name: mast1
+description: A Helm chart for Kubernetes
+version: 0.1.0
+home: ""

pkg/chart/loader/testdata/frobnitz_with_dev_null/charts/alpine/charts/mast1/values.yaml

@@ -0,0 +1,4 @@
+# Default values for mast1.
+# This is a YAML-formatted file.
+# Declare name/value pairs to be passed into your templates.
+# name = "value"

pkg/chart/loader/testdata/frobnitz_with_dev_null/charts/alpine/templates/alpine-pod.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: {{.Release.Name}}-{{.Chart.Name}}
+  labels:
+    app.kubernetes.io/managed-by: {{.Release.Service}}
+    app.kubernetes.io/name: {{.Chart.Name}}
+    helm.sh/chart: "{{.Chart.Name}}-{{.Chart.Version}}"
+spec:
+  restartPolicy: {{default "Never" .restart_policy}}
+  containers:
+  - name: waiter
+    image: "alpine:3.9"
+    command: ["/bin/sleep","9000"]

pkg/chart/loader/testdata/frobnitz_with_dev_null/charts/alpine/values.yaml

@@ -0,0 +1,2 @@
+# The pod name
+name: "my-alpine"

pkg/chart/loader/testdata/frobnitz_with_dev_null/docs/README.md

@@ -0,0 +1 @@
+This is a placeholder for documentation.

pkg/chart/loader/testdata/frobnitz_with_dev_null/icon.svg

@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<svg xmlns:svg="http://www.w3.org/2000/svg" xmlns="http://www.w3.org/2000/svg"
+    xmlns:xlink="http://www.w3.org/1999/xlink"
+    version="1.0" width="256" height="256" id="test">
+  <desc>Example icon</desc>
+  <rect id="first" x="2" y="2" width="40" height="60" fill="navy"/>
+  <rect id="second" x="15" y="4" width="40" height="60" fill="red"/>
+</svg>

pkg/chart/loader/testdata/frobnitz_with_dev_null/null

@@ -0,0 +1 @@
+/dev/null

pkg/chart/loader/testdata/frobnitz_with_dev_null/templates/template.tpl

@@ -0,0 +1 @@
+Hello {{.Name | default "world"}}

pkg/chart/loader/testdata/frobnitz_with_dev_null/values.yaml

@@ -0,0 +1,6 @@
+# A values file contains configuration.
+
+name: "Some Name"
+
+section:
+  name: "Name in a section"

pkg/chart/loader/testdata/frobnitz_with_symlink/.helmignore

@@ -0,0 +1 @@
+ignore/

pkg/chart/loader/testdata/frobnitz_with_symlink/Chart.lock

@@ -0,0 +1,8 @@
+dependencies:
+  - name: alpine
+    version: "0.1.0"
+    repository: https://example.com/charts
+  - name: mariner
+    version: "4.3.2"
+    repository: https://example.com/charts
+digest: invalid

pkg/chart/loader/testdata/frobnitz_with_symlink/Chart.yaml

@@ -0,0 +1,27 @@
+apiVersion: v1
+name: frobnitz
+description: This is a frobnitz.
+version: "1.2.3"
+keywords:
+  - frobnitz
+  - sprocket
+  - dodad
+maintainers:
+  - name: The Helm Team
+    email: helm@example.com
+  - name: Someone Else
+    email: nobody@example.com
+sources:
+  - https://example.com/foo/bar
+home: http://example.com
+icon: https://example.com/64x64.png
+annotations:
+  extrakey: extravalue
+  anotherkey: anothervalue
+dependencies:
+  - name: alpine
+    version: "0.1.0"
+    repository: https://example.com/charts
+  - name: mariner
+    version: "4.3.2"
+    repository: https://example.com/charts

pkg/chart/loader/testdata/frobnitz_with_symlink/INSTALL.txt

@@ -0,0 +1 @@
+This is an install document. The client may display this.

pkg/chart/loader/testdata/frobnitz_with_symlink/README.md

@@ -0,0 +1,11 @@
+# Frobnitz
+
+This is an example chart.
+
+## Usage
+
+This is an example. It has no usage.
+
+## Development
+
+For developer info, see the top-level repository.

pkg/chart/loader/testdata/frobnitz_with_symlink/charts/_ignore_me

@@ -0,0 +1 @@
+This should be ignored by the loader, but may be included in a chart.

pkg/chart/loader/testdata/frobnitz_with_symlink/charts/alpine/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+name: alpine
+description: Deploy a basic Alpine Linux pod
+version: 0.1.0
+home: https://helm.sh/helm

pkg/chart/loader/testdata/frobnitz_with_symlink/charts/alpine/README.md

@@ -0,0 +1,9 @@
+This example was generated using the command `helm create alpine`.
+
+The `templates/` directory contains a very simple pod resource with a
+couple of parameters.
+
+The `values.toml` file contains the default values for the
+`alpine-pod.yaml` template.
+
+You can install this example using `helm install ./alpine`.

pkg/chart/loader/testdata/frobnitz_with_symlink/charts/alpine/charts/mast1/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+name: mast1
+description: A Helm chart for Kubernetes
+version: 0.1.0
+home: ""

pkg/chart/loader/testdata/frobnitz_with_symlink/charts/alpine/charts/mast1/values.yaml

@@ -0,0 +1,4 @@
+# Default values for mast1.
+# This is a YAML-formatted file.
+# Declare name/value pairs to be passed into your templates.
+# name = "value"

pkg/chart/loader/testdata/frobnitz_with_symlink/charts/alpine/templates/alpine-pod.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: {{.Release.Name}}-{{.Chart.Name}}
+  labels:
+    app.kubernetes.io/managed-by: {{.Release.Service}}
+    app.kubernetes.io/name: {{.Chart.Name}}
+    helm.sh/chart: "{{.Chart.Name}}-{{.Chart.Version}}"
+spec:
+  restartPolicy: {{default "Never" .restart_policy}}
+  containers:
+  - name: waiter
+    image: "alpine:3.9"
+    command: ["/bin/sleep","9000"]

pkg/chart/loader/testdata/frobnitz_with_symlink/charts/alpine/values.yaml

@@ -0,0 +1,2 @@
+# The pod name
+name: "my-alpine"

pkg/chart/loader/testdata/frobnitz_with_symlink/docs/README.md

@@ -0,0 +1 @@
+This is a placeholder for documentation.

pkg/chart/loader/testdata/frobnitz_with_symlink/icon.svg

@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<svg xmlns:svg="http://www.w3.org/2000/svg" xmlns="http://www.w3.org/2000/svg"
+    xmlns:xlink="http://www.w3.org/1999/xlink"
+    version="1.0" width="256" height="256" id="test">
+  <desc>Example icon</desc>
+  <rect id="first" x="2" y="2" width="40" height="60" fill="navy"/>
+  <rect id="second" x="15" y="4" width="40" height="60" fill="red"/>
+</svg>

pkg/chart/loader/testdata/frobnitz_with_symlink/templates/template.tpl

@@ -0,0 +1 @@
+Hello {{.Name | default "world"}}

pkg/chart/loader/testdata/frobnitz_with_symlink/values.yaml

@@ -0,0 +1,6 @@
+# A values file contains configuration.
+
+name: "Some Name"
+
+section:
+  name: "Name in a section"