Add safe guards for relative paths

Signed-off-by: Hidde Beydals <hello@hidde.co>

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)
@@ -581,8 +584,9 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 		// Construct dependencies for chart if any
 		if len(dwr) > 0 {
 			dm := &helm.DependencyManager{
-				Chart:        helmChart,
+				BaseDir:      tmpDir,
 				ChartPath:    chartPath,
+				Chart:        helmChart,
 				Dependencies: dwr,
 			}
 			err = dm.Build()

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

@@ -20,11 +20,11 @@ import (
 	"context"
 	"fmt"
 	"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"
@@ -39,14 +39,15 @@ type DependencyWithRepository struct {
 
 // DependencyManager manages dependencies for helm charts
 type DependencyManager struct {
-	Chart        *helmchart.Chart
+	BaseDir      string
 	ChartPath    string
+	Chart        *helmchart.Chart
 	Dependencies []*DependencyWithRepository
 }
 
 // Build compiles and builds the chart dependencies
 func (dm *DependencyManager) Build() error {
-	if dm.Dependencies == nil {
+	if len(dm.Dependencies) == 0 {
 		return nil
 	}
 
@@ -54,17 +55,15 @@ func (dm *DependencyManager) Build() error {
 	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://") {
+			if strings.HasPrefix(item.Dependency.Repository, "file://") {
-				ch, err = chartForLocalDependency(dep, dm.ChartPath)
+				ch, err = chartForLocalDependency(item.Dependency, dm.BaseDir, dm.ChartPath)
 			} else {
-				ch, err = chartForRemoteDependency(dep, chartRepo)
+				ch, err = chartForRemoteDependency(item.Dependency, item.Repo)
 			}
 			if err != nil {
 				return err
@@ -77,8 +76,9 @@ func (dm *DependencyManager) Build() error {
 	return errs.Wait()
 }
 
-func chartForLocalDependency(dep *helmchart.Dependency, cp string) (*helmchart.Chart, error) {
+func chartForLocalDependency(dep *helmchart.Dependency, baseDir, chartPath string) (*helmchart.Chart, error) {
-	origPath, err := filepath.Abs(path.Join(cp, strings.TrimPrefix(dep.Repository, "file://")))
+	origPath, err := securejoin.SecureJoin(baseDir,
+		filepath.Join(strings.TrimPrefix(chartPath, baseDir), strings.TrimPrefix(dep.Repository, "file://")))
 	if err != nil {
 		return nil, err
 	}
@@ -114,20 +114,19 @@ func chartForLocalDependency(dep *helmchart.Dependency, cp string) (*helmchart.C
 	return ch, nil
 }
 
-func chartForRemoteDependency(dep *helmchart.Dependency, chartrepo *ChartRepository) (*helmchart.Chart, error) {
+func chartForRemoteDependency(dep *helmchart.Dependency, chartRepo *ChartRepository) (*helmchart.Chart, error) {
-	if chartrepo == nil {
+	if chartRepo == nil {
-		err := fmt.Errorf("chartrepo should not be nil")
+		return nil, fmt.Errorf("chartrepo should not be nil")
-		return nil, err
 	}
 
 	// Lookup the chart version in the chart repository index
-	chartVer, err := chartrepo.Get(dep.Name, dep.Version)
+	chartVer, err := chartRepo.Get(dep.Name, dep.Version)
 	if err != nil {
 		return nil, err
 	}
 
 	// Download chart
-	res, err := chartrepo.DownloadChart(chartVer)
+	res, err := chartRepo.DownloadChart(chartVer)
 	if err != nil {
 		return nil, err
 	}

internal/helm/dependency_manager_test.go

@@ -108,8 +108,9 @@ func TestBuild_WithLocalChart(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			c := chartFixture
 			dm := DependencyManager{
-				Chart:     &c,
+				BaseDir:   "./",
 				ChartPath: "testdata/charts/helmchart",
+				Chart:     &c,
 				Dependencies: []*DependencyWithRepository{
 					{
 						Dependency: &tt.dep,