From bc890874e1fde06e041cc23b0f57618f04b755ef Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 15 Dec 2020 00:49:35 +0100 Subject: [PATCH] Add safe guards for relative paths Signed-off-by: Hidde Beydals --- controllers/helmchart_controller.go | 10 +++++--- go.mod | 1 + internal/helm/dependency_manager.go | 31 ++++++++++++------------ internal/helm/dependency_manager_test.go | 3 ++- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 431527ff..db312f96 100644 --- a/controllers/helmchart_controller.go +++ b/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() diff --git a/go.mod b/go.mod index 371974e5..14766bcb 100644 --- a/go.mod +++ b/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 diff --git a/internal/helm/dependency_manager.go b/internal/helm/dependency_manager.go index c4427891..55ebc337 100644 --- a/internal/helm/dependency_manager.go +++ b/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://") { - ch, err = chartForLocalDependency(dep, dm.ChartPath) + if strings.HasPrefix(item.Dependency.Repository, "file://") { + 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) { - origPath, err := filepath.Abs(path.Join(cp, strings.TrimPrefix(dep.Repository, "file://"))) +func chartForLocalDependency(dep *helmchart.Dependency, baseDir, chartPath string) (*helmchart.Chart, error) { + 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) { - if chartrepo == nil { - err := fmt.Errorf("chartrepo should not be nil") - return nil, err +func chartForRemoteDependency(dep *helmchart.Dependency, chartRepo *ChartRepository) (*helmchart.Chart, error) { + if chartRepo == nil { + return nil, fmt.Errorf("chartrepo should not be nil") } // 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 } diff --git a/internal/helm/dependency_manager_test.go b/internal/helm/dependency_manager_test.go index cfadaf21..ae948537 100644 --- a/internal/helm/dependency_manager_test.go +++ b/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,