diff --git a/pkg/lifecycle/render/docker/step.go b/pkg/lifecycle/render/docker/step.go index e7b5f1e81..d058cb592 100644 --- a/pkg/lifecycle/render/docker/step.go +++ b/pkg/lifecycle/render/docker/step.go @@ -14,6 +14,8 @@ import ( "github.com/replicatedhq/ship/pkg/images" "github.com/replicatedhq/ship/pkg/lifecycle/render/root" "github.com/replicatedhq/ship/pkg/templates" + "github.com/replicatedhq/ship/pkg/util" + "github.com/spf13/viper" ) @@ -96,6 +98,11 @@ func (p *DefaultStep) Execute( } destIsDockerURL := destinationURL.Scheme == "docker" if !destIsDockerURL { + err = util.IsLegalPath(dest) + if err != nil { + return errors.Wrap(err, "find docker image dest") + } + basePath := filepath.Dir(dest) debug.Log("event", "mkdirall.attempt", "dest", dest, "basePath", basePath) if err := rootFs.MkdirAll(basePath, 0755); err != nil { diff --git a/pkg/lifecycle/render/dockerlayer/layer.go b/pkg/lifecycle/render/dockerlayer/layer.go index 01c29ec37..18d33126d 100644 --- a/pkg/lifecycle/render/dockerlayer/layer.go +++ b/pkg/lifecycle/render/dockerlayer/layer.go @@ -12,6 +12,8 @@ import ( "github.com/replicatedhq/ship/pkg/api" "github.com/replicatedhq/ship/pkg/lifecycle/render/docker" "github.com/replicatedhq/ship/pkg/lifecycle/render/root" + "github.com/replicatedhq/ship/pkg/util" + "github.com/spf13/afero" "github.com/spf13/viper" ) @@ -66,6 +68,11 @@ func (u *Unpacker) Execute( return errors.Wrap(err, "resolve unpack paths") } + err = util.IsLegalPath(basePath) + if err != nil { + return errors.Wrap(err, "write docker layer") + } + debug.Log( "event", "execute", "savePath", savePath, diff --git a/pkg/lifecycle/render/github/render.go b/pkg/lifecycle/render/github/render.go index b5669d4fa..9e4fc6210 100644 --- a/pkg/lifecycle/render/github/render.go +++ b/pkg/lifecycle/render/github/render.go @@ -21,6 +21,7 @@ import ( "github.com/replicatedhq/ship/pkg/specs/gogetter" "github.com/replicatedhq/ship/pkg/state" "github.com/replicatedhq/ship/pkg/templates" + "github.com/replicatedhq/ship/pkg/util" "github.com/spf13/afero" "github.com/spf13/viper" @@ -263,7 +264,14 @@ func getDestPath(githubPath string, asset api.GitHubAsset, builder *templates.Bu } } - return filepath.Join(destDir, githubPath), nil + combinedPath := filepath.Join(destDir, githubPath) + + err = util.IsLegalPath(combinedPath) + if err != nil { + return "", errors.Wrap(err, "write github asset") + } + + return combinedPath, nil } func (r *LocalRenderer) getDestPathNoProxy(asset api.GitHubAsset, builder *templates.Builder, renderRoot string) (string, error) { diff --git a/pkg/lifecycle/render/github/render_test.go b/pkg/lifecycle/render/github/render_test.go index 67b96ea10..cabca2a09 100644 --- a/pkg/lifecycle/render/github/render_test.go +++ b/pkg/lifecycle/render/github/render_test.go @@ -206,6 +206,36 @@ func Test_getDestPath(t *testing.T) { want: "", wantErr: true, }, + { + name: "file in root", + args: args{ + githubPath: "subdir/README.md", + asset: api.GitHubAsset{ + Path: "", + StripPath: "", + AssetShared: api.AssetShared{ + Dest: "/bin/runc", + }, + }, + }, + want: "", + wantErr: true, + }, + { + name: "file in parent dir", + args: args{ + githubPath: "subdir/README.md", + asset: api.GitHubAsset{ + Path: "abc/", + StripPath: "", + AssetShared: api.AssetShared{ + Dest: "../../../bin/runc", + }, + }, + }, + want: "", + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/lifecycle/render/helm/template.go b/pkg/lifecycle/render/helm/template.go index b510ce980..29d373c96 100644 --- a/pkg/lifecycle/render/helm/template.go +++ b/pkg/lifecycle/render/helm/template.go @@ -324,6 +324,11 @@ func (f *LocalTemplater) cleanUpAndOutputRenderedFiles( tempRenderedChartTemplatesDir := path.Join(tempRenderedChartDir, "templates") tempRenderedSubChartsDir := path.Join(tempRenderedChartDir, subChartsDirName) + err := util.IsLegalPath(asset.Dest) + if err != nil { + return errors.Wrap(err, "write helm asset") + } + if f.Viper.GetBool("rm-asset-dest") { debug.Log("event", "baseDir.rm", "path", asset.Dest) if err := f.FS.RemoveAll(asset.Dest); err != nil { diff --git a/pkg/lifecycle/render/inline/render.go b/pkg/lifecycle/render/inline/render.go index 28e371080..f67512a95 100644 --- a/pkg/lifecycle/render/inline/render.go +++ b/pkg/lifecycle/render/inline/render.go @@ -12,6 +12,8 @@ import ( "github.com/replicatedhq/ship/pkg/api" "github.com/replicatedhq/ship/pkg/lifecycle/render/root" "github.com/replicatedhq/ship/pkg/templates" + "github.com/replicatedhq/ship/pkg/util" + "github.com/spf13/viper" ) @@ -65,29 +67,51 @@ func (r *LocalRenderer) Execute( return errors.Wrap(err, "init builder") } - built, err := builder.String(asset.Contents) + builtAsset, err := templateInline(builder, asset) if err != nil { return errors.Wrap(err, "building contents") } + err = util.IsLegalPath(builtAsset.Dest) + if err != nil { + return errors.Wrap(err, "write inline asset") + } + basePath := filepath.Dir(asset.Dest) - debug.Log("event", "mkdirall.attempt", "dest", asset.Dest, "basePath", basePath) + debug.Log("event", "mkdirall.attempt", "dest", builtAsset.Dest, "basePath", basePath) if err := rootFs.MkdirAll(basePath, 0755); err != nil { - debug.Log("event", "mkdirall.fail", "err", err, "dest", asset.Dest, "basePath", basePath) - return errors.Wrapf(err, "write directory to %s", asset.Dest) + debug.Log("event", "mkdirall.fail", "err", err, "dest", builtAsset.Dest, "basePath", basePath) + return errors.Wrapf(err, "write directory to %s", builtAsset.Dest) } mode := os.FileMode(0644) - if asset.Mode != os.FileMode(0) { + if builtAsset.Mode != os.FileMode(0) { debug.Log("event", "applying override permissions") - mode = asset.Mode + mode = builtAsset.Mode } - if err := rootFs.WriteFile(asset.Dest, []byte(built), mode); err != nil { + if err := rootFs.WriteFile(builtAsset.Dest, []byte(builtAsset.Contents), mode); err != nil { debug.Log("event", "execute.fail", "err", err) - return errors.Wrapf(err, "Write inline asset to %s", asset.Dest) + return errors.Wrapf(err, "Write inline asset to %s", builtAsset.Dest) } return nil } } + +func templateInline(builder *templates.Builder, asset api.InlineAsset) (api.InlineAsset, error) { + builtAsset := asset + var err error + + builtAsset.Contents, err = builder.String(asset.Contents) + if err != nil { + return builtAsset, errors.Wrap(err, "building contents") + } + + builtAsset.Dest, err = builder.String(asset.Dest) + if err != nil { + return builtAsset, errors.Wrap(err, "building dest") + } + + return builtAsset, nil +} diff --git a/pkg/lifecycle/render/inline/render_test.go b/pkg/lifecycle/render/inline/render_test.go index 178c6cc0d..a50f02638 100644 --- a/pkg/lifecycle/render/inline/render_test.go +++ b/pkg/lifecycle/render/inline/render_test.go @@ -23,6 +23,7 @@ func TestInlineRender(t *testing.T) { templateContext map[string]interface{} configGroups []libyaml.ConfigGroup expect map[string]interface{} + expectErr bool }{ { name: "happy path", @@ -40,6 +41,52 @@ func TestInlineRender(t *testing.T) { templateContext: map[string]interface{}{}, configGroups: []libyaml.ConfigGroup{}, }, + { + name: "templated dest path", + asset: api.InlineAsset{ + Contents: "hello!", + AssetShared: api.AssetShared{ + Dest: "{{repl if true}}foo.txt{{repl else}}notfoo.txt{{repl end}}", + }, + }, + expect: map[string]interface{}{ + "foo.txt": "hello!", + }, + + meta: api.ReleaseMetadata{}, + templateContext: map[string]interface{}{}, + configGroups: []libyaml.ConfigGroup{}, + }, + { + name: "absolute dest path", + asset: api.InlineAsset{ + Contents: "hello!", + AssetShared: api.AssetShared{ + Dest: "/bin/runc", + }, + }, + expect: map[string]interface{}{}, + + meta: api.ReleaseMetadata{}, + templateContext: map[string]interface{}{}, + configGroups: []libyaml.ConfigGroup{}, + expectErr: true, + }, + { + name: "parent dir dest path", + asset: api.InlineAsset{ + Contents: "hello!", + AssetShared: api.AssetShared{ + Dest: "../../../bin/runc", + }, + }, + expect: map[string]interface{}{}, + + meta: api.ReleaseMetadata{}, + templateContext: map[string]interface{}{}, + configGroups: []libyaml.ConfigGroup{}, + expectErr: true, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -65,7 +112,11 @@ func TestInlineRender(t *testing.T) { test.templateContext, test.configGroups, )(context.Background()) - req.NoError(err) + if !test.expectErr { + req.NoError(err) + } else { + req.Error(err) + } for filename, expectContents := range test.expect { contents, err := rootFs.ReadFile(filename) diff --git a/pkg/lifecycle/render/local/render.go b/pkg/lifecycle/render/local/render.go index 1bfe48658..e7e72d138 100644 --- a/pkg/lifecycle/render/local/render.go +++ b/pkg/lifecycle/render/local/render.go @@ -9,6 +9,8 @@ import ( "github.com/pkg/errors" "github.com/replicatedhq/libyaml" "github.com/replicatedhq/ship/pkg/api" + "github.com/replicatedhq/ship/pkg/util" + "github.com/spf13/afero" ) @@ -47,6 +49,16 @@ func (r *LocalRenderer) Execute( return func(ctx context.Context) error { debug := level.Debug(log.With(r.Logger, "step.type", "render", "render.phase", "execute", "asset.type", "local")) + err := util.IsLegalPath(asset.Dest) + if err != nil { + return errors.Wrap(err, "local asset dest") + } + + err = util.IsLegalPath(asset.Path) + if err != nil { + return errors.Wrap(err, "local asset path") + } + if err := r.Fs.MkdirAll(filepath.Dir(asset.Dest), 0777); err != nil { return errors.Wrapf(err, "mkdir %s", asset.Dest) } diff --git a/pkg/lifecycle/render/web/step.go b/pkg/lifecycle/render/web/step.go index 615ff38be..55bc111c1 100644 --- a/pkg/lifecycle/render/web/step.go +++ b/pkg/lifecycle/render/web/step.go @@ -14,6 +14,8 @@ import ( "github.com/replicatedhq/ship/pkg/api" "github.com/replicatedhq/ship/pkg/lifecycle/render/root" "github.com/replicatedhq/ship/pkg/templates" + "github.com/replicatedhq/ship/pkg/util" + "github.com/spf13/afero" "github.com/spf13/viper" ) @@ -82,6 +84,11 @@ func (p *DefaultStep) Execute( return errors.Wrapf(err, "Build web asset") } + err = util.IsLegalPath(built.Dest) + if err != nil { + return errors.Wrap(err, "write web asset") + } + basePath := filepath.Dir(asset.Dest) debug.Log("event", "mkdirall.attempt", "root", rootFs.RootPath, "dest", built.Dest, "basePath", basePath) if err := rootFs.MkdirAll(basePath, 0755); err != nil { diff --git a/pkg/lifecycle/render/web/step_test.go b/pkg/lifecycle/render/web/step_test.go index 3e7817337..4709bde17 100644 --- a/pkg/lifecycle/render/web/step_test.go +++ b/pkg/lifecycle/render/web/step_test.go @@ -200,6 +200,36 @@ func TestWebStep(t *testing.T) { ExpectFiles: map[string]interface{}{}, ExpectedErr: errors.New("Get web asset from http://foo.bar: received response with status 500"), }, + { + Name: "illegal dest path", + Asset: api.WebAsset{ + AssetShared: api.AssetShared{ + Dest: "/bin/runc", + }, + URL: "http://foo.bar", + }, + RegisterResponders: func() { + httpmock.RegisterResponder("GET", "http://foo.bar", + httpmock.NewStringResponder(200, "hi from foo.bar")) + }, + ExpectFiles: map[string]interface{}{}, + ExpectedErr: errors.Wrap(errors.New("cannot write to an absolute path: /bin/runc"), "write web asset"), + }, + { + Name: "illegal dest path", + Asset: api.WebAsset{ + AssetShared: api.AssetShared{ + Dest: "../../../bin/runc", + }, + URL: "http://foo.bar", + }, + RegisterResponders: func() { + httpmock.RegisterResponder("GET", "http://foo.bar", + httpmock.NewStringResponder(200, "hi from foo.bar")) + }, + ExpectFiles: map[string]interface{}{}, + ExpectedErr: errors.Wrap(errors.New("cannot write to a path that is a parent of the working dir: ../../../bin/runc"), "write web asset"), + }, } for _, test := range tests { diff --git a/pkg/util/legal_path.go b/pkg/util/legal_path.go new file mode 100644 index 000000000..e1247ed1a --- /dev/null +++ b/pkg/util/legal_path.go @@ -0,0 +1,40 @@ +package util + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/pkg/errors" +) + +// IsLegalPath checks if the provided path is a relative path within the current working directory or within the os tempdir. +// If it is not, it returns an error. +func IsLegalPath(path string) error { + + if filepath.IsAbs(path) { + relAbsPath, err := filepath.Rel(os.TempDir(), path) + if err != nil { + return fmt.Errorf("cannot write to an absolute path: %s, got error finding relative path from tempdir: %s", path, err.Error()) + } + + // subdirectories of the os tempdir are fine + if !strings.Contains(relAbsPath, "..") { + return nil + } + + return fmt.Errorf("cannot write to an absolute path: %s", path) + } + + relPath, err := filepath.Rel(".", path) + if err != nil { + return errors.Wrap(err, "find relative path to dest") + } + + if strings.Contains(relPath, "..") { + return fmt.Errorf("cannot write to a path that is a parent of the working dir: %s", relPath) + } + + return nil +} diff --git a/pkg/util/legal_path_test.go b/pkg/util/legal_path_test.go new file mode 100644 index 000000000..6deec8ce1 --- /dev/null +++ b/pkg/util/legal_path_test.go @@ -0,0 +1,48 @@ +package util + +import ( + "os" + "path/filepath" + "testing" +) + +func TestIsLegalPath(t *testing.T) { + tests := []struct { + name string + path string + wantErr bool + }{ + { + name: "relative path", + path: "./happy/path", + wantErr: false, + }, + { + name: "absolute path", + path: "/unhappy/path", + wantErr: true, + }, + { + name: "relative parent path", + path: "../../unhappy/path", + wantErr: true, + }, + { + name: "embedded relative parent path", + path: "./happy/../../../unhappy/path", + wantErr: true, + }, + { + name: "absolute path to tempdir", + path: filepath.Join(os.TempDir(), "mydir"), + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := IsLegalPath(tt.path); (err != nil) != tt.wantErr { + t.Errorf("IsLegalPath() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +}