Skip to content
This repository was archived by the owner on Mar 24, 2023. It is now read-only.

Commit 80d5281

Browse files
laveryalaverya
authored
Merge pull request #842 from laverya/block-writing-to-parent-dir
Prevent assets from writing files outside of the dir ship is run in
2 parents 8ca05ba + 63da032 commit 80d5281

File tree

12 files changed

+279
-10
lines changed

12 files changed

+279
-10
lines changed

‎pkg/lifecycle/render/docker/step.go

+7
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import (
1414
"github.com/replicatedhq/ship/pkg/images"
1515
"github.com/replicatedhq/ship/pkg/lifecycle/render/root"
1616
"github.com/replicatedhq/ship/pkg/templates"
17+
"github.com/replicatedhq/ship/pkg/util"
18+
1719
"github.com/spf13/viper"
1820
)
1921

@@ -96,6 +98,11 @@ func (p *DefaultStep) Execute(
9698
}
9799
destIsDockerURL := destinationURL.Scheme == "docker"
98100
if !destIsDockerURL {
101+
err = util.IsLegalPath(dest)
102+
if err != nil {
103+
return errors.Wrap(err, "find docker image dest")
104+
}
105+
99106
basePath := filepath.Dir(dest)
100107
debug.Log("event", "mkdirall.attempt", "dest", dest, "basePath", basePath)
101108
if err := rootFs.MkdirAll(basePath, 0755); err != nil {

‎pkg/lifecycle/render/dockerlayer/layer.go

+7
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
"github.com/replicatedhq/ship/pkg/api"
1313
"github.com/replicatedhq/ship/pkg/lifecycle/render/docker"
1414
"github.com/replicatedhq/ship/pkg/lifecycle/render/root"
15+
"github.com/replicatedhq/ship/pkg/util"
16+
1517
"github.com/spf13/afero"
1618
"github.com/spf13/viper"
1719
)
@@ -66,6 +68,11 @@ func (u *Unpacker) Execute(
6668
return errors.Wrap(err, "resolve unpack paths")
6769
}
6870

71+
err = util.IsLegalPath(basePath)
72+
if err != nil {
73+
return errors.Wrap(err, "write docker layer")
74+
}
75+
6976
debug.Log(
7077
"event", "execute",
7178
"savePath", savePath,

‎pkg/lifecycle/render/github/render.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/replicatedhq/ship/pkg/specs/gogetter"
2222
"github.com/replicatedhq/ship/pkg/state"
2323
"github.com/replicatedhq/ship/pkg/templates"
24+
"github.com/replicatedhq/ship/pkg/util"
2425

2526
"github.com/spf13/afero"
2627
"github.com/spf13/viper"
@@ -263,7 +264,14 @@ func getDestPath(githubPath string, asset api.GitHubAsset, builder *templates.Bu
263264
}
264265
}
265266

266-
return filepath.Join(destDir, githubPath), nil
267+
combinedPath := filepath.Join(destDir, githubPath)
268+
269+
err = util.IsLegalPath(combinedPath)
270+
if err != nil {
271+
return "", errors.Wrap(err, "write github asset")
272+
}
273+
274+
return combinedPath, nil
267275
}
268276

