From e3e89a2ecc6d643d7970cd7819e34384b3ae88c0 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Thu, 1 Oct 2020 21:13:42 -0700 Subject: [PATCH] Cover symlink attacks with test cases --- server/filesystem/filesystem.go | 2 +- server/filesystem/filesystem_test.go | 207 +++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 1 deletion(-) diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index b7ee4f0..c77e098 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -332,7 +332,7 @@ func (fs *Filesystem) Delete(p string) error { return errors.New("cannot delete root server directory") } - if st, err := os.Stat(resolved); err != nil { + if st, err := os.Lstat(resolved); err != nil { if !os.IsNotExist(err) { fs.error(err).Warn("error while attempting to stat file before deletion") } diff --git a/server/filesystem/filesystem_test.go b/server/filesystem/filesystem_test.go index 9494de0..0ee8931 100644 --- a/server/filesystem/filesystem_test.go +++ b/server/filesystem/filesystem_test.go @@ -80,6 +80,213 @@ func TestFilesystem_Path(t *testing.T) { }) } +func TestFilesystem_SafePath(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + prefix := filepath.Join(rfs.root, "/server") + + g.Describe("SafePath", func() { + g.It("returns a cleaned path to a given file", func() { + p, err := fs.SafePath("test.txt") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix + "/test.txt") + + p, err = fs.SafePath("/test.txt") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix + "/test.txt") + + p, err = fs.SafePath("./test.txt") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix + "/test.txt") + + p, err = fs.SafePath("/foo/../test.txt") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix + "/test.txt") + + p, err = fs.SafePath("/foo/bar") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix + "/foo/bar") + }) + + g.It("handles root directory access", func() { + p, err := fs.SafePath("/") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix) + + p, err = fs.SafePath("") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix) + }) + + g.It("removes trailing slashes from paths", func() { + p, err := fs.SafePath("/foo/bar/") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix + "/foo/bar") + }) + + g.It("handles deeply nested directories that do not exist", func() { + p, err := fs.SafePath("/foo/bar/baz/quaz/../../ducks/testing.txt") + g.Assert(err).IsNil() + g.Assert(p).Equal(prefix + "/foo/bar/ducks/testing.txt") + }) + + g.It("blocks access to files outside the root directory", func() { + p, err := fs.SafePath("../test.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + g.Assert(p).Equal("") + + p, err = fs.SafePath("/../test.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + g.Assert(p).Equal("") + + p, err = fs.SafePath("./foo/../../test.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + g.Assert(p).Equal("") + + p, err = fs.SafePath("..") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + g.Assert(p).Equal("") + }) + }) +} + +// We test against accessing files outside the root directory in the tests, however it +// is still possible for someone to mess up and not properly use this safe path call. In +// order to truly confirm this, we'll try to pass in a symlinked malicious file to all of +// the calls and ensure they all fail with the same reason. +func TestFilesystem_Blocks_Symlinks(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + if err := rfs.CreateServerFile("/../malicious.txt", "external content"); err != nil { + panic(err) + } + + if err := os.Mkdir(filepath.Join(rfs.root, "/malicious_dir"), 0777); err != nil { + panic(err) + } + + if err := os.Symlink(filepath.Join(rfs.root, "malicious.txt"), filepath.Join(rfs.root, "/server/symlinked.txt")); err != nil { + panic(err) + } + + if err := os.Symlink(filepath.Join(rfs.root, "/malicious_dir"), filepath.Join(rfs.root, "/server/external_dir")); err != nil { + panic(err) + } + + g.Describe("Readfile", func() { + g.It("cannot read a file symlinked outside the root", func() { + b := bytes.Buffer{} + + err := fs.Readfile("symlinked.txt", &b) + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + }) + + g.Describe("Writefile", func() { + g.It("cannot write to a file symlinked outside the root", func() { + r := bytes.NewReader([]byte("testing")) + + err := fs.Writefile("symlinked.txt", r) + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + + g.It("cannot write a file to a directory symlinked outside the root", func() { + r := bytes.NewReader([]byte("testing")) + + err := fs.Writefile("external_dir/foo.txt", r) + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + }) + + g.Describe("CreateDirectory", func() { + g.It("cannot create a directory outside the root", func() { + err := fs.CreateDirectory("my_dir", "external_dir") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + + g.It("cannot create a nested directory outside the root", func() { + err := fs.CreateDirectory("my/nested/dir", "external_dir/foo/bar") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + + g.It("cannot create a nested directory outside the root", func() { + err := fs.CreateDirectory("my/nested/dir", "external_dir/server") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + }) + + g.Describe("Rename", func() { + g.It("cannot rename a file symlinked outside the directory root", func() { + err := fs.Rename("symlinked.txt", "foo.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + + g.It("cannot rename a symlinked directory outside the root", func() { + err := fs.Rename("external_dir", "foo") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + + g.It("cannot rename a file to a location outside the directory root", func() { + rfs.CreateServerFile("my_file.txt", "internal content") + + err := fs.Rename("my_file.txt", "external_dir/my_file.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + }) + + g.Describe("Chown", func() { + g.It("cannot chown a file symlinked outside the directory root", func() { + err := fs.Chown("symlinked.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + + g.It("cannot chown a directory symlinked outside the directory root", func() { + err := fs.Chown("external_dir") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + }) + + g.Describe("Copy", func() { + g.It("cannot copy a file symlinked outside the directory root", func() { + err := fs.Copy("symlinked.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() + }) + }) + + g.Describe("Delete", func() { + g.It("deletes the symlinked file but leaves the source", func() { + err := fs.Delete("symlinked.txt") + g.Assert(err).IsNil() + + _, err = os.Stat(filepath.Join(rfs.root, "malicious.txt")) + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("symlinked.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + }) + + rfs.reset() +} + func TestFilesystem_Readfile(t *testing.T) { g := Goblin(t) fs, rfs := NewFs()