From 2c4d212b85f2cbe7e282297b9761755e74e2df3a Mon Sep 17 00:00:00 2001 From: Andrew Jeddeloh Date: Wed, 5 Dec 2018 13:39:53 -0800 Subject: [PATCH 1/3] exec/util: don't follow links for last path elem --- internal/exec/util/util.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/exec/util/util.go b/internal/exec/util/util.go index be600eb83..9c2a6c112 100644 --- a/internal/exec/util/util.go +++ b/internal/exec/util/util.go @@ -56,11 +56,14 @@ func wantsToEscape(p string) bool { // u.DestDir. This means that the resulting path will always be under // u.DestDir. If u.IsRoot is false, it fails if a symlink resolves such // that it would escape u.DestDir. +// The last element of the path is never followed. func (u Util) JoinPath(path ...string) (string, error) { components := []string{} for _, tmp := range path { components = append(components, splitPath(tmp)...) } + last := components[len(components)-1] + components = components[:len(components)-1] realpath := "/" for _, component := range components { @@ -94,5 +97,5 @@ func (u Util) JoinPath(path ...string) (string, error) { realpath = filepath.Join(realpath, symlinkPath) } - return filepath.Join(u.DestDir, realpath), nil + return filepath.Join(u.DestDir, realpath, last), nil } From 54e3c043e44efb839bfff89b9e9dc2b50b7fef93 Mon Sep 17 00:00:00 2001 From: Andrew Jeddeloh Date: Wed, 5 Dec 2018 15:21:47 -0800 Subject: [PATCH 2/3] exec/util: use Lstat() for existance checks We care if the link itself exists, not if the file it points to exists. --- internal/exec/util/file.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/exec/util/file.go b/internal/exec/util/file.go index aa4fab253..902404405 100644 --- a/internal/exec/util/file.go +++ b/internal/exec/util/file.go @@ -153,7 +153,7 @@ func (u Util) PerformFetch(f *FetchOp) error { // guarantees here. If the user explicitly doesn't want us to overwrite // preexisting nodes, check the target path and fail if something's // there. - _, err := os.Stat(path) + _, err := os.Lstat(path) switch { case os.IsNotExist(err): break @@ -282,7 +282,7 @@ func (u Util) PathExists(path string) (bool, error) { return false, err } - _, err = os.Stat(path) + _, err = os.Lstat(path) switch { case os.IsNotExist(err): return false, nil From 91bab0c10fddcc2af6807d63dd789f6dcf5a9c03 Mon Sep 17 00:00:00 2001 From: Andrew Jeddeloh Date: Wed, 5 Dec 2018 13:15:19 -0800 Subject: [PATCH 3/3] tests: add bb test for symlink resolution Verify that the last element of fs entries' paths is not followed when resolving paths. Test that we dont overwrite dead links when overwrite is false. --- tests/negative/files/link.go | 36 +++++++++++ tests/positive/files/link.go | 112 +++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) diff --git a/tests/negative/files/link.go b/tests/negative/files/link.go index 221853100..15f85a72b 100644 --- a/tests/negative/files/link.go +++ b/tests/negative/files/link.go @@ -22,6 +22,7 @@ import ( func init() { register.Register(register.NegativeTest, WriteThroughRelativeSymlink()) register.Register(register.NegativeTest, WriteThroughAbsoluteSymlink()) + register.Register(register.NegativeTest, WriteOverBrokenSymlink()) } func WriteThroughRelativeSymlink() types.Test { name := "Write Through Relative Symlink off the Root Filesystem" @@ -114,3 +115,38 @@ func WriteThroughAbsoluteSymlink() types.Test { ConfigMinVersion: configMinVersion, } } + +func WriteOverBrokenSymlink() types.Test { + name := "Write Over Broken Symlink at end of path" + in := types.GetBaseDisk() + out := types.GetBaseDisk() + config := `{ + "ignition": { "version": "$version" }, + "storage": { + "files": [{ + "filesystem": "root", + "path": "/etc/file", + "overwrite": false, + "mode": 420 + }] + } + }` + in[0].Partitions.AddLinks("ROOT", []types.Link{ + { + Node: types.Node{ + Name: "file", + Directory: "etc", + }, + Target: "/usr/rofile", + }, + }) + configMinVersion := "2.2.0" + + return types.Test{ + Name: name, + In: in, + Out: out, + Config: config, + ConfigMinVersion: configMinVersion, + } +} diff --git a/tests/positive/files/link.go b/tests/positive/files/link.go index 77f53c058..13480b123 100644 --- a/tests/positive/files/link.go +++ b/tests/positive/files/link.go @@ -27,6 +27,8 @@ func init() { register.Register(register.PositiveTest, WriteThroughAbsoluteSymlink()) register.Register(register.PositiveTest, ForceLinkCreation()) register.Register(register.PositiveTest, ForceHardLinkCreation()) + register.Register(register.PositiveTest, WriteOverSymlink()) + register.Register(register.PositiveTest, WriteOverBrokenSymlink()) } func CreateHardLinkOnRoot() types.Test { @@ -396,3 +398,113 @@ func ForceHardLinkCreation() types.Test { ConfigMinVersion: configMinVersion, } } + +func WriteOverSymlink() types.Test { + name := "Write Over Symlink at end of path" + in := types.GetBaseDisk() + out := types.GetBaseDisk() + // note this abuses the order in which ignition writes links and will break with 3.0.0 + // Also tests that Ignition does not try to resolve symlink targets + config := `{ + "ignition": { "version": "$version" }, + "storage": { + "files": [{ + "filesystem": "root", + "path": "/etc/file", + "mode": 420 + }] + } + }` + in[0].Partitions.AddLinks("ROOT", []types.Link{ + { + Node: types.Node{ + Name: "file", + Directory: "etc", + }, + Target: "/usr/rofile", + }, + }) + in[0].Partitions.AddFiles("ROOT", []types.File{ + { + Node: types.Node{ + Name: "rofile", + Directory: "usr", + }, + Contents: "", + Mode: 420, + }, + }) + out[0].Partitions.AddFiles("ROOT", []types.File{ + { + Node: types.Node{ + Name: "rofile", + Directory: "usr", + }, + Contents: "", + Mode: 420, + }, + { + Node: types.Node{ + Name: "file", + Directory: "etc", + }, + Contents: "", + Mode: 420, + }, + }) + configMinVersion := "2.1.0" + + return types.Test{ + Name: name, + In: in, + Out: out, + Config: config, + ConfigMinVersion: configMinVersion, + } +} + +func WriteOverBrokenSymlink() types.Test { + name := "Write Over Broken Symlink at end of path" + in := types.GetBaseDisk() + out := types.GetBaseDisk() + // note this abuses the order in which ignition writes links and will break with 3.0.0 + // Also tests that Ignition does not try to resolve symlink targets + config := `{ + "ignition": { "version": "$version" }, + "storage": { + "files": [{ + "filesystem": "root", + "path": "/etc/file", + "mode": 420 + }] + } + }` + in[0].Partitions.AddLinks("ROOT", []types.Link{ + { + Node: types.Node{ + Name: "file", + Directory: "etc", + }, + Target: "/usr/rofile", + }, + }) + out[0].Partitions.AddFiles("ROOT", []types.File{ + { + Node: types.Node{ + Name: "file", + Directory: "etc", + }, + Contents: "", + Mode: 420, + }, + }) + configMinVersion := "2.1.0" + + return types.Test{ + Name: name, + In: in, + Out: out, + Config: config, + ConfigMinVersion: configMinVersion, + } +}