269277
func (r *LocalRenderer) getDestPathNoProxy(asset api.GitHubAsset, builder *templates.Builder, renderRoot string) (string, error) {

‎pkg/lifecycle/render/github/render_test.go

+30
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,36 @@ func Test_getDestPath(t *testing.T) {
206206
want: "",
207207
wantErr: true,
208208
},
209+
{
210+
name: "file in root",
211+
args: args{
212+
githubPath: "subdir/README.md",
213+
asset: api.GitHubAsset{
214+
Path: "",
215+
StripPath: "",
216+
AssetShared: api.AssetShared{
217+
Dest: "/bin/runc",
218+
},
219+
},
220+
},
221+
want: "",
222+
wantErr: true,
223+
},
224+
{
225+
name: "file in parent dir",
226+
args: args{
227+
githubPath: "subdir/README.md",
228+
asset: api.GitHubAsset{
229+
Path: "abc/",
230+
StripPath: "",
231+
AssetShared: api.AssetShared{
232+
Dest: "../../../bin/runc",
233+
},
234+
},
235+
},
236+
want: "",
237+
wantErr: true,
238+
},
209239
}
210240
for _, tt := range tests {
211241
t.Run(tt.name, func(t *testing.T) {

‎pkg/lifecycle/render/helm/template.go

+5
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,11 @@ func (f *LocalTemplater) cleanUpAndOutputRenderedFiles(
324324
tempRenderedChartTemplatesDir := path.Join(tempRenderedChartDir, "templates")
325325
tempRenderedSubChartsDir := path.Join(tempRenderedChartDir, subChartsDirName)
326326

327+
err := util.IsLegalPath(asset.Dest)
328+
if err != nil {
329+
return errors.Wrap(err, "write helm asset")
330+
}
331+
327332
if f.Viper.GetBool("rm-asset-dest") {
328333
debug.Log("event", "baseDir.rm", "path", asset.Dest)
329334
if err := f.FS.RemoveAll(asset.Dest); err != nil {

‎pkg/lifecycle/render/inline/render.go

+32-8
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
"github.com/replicatedhq/ship/pkg/api"
1313
"github.com/replicatedhq/ship/pkg/lifecycle/render/root"
1414
"github.com/replicatedhq/ship/pkg/templates"
15+
"github.com/replicatedhq/ship/pkg/util"
16+
1517
"github.com/spf13/viper"
1618
)
1719

@@ -65,29 +67,51 @@ func (r *LocalRenderer) Execute(
6567
return errors.Wrap(err, "init builder")
6668
}
6769

68-
built, err := builder.String(asset.Contents)
70+
builtAsset, err := templateInline(builder, asset)
6971
if err != nil {
7072
return errors.Wrap(err, "building contents")
7173
}
7274

75+
err = util.IsLegalPath(builtAsset.Dest)
76+
if err != nil {
77+
return errors.Wrap(err, "write inline asset")
78+
}
79+
7380
basePath := filepath.Dir(asset.Dest)
74-
debug.Log("event", "mkdirall.attempt", "dest", asset.Dest, "basePath", basePath)
81+
debug.Log("event", "mkdirall.attempt", "dest", builtAsset.Dest, "basePath", basePath)
7582
if err := rootFs.MkdirAll(basePath, 0755); err != nil {
76-
debug.Log("event", "mkdirall.fail", "err", err, "dest", asset.Dest, "basePath", basePath)
77-
return errors.Wrapf(err, "write directory to %s", asset.Dest)
83+
debug.Log("event", "mkdirall.fail", "err", err, "dest", builtAsset.Dest, "basePath", basePath)
84+
return errors.Wrapf(err, "write directory to %s", builtAsset.Dest)
7885
}
7986

8087
mode := os.FileMode(0644)
81-
if asset.Mode != os.FileMode(0) {
88+
if builtAsset.Mode != os.FileMode(0) {
8289
debug.Log("event", "applying override permissions")
83-
mode = asset.Mode
90+
mode = builtAsset.Mode
8491
}
8592

86-
if err := rootFs.WriteFile(asset.Dest, []byte(built), mode); err != nil {
93+
if err := rootFs.WriteFile(builtAsset.Dest, []byte(builtAsset.Contents), mode); err != nil {
8794
debug.Log("event", "execute.fail", "err", err)
88-
return errors.Wrapf(err, "Write inline asset to %s", asset.Dest)
95+
return errors.Wrapf(err, "Write inline asset to %s", builtAsset.Dest)
8996
}
9097
return nil
9198

9299
}
93100
}
101+
102+
func templateInline(builder *templates.Builder, asset api.InlineAsset) (api.InlineAsset, error) {
103+
builtAsset := asset
104+
var err error
105+
106+
builtAsset.Contents, err = builder.String(asset.Contents)
107+
if err != nil {
108+
return builtAsset, errors.Wrap(err, "building contents")
109+
}
110+
111+
builtAsset.Dest, err = builder.String(asset.Dest)
112+
if err != nil {
113+
return builtAsset, errors.Wrap(err, "building dest")
114+
}
115+
116+
return builtAsset, nil
117+
}

‎pkg/lifecycle/render/inline/render_test.go

+52-1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ func TestInlineRender(t *testing.T) {
2323
templateContext map[string]interface{}
2424
configGroups []libyaml.ConfigGroup
2525
expect map[string]interface{}
26+
expectErr bool
2627
}{
2728
{
2829
name: "happy path",
@@ -40,6 +41,52 @@ func TestInlineRender(t *testing.T) {
4041
templateContext: map[string]interface{}{},
4142
configGroups: []libyaml.ConfigGroup{},
4243
},
44+
{
45+
name: "templated dest path",
46+
asset: api.InlineAsset{
47+
Contents: "hello!",
48+
AssetShared: api.AssetShared{
49+
Dest: "{{repl if true}}foo.txt{{repl else}}notfoo.txt{{repl end}}",
50+
},
51+
},
52+
expect: map[string]interface{}{
53+
"foo.txt": "hello!",
54+
},
55+
56+
meta: api.ReleaseMetadata{},
57+
templateContext: map[string]interface{}{},
58+
configGroups: []libyaml.ConfigGroup{},
59+
},
60+
{
61+
name: "absolute dest path",
62+
asset: api.InlineAsset{
63+
Contents: "hello!",
64+
AssetShared: api.AssetShared{
65+
Dest: "/bin/runc",
66+
},
67+
},
68+
expect: map[string]interface{}{},
69+
70+
meta: api.ReleaseMetadata{},
71+
templateContext: map[string]interface{}{},
72+
configGroups: []libyaml.ConfigGroup{},
73+
expectErr: true,
74+
},
75+
{
76+
name: "parent dir dest path",
77+
asset: api.InlineAsset{
78+
Contents: "hello!",
79+
AssetShared: api.AssetShared{
80+
Dest: "../../../bin/runc",
81+
},
82+
},
83+
expect: map[string]interface{}{},
84+
85+
meta: api.ReleaseMetadata{},
86+
templateContext: map[string]interface{}{},
87+
configGroups: []libyaml.ConfigGroup{},
88+
expectErr: true,
89+
},
4390
}
4491
for _, test := range tests {
4592
t.Run(test.name, func(t *testing.T) {
@@ -65,7 +112,11 @@ func TestInlineRender(t *testing.T) {
65112
test.templateContext,
66113
test.configGroups,
67114
)(context.Background())
68-
req.NoError(err)
115+
if !test.expectErr {
116+
req.NoError(err)
117+
} else {
118+
req.Error(err)
119+
}
69120

70121
for filename, expectContents := range test.expect {
71122
contents, err := rootFs.ReadFile(filename)

‎pkg/lifecycle/render/local/render.go

+12
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import (
99
"github.com/pkg/errors"
1010
"github.com/replicatedhq/libyaml"
1111
"github.com/replicatedhq/ship/pkg/api"
12+
"github.com/replicatedhq/ship/pkg/util"
13+
1214
"github.com/spf13/afero"
1315
)
1416

@@ -47,6 +49,16 @@ func (r *LocalRenderer) Execute(
4749
return func(ctx context.Context) error {
4850
debug := level.Debug(log.With(r.Logger, "step.type", "render", "render.phase", "execute", "asset.type", "local"))
4951

52+
err := util.IsLegalPath(asset.Dest)
53+
if err != nil {
54+
return errors.Wrap(err, "local asset dest")
55+
}
56+
57+
err = util.IsLegalPath(asset.Path)
58+
if err != nil {
59+
return errors.Wrap(err, "local asset path")
60+
}
61+
5062
if err := r.Fs.MkdirAll(filepath.Dir(asset.Dest), 0777); err != nil {
5163
return errors.Wrapf(err, "mkdir %s", asset.Dest)
5264
}

‎pkg/lifecycle/render/web/step.go

+7
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import (
1414
"github.com/replicatedhq/ship/pkg/api"
1515
"github.com/replicatedhq/ship/pkg/lifecycle/render/root"
1616
"github.com/replicatedhq/ship/pkg/templates"
17+
"github.com/replicatedhq/ship/pkg/util"
18+
1719
"github.com/spf13/afero"
1820
"github.com/spf13/viper"
1921
)
@@ -82,6 +84,11 @@ func (p *DefaultStep) Execute(
8284
return errors.Wrapf(err, "Build web asset")
8385
}
8486

87+
err = util.IsLegalPath(built.Dest)
88+
if err != nil {
89+
return errors.Wrap(err, "write web asset")
90+
}
91+
8592
basePath := filepath.Dir(asset.Dest)
8693
debug.Log("event", "mkdirall.attempt", "root", rootFs.RootPath, "dest", built.Dest, "basePath", basePath)
8794
if err := rootFs.MkdirAll(basePath, 0755); err != nil {

‎pkg/lifecycle/render/web/step_test.go

+30
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,36 @@ func TestWebStep(t *testing.T) {
200200
ExpectFiles: map[string]interface{}{},
201201
ExpectedErr: errors.New("Get web asset from http://foo.bar: received response with status 500"),
202202
},
203+
{
204+
Name: "illegal dest path",
205+
Asset: api.WebAsset{
206+
AssetShared: api.AssetShared{
207+
Dest: "/bin/runc",
208+
},
209+
URL: "http://foo.bar",
210+
},
211+
RegisterResponders: func() {
212+
httpmock.RegisterResponder("GET", "http://foo.bar",
213+
httpmock.NewStringResponder(200, "hi from foo.bar"))
214+
},
215+
ExpectFiles: map[string]interface{}{},
216+
ExpectedErr: errors.Wrap(errors.New("cannot write to an absolute path: /bin/runc"), "write web asset"),
217+
},
218+
{
219+
Name: "illegal dest path",
220+
Asset: api.WebAsset{
221+
AssetShared: api.AssetShared{
222+
Dest: "../../../bin/runc",
223+
},
224+
URL: "http://foo.bar",
225+
},
226+
RegisterResponders: func() {
227+
httpmock.RegisterResponder("GET", "http://foo.bar",
228+
httpmock.NewStringResponder(200, "hi from foo.bar"))
229+
},
230+
ExpectFiles: map[string]interface{}{},
231+
ExpectedErr: errors.Wrap(errors.New("cannot write to a path that is a parent of the working dir: ../../../bin/runc"), "write web asset"),
232+
},
203233
}
204234

205235
for _, test := range tests {

‎pkg/util/legal_path.go

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package util
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"path/filepath"
7+
"strings"
8+
9+
"github.com/pkg/errors"
10+
)
11+
12+
// IsLegalPath checks if the provided path is a relative path within the current working directory or within the os tempdir.
13+
// If it is not, it returns an error.
14+
func IsLegalPath(path string) error {
15+
16+
if filepath.IsAbs(path) {
17+
relAbsPath, err := filepath.Rel(os.TempDir(), path)
18+
if err != nil {
19+
return fmt.Errorf("cannot write to an absolute path: %s, got error finding relative path from tempdir: %s", path, err.Error())
20+
}
21+
22+
// subdirectories of the os tempdir are fine
23+
if !strings.Contains(relAbsPath, "..") {
24+
return nil
25+
}
26+
27+
return fmt.Errorf("cannot write to an absolute path: %s", path)
28+
}
29+
30+
relPath, err := filepath.Rel(".", path)
31+
if err != nil {
32+
return errors.Wrap(err, "find relative path to dest")
33+
}
34+
35+
if strings.Contains(relPath, "..") {
36+
return fmt.Errorf("cannot write to a path that is a parent of the working dir: %s", relPath)
37+
}
38+
39+
return nil
40+
}

0 commit comments

Comments
 (0)