Merge pull request fluxcd#239 from fluxcd/safe-rel-path

controllers/helmchart_controller.go

@@ -22,11 +22,11 @@ import (
 	"io/ioutil"
 	"net/url"
 	"os"
-	"path"
 	"regexp"
 	"strings"
 	"time"
 
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/go-logr/logr"
 	helmchart "helm.sh/helm/v3/pkg/chart"
@@ -453,7 +453,10 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 	f.Close()
 
 	// Load the chart
-	chartPath := path.Join(tmpDir, chart.Spec.Chart)
+	chartPath, err := securejoin.SecureJoin(tmpDir, chart.Spec.Chart)
+	if err != nil {
+		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
+	}
 	chartFileInfo, err := os.Stat(chartPath)
 	if err != nil {
 		err = fmt.Errorf("chart location read error: %w", err)
@@ -512,7 +515,7 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 			if strings.HasPrefix(dep.Repository, "file://") {
 				dwr = append(dwr, &helm.DependencyWithRepository{
 					Dependency: dep,
-					Repo:       nil,
+					Repository: nil,
 				})
 				continue
 			}
@@ -574,18 +577,19 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 
 			dwr = append(dwr, &helm.DependencyWithRepository{
 				Dependency: dep,
-				Repo:       chartRepo,
+				Repository: chartRepo,
 			})
 		}
 
 		// Construct dependencies for chart if any
 		if len(dwr) > 0 {
 			dm := &helm.DependencyManager{
+				WorkingDir:   tmpDir,
+				ChartPath:    chart.Spec.Chart,
 				Chart:        helmChart,
-				ChartPath:    chartPath,
 				Dependencies: dwr,
 			}
-			err = dm.Build()
+			err = dm.Build(ctx)
 			if err != nil {
 				return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 			}

go.mod

@@ -7,6 +7,7 @@ replace github.com/fluxcd/source-controller/api => ./api
 require (
 	github.com/Masterminds/semver/v3 v3.1.0
 	github.com/blang/semver/v4 v4.0.0
+	github.com/cyphar/filepath-securejoin v0.2.2
 	github.com/fluxcd/pkg/apis/meta v0.5.0
 	github.com/fluxcd/pkg/gittestserver v0.1.0
 	github.com/fluxcd/pkg/helmtestserver v0.1.0

internal/helm/dependency_manager.go

@@ -19,123 +19,145 @@ package helm
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 
 	"github.com/Masterminds/semver/v3"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"golang.org/x/sync/errgroup"
 	helmchart "helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
 )
 
-// DependencyWithRepository is a container for a dependency and its respective
-// repository
+// DependencyWithRepository is a container for a Helm chart dependency
+// and its respective repository.
 type DependencyWithRepository struct {
+	// Dependency holds the reference to a chart.Chart dependency.
 	Dependency *helmchart.Dependency
-	Repo       *ChartRepository
+	// Repository is the ChartRepository the dependency should be
+	// available at and can be downloaded from. If there is none,
+	// a local ('file://') dependency is assumed.
+	Repository *ChartRepository
 }
 
-// DependencyManager manages dependencies for helm charts
+// DependencyManager manages dependencies for a Helm chart.
 type DependencyManager struct {
-	Chart        *helmchart.Chart
-	ChartPath    string
+	// WorkingDir is the chroot path for dependency manager operations,
+	// Dependencies that hold a local (relative) path reference are not
+	// allowed to traverse outside this directory.
+	WorkingDir string
+	// ChartPath is the path of the Chart relative to the WorkingDir,
+	// the combination of the WorkingDir and ChartPath is used to
+	// determine the absolute path of a local dependency.
+	ChartPath string
+	// Chart holds the loaded chart.Chart from the ChartPath.
+	Chart *helmchart.Chart
+	// Dependencies contains a list of dependencies, and the respective
+	// repository the dependency can be found at.
 	Dependencies []*DependencyWithRepository
 }
 
-// Build compiles and builds the chart dependencies
-func (dm *DependencyManager) Build() error {
-	if dm.Dependencies == nil {
+// Build compiles and builds the dependencies of the Chart.
+func (dm *DependencyManager) Build(ctx context.Context) error {
+	if len(dm.Dependencies) == 0 {
 		return nil
 	}
 
-	ctx := context.Background()
 	errs, ctx := errgroup.WithContext(ctx)
-
 	for _, item := range dm.Dependencies {
-		dep := item.Dependency
-		chartRepo := item.Repo
 		errs.Go(func() error {
-			var (
-				ch  *helmchart.Chart
-				err error
-			)
-			if strings.HasPrefix(dep.Repository, "file://") {
-				ch, err = chartForLocalDependency(dep, dm.ChartPath)
-			} else {
-				ch, err = chartForRemoteDependency(dep, chartRepo)
+			select {
+			case <-ctx.Done():
+				return ctx.Err()
+			default:
 			}
-			if err != nil {
-				return err
+
+			var err error
+			switch item.Repository {
+			case nil:
+				err = dm.addLocalDependency(item)
+			default:
+				err = dm.addRemoteDependency(item)
 			}
-			dm.Chart.AddDependency(ch)
-			return nil
+			return err
 		})
 	}
 
 	return errs.Wait()
 }
 
-func chartForLocalDependency(dep *helmchart.Dependency, cp string) (*helmchart.Chart, error) {
-	origPath, err := filepath.Abs(path.Join(cp, strings.TrimPrefix(dep.Repository, "file://")))
+func (dm *DependencyManager) addLocalDependency(dpr *DependencyWithRepository) error {
+	sLocalChartPath, err := dm.secureLocalChartPath(dpr)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	if _, err := os.Stat(origPath); os.IsNotExist(err) {
-		err := fmt.Errorf("chart path %s not found: %w", origPath, err)
-		return nil, err
-	} else if err != nil {
-		return nil, err
+	if _, err := os.Stat(sLocalChartPath); err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("no chart found at '%s' (reference '%s') for dependency '%s'",
+				strings.TrimPrefix(sLocalChartPath, dm.WorkingDir), dpr.Dependency.Repository, dpr.Dependency.Name)
+		}
+		return err
 	}
 
-	ch, err := loader.Load(origPath)
+	ch, err := loader.Load(sLocalChartPath)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	constraint, err := semver.NewConstraint(dep.Version)
+	constraint, err := semver.NewConstraint(dpr.Dependency.Version)
 	if err != nil {
-		err := fmt.Errorf("dependency %s has an invalid version/constraint format: %w", dep.Name, err)
-		return nil, err
+		err := fmt.Errorf("dependency '%s' has an invalid version/constraint format: %w", dpr.Dependency.Name, err)
+		return err
 	}
 
 	v, err := semver.NewVersion(ch.Metadata.Version)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	if !constraint.Check(v) {
-		err = fmt.Errorf("can't get a valid version for dependency %s", dep.Name)
-		return nil, err
+		err = fmt.Errorf("can't get a valid version for dependency '%s'", dpr.Dependency.Name)
+		return err
 	}
 
-	return ch, nil
+	dm.Chart.AddDependency(ch)
+	return nil
 }
 
-func chartForRemoteDependency(dep *helmchart.Dependency, chartrepo *ChartRepository) (*helmchart.Chart, error) {
-	if chartrepo == nil {
-		err := fmt.Errorf("chartrepo should not be nil")
-		return nil, err
+func (dm *DependencyManager) addRemoteDependency(dpr *DependencyWithRepository) error {
+	if dpr.Repository == nil {
+		return fmt.Errorf("no ChartRepository given for '%s' dependency", dpr.Dependency.Name)
 	}
 
-	// Lookup the chart version in the chart repository index
-	chartVer, err := chartrepo.Get(dep.Name, dep.Version)
+	chartVer, err := dpr.Repository.Get(dpr.Dependency.Name, dpr.Dependency.Version)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	// Download chart
-	res, err := chartrepo.DownloadChart(chartVer)
+	res, err := dpr.Repository.DownloadChart(chartVer)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	ch, err := loader.LoadArchive(res)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	return ch, nil
+	dm.Chart.AddDependency(ch)
+	return nil
+}
+
+func (dm *DependencyManager) secureLocalChartPath(dep *DependencyWithRepository) (string, error) {
+	localUrl, err := url.Parse(dep.Dependency.Repository)
+	if err != nil {
+		return "", fmt.Errorf("failed to parse alleged local chart reference: %w", err)
+	}
+	if localUrl.Scheme != "file" {
+		return "", fmt.Errorf("'%s' is not a local chart reference", dep.Dependency.Repository)
+	}
+	return securejoin.SecureJoin(dm.WorkingDir, filepath.Join(dm.ChartPath, localUrl.Host, localUrl.Path))
 }

internal/helm/dependency_manager_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package helm
 
 import (
+	"context"
 	"fmt"
 	"io/ioutil"
 	"strings"
@@ -43,7 +44,7 @@ func TestBuild_WithEmptyDependencies(t *testing.T) {
 	dm := DependencyManager{
 		Dependencies: nil,
 	}
-	if err := dm.Build(); err != nil {
+	if err := dm.Build(context.TODO()); err != nil {
 		t.Errorf("Build() should return nil")
 	}
 }
@@ -72,6 +73,15 @@ func TestBuild_WithLocalChart(t *testing.T) {
 				Repository: chartLocalRepository,
 			},
 		},
+		{
+			name: "allowed traversing path",
+			dep: helmchart.Dependency{
+				Name:       chartName,
+				Alias:      "aliased",
+				Version:    chartVersion,
+				Repository: "file://../../../testdata/charts/helmchartwithdeps/../helmchart",
+			},
+		},
 		{
 			name: "invalid path",
 			dep: helmchart.Dependency{
@@ -80,7 +90,17 @@ func TestBuild_WithLocalChart(t *testing.T) {
 				Repository: "file://../invalid",
 			},
 			wantErr: true,
-			errMsg:  "no such file or directory",
+			errMsg:  "no chart found at",
+		},
+		{
+			name: "illegal traversing path",
+			dep: helmchart.Dependency{
+				Name:       chartName,
+				Version:    chartVersion,
+				Repository: "file://../../../../../controllers/testdata/charts/helmchart",
+			},
+			wantErr: true,
+			errMsg:  "no chart found at",
 		},
 		{
 			name: "invalid version constraint format",
@@ -108,17 +128,18 @@ func TestBuild_WithLocalChart(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			c := chartFixture
 			dm := DependencyManager{
-				Chart:     &c,
-				ChartPath: "testdata/charts/helmchart",
+				WorkingDir: "./",
+				ChartPath:  "testdata/charts/helmchart",
+				Chart:      &c,
 				Dependencies: []*DependencyWithRepository{
 					{
 						Dependency: &tt.dep,
-						Repo:       nil,
+						Repository: nil,
 					},
 				},
 			}
 
-			err := dm.Build()
+			err := dm.Build(context.TODO())
 			deps := dm.Chart.Dependencies()
 
 			if (err != nil) && tt.wantErr {
@@ -130,7 +151,7 @@ func TestBuild_WithLocalChart(t *testing.T) {
 				}
 				return
 			} else if err != nil {
-				t.Errorf("Build() expected to return error")
+				t.Errorf("Build() not expected to return an error: %s", err)
 				return
 			}
 
@@ -166,12 +187,12 @@ func TestBuild_WithRemoteChart(t *testing.T) {
 		Dependencies: []*DependencyWithRepository{
 			{
 				Dependency: &remoteDepFixture,
-				Repo:       cr,
+				Repository: cr,
 			},
 		},
 	}
 
-	if err := dm.Build(); err != nil {
+	if err := dm.Build(context.TODO()); err != nil {
 		t.Errorf("Build() expected to not return error: %s", err)
 	}
 
@@ -187,10 +208,10 @@ func TestBuild_WithRemoteChart(t *testing.T) {
 	}
 
 	// When repo is not set
-	dm.Dependencies[0].Repo = nil
-	if err := dm.Build(); err == nil {
+	dm.Dependencies[0].Repository = nil
+	if err := dm.Build(context.TODO()); err == nil {
 		t.Errorf("Build() expected to return error")
-	} else if !strings.Contains(err.Error(), "chartrepo should not be nil") {
+	} else if !strings.Contains(err.Error(), "is not a local chart reference") {
 		t.Errorf("Build() expected to return different error, got: %s", err)
 	}
 }