Fix possible path traversal attack when supporting Helm values.yaml

docs/user-guide/helm.md

@@ -9,6 +9,8 @@ flag. The flag can be repeated to support multiple values files:
 ```bash
 argocd app set helm-guestbook --values values-production.yaml
 ```
+!!! note
+    Values files must be on the same directory or a subdirectory of the Helm application
 
 ## Helm Parameters
 

reposerver/repository/repository.go

@@ -227,7 +227,7 @@ func helmTemplate(appPath string, q *apiclient.ManifestRequest) ([]*unstructured
 		}
 		templateOpts.Values = appHelm.ValueFiles
 		if appHelm.Values != "" {
-			file, err := ioutil.TempFile("", "values-*.yaml")
+			file, err := ioutil.TempFile(appPath, "values-*.yaml")
 			if err != nil {
 				return nil, err
 			}

reposerver/repository/repository_test.go

@@ -193,6 +193,26 @@ func TestGenerateHelmWithValues(t *testing.T) {
 
 }
 
+// This tests against a path traversal attack. The requested value file (`../minio/values.yaml`) is outside the
+// app path (`./util/helm/testdata/redis`)
+func TestGenerateHelmWithValuesDirectoryTraversal(t *testing.T) {
+	service := newService("../..")
+
+	_, err := service.GenerateManifest(context.Background(), &apiclient.ManifestRequest{
+		Repo:          &argoappv1.Repository{},
+		AppLabelValue: "test",
+		ApplicationSource: &argoappv1.ApplicationSource{
+			Path: "./util/helm/testdata/redis",
+			Helm: &argoappv1.ApplicationSourceHelm{
+				ValueFiles: []string{"../minio/values.yaml"},
+				Values:     `cluster: {slaveCount: 2}`,
+			},
+		},
+	})
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "should be on or under current directory")
+}
+
 func TestGenerateNullList(t *testing.T) {
 	service := newService(".")
 

test/e2e/helm_test.go

@@ -98,7 +98,7 @@ func TestDeclarativeHelmInvalidValuesFile(t *testing.T) {
 		Then().
 		Expect(HealthIs(HealthStatusHealthy)).
 		Expect(SyncStatusIs(SyncStatusCodeUnknown)).
-		Expect(Condition(ApplicationConditionComparisonError, "open does-not-exist-values.yaml: no such file or directory"))
+		Expect(Condition(ApplicationConditionComparisonError, "does-not-exist-values.yaml: no such file or directory"))
 }
 
 func TestHelmRepo(t *testing.T) {

util/helm/cmd.go

@@ -6,8 +6,11 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"regexp"
 
+	"github.com/argoproj/argo-cd/util/security"
+
 	"github.com/argoproj/argo-cd/util"
 
 	argoexec "github.com/argoproj/pkg/exec"
@@ -192,7 +195,18 @@ func (c *Cmd) template(chart string, opts *TemplateOpts) (string, error) {
 		args = append(args, "--set-string", key+"="+cleanSetParameters(val))
 	}
 	for _, val := range opts.Values {
-		args = append(args, "--values", val)
+		absWorkDir, err := filepath.Abs(c.WorkDir)
+		if err != nil {
+			return "", err
+		}
+		if !filepath.IsAbs(val) {
+			val = filepath.Join(absWorkDir, val)
+		}
+		cleanVal, err := security.EnforceToCurrentRoot(absWorkDir, val)
+		if err != nil {
+			return "", err
+		}
+		args = append(args, "--values", cleanVal)
 	}
 
 	return c.run(args...)

util/helm/cmd_test.go

@@ -21,3 +21,27 @@ func TestCmd_template_kubeVersion(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NotEmpty(t, s)
 }
+
+func TestCmd_template_PathTraversal(t *testing.T) {
+	cmd, err := NewCmd("./testdata/redis")
+	assert.NoError(t, err)
+	s, err := cmd.template(".", &TemplateOpts{
+		KubeVersion: "1.14",
+		Values:      []string{"values.yaml"},
+	})
+	assert.NoError(t, err)
+	assert.NotEmpty(t, s)
+
+	_, err = cmd.template(".", &TemplateOpts{
+		KubeVersion: "1.14",
+		Values:      []string{"../minio/values.yaml"},
+	})
+	assert.Error(t, err)
+
+	s, err = cmd.template(".", &TemplateOpts{
+		KubeVersion: "1.14",
+		Values:      []string{"../minio/../redis/values.yaml"},
+	})
+	assert.NoError(t, err)
+	assert.NotEmpty(t, s)
+}

util/security/path_traversal.go

@@ -0,0 +1,35 @@
+package security
+
+import (
+	"fmt"
+	"path/filepath"
+	"strings"
+)
+
+// Ensure that `requestedPath` is on the same directory or any subdirectory of `currentRoot`. Both `currentRoot` and
+// `requestedPath` must be absolute paths. They may contain any number of `./` or `/../` dir changes.
+func EnforceToCurrentRoot(currentRoot, requestedPath string) (string, error) {
+	currentRoot = filepath.Clean(currentRoot)
+	requestedDir, requestedFile := parsePath(requestedPath)
+	if !isRequestedDirUnderCurrentRoot(currentRoot, requestedDir) {
+		return "", fmt.Errorf("requested path %s should be on or under current directory %s", requestedPath, currentRoot)
+	}
+	return requestedDir + string(filepath.Separator) + requestedFile, nil
+}
+
+func isRequestedDirUnderCurrentRoot(currentRoot, requestedDir string) bool {
+	if currentRoot == string(filepath.Separator) {
+		return true
+	} else if currentRoot == requestedDir {
+		return true
+	}
+	return strings.HasPrefix(requestedDir, currentRoot+string(filepath.Separator))
+}
+
+func parsePath(path string) (string, string) {
+	directory := filepath.Dir(path)
+	if directory == path {
+		return directory, ""
+	}
+	return directory, filepath.Base(path)
+}

util/security/path_traversal_test.go

@@ -0,0 +1,26 @@
+package security
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEnforceToCurrentRoot(t *testing.T) {
+	cleanDir, err := EnforceToCurrentRoot("/home/argo/helmapp/", "/home/argo/helmapp/values.yaml")
+	assert.NoError(t, err)
+	assert.Equal(t, "/home/argo/helmapp/values.yaml", cleanDir)
+
+	// File is outside current working directory
+	_, err = EnforceToCurrentRoot("/home/argo/helmapp/", "/home/values.yaml")
+	assert.Error(t, err)
+
+	// File is outside current working directory
+	_, err = EnforceToCurrentRoot("/home/argo/helmapp/", "/home/argo/helmapp/../differentapp/values.yaml")
+	assert.Error(t, err)
+
+	// Goes back and forth, but still legal
+	cleanDir, err = EnforceToCurrentRoot("/home/argo/helmapp/", "/home/argo/helmapp/../../argo/helmapp/values.yaml")
+	assert.NoError(t, err)
+	assert.Equal(t, "/home/argo/helmapp/values.yaml", cleanDir)
+}