From 9b7c0fb7f3a7b13ca9c503d9c3d50e39e9054d74 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 30 Sep 2020 21:46:32 -0700 Subject: [PATCH 1/6] Steal tests from other branch that is being discarded, attempt to get at least one of them to pass; WIP --- go.mod | 1 + go.sum | 2 + router/router_server_files.go | 32 +- server/filesystem/disk_space.go | 13 +- server/filesystem/filesystem.go | 20 +- server/filesystem/filesystem_test.go | 578 +++++++++++++++++++++++++++ 6 files changed, 613 insertions(+), 33 deletions(-) create mode 100644 server/filesystem/filesystem_test.go diff --git a/go.mod b/go.mod index f174131..cecb97c 100644 --- a/go.mod +++ b/go.mod @@ -24,6 +24,7 @@ require ( github.com/docker/go-metrics v0.0.1 // indirect github.com/docker/go-units v0.4.0 // indirect github.com/fatih/color v1.9.0 + github.com/franela/goblin v0.0.0-20200825194134-80c0062ed6cd github.com/frankban/quicktest v1.10.2 // indirect github.com/fsnotify/fsnotify v1.4.9 // indirect github.com/gabriel-vasile/mimetype v1.1.1 diff --git a/go.sum b/go.sum index 5733b1f..5c762cc 100644 --- a/go.sum +++ b/go.sum @@ -123,6 +123,8 @@ github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5Kwzbycv github.com/fatih/color v1.9.0 h1:8xPHl4/q1VyqGIPif1F+1V3Y3lSmrq01EabUW3CoW5s= github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL+zU= github.com/franela/goblin v0.0.0-20200105215937-c9ffbefa60db/go.mod h1:7dvUGVsVBjqR7JHJk0brhHOZYGmfBYOrK0ZhYMEtBr4= +github.com/franela/goblin v0.0.0-20200825194134-80c0062ed6cd h1:b/30UOB56Rhfe185ZfgvZT0/HOql0OzxuiNOxRKXRXc= +github.com/franela/goblin v0.0.0-20200825194134-80c0062ed6cd/go.mod h1:VzmDKDJVZI3aJmnRI9VjAn9nJ8qPPsN1fqzr9dqInIo= github.com/franela/goreq v0.0.0-20171204163338-bcd34c9993f8/go.mod h1:ZhphrRTfi2rbfLwlschooIH4+wKKDR4Pdxhh+TRoA20= github.com/frankban/quicktest v1.10.2 h1:19ARM85nVi4xH7xPXuc5eM/udya5ieh7b/Sv+d844Tk= github.com/frankban/quicktest v1.10.2/go.mod h1:K+q6oSqb0W0Ininfk863uOk1lMy69l/P6txr3mVT54s= diff --git a/router/router_server_files.go b/router/router_server_files.go index 2027dc6..712010a 100644 --- a/router/router_server_files.go +++ b/router/router_server_files.go @@ -1,7 +1,6 @@ package router import ( - "bufio" "context" "github.com/gin-gonic/gin" "github.com/pkg/errors" @@ -30,34 +29,12 @@ func getServerFileContents(c *gin.Context) { } p = "/" + strings.TrimLeft(p, "/") - cleaned, err := s.Filesystem().SafePath(p) + st, err := s.Filesystem().Stat(p) if err != nil { - c.AbortWithStatusJSON(http.StatusNotFound, gin.H{ - "error": "The file requested could not be found.", - }) + TrackedServerError(err, s).AbortFilesystemError(c) return } - st, err := s.Filesystem().Stat(cleaned) - if err != nil { - TrackedServerError(err, s).AbortWithServerError(c) - return - } - - if st.Info.IsDir() { - c.AbortWithStatusJSON(http.StatusNotFound, gin.H{ - "error": "The requested resource was not found on the system.", - }) - return - } - - f, err := os.Open(cleaned) - if err != nil { - TrackedServerError(err, s).AbortWithServerError(c) - return - } - defer f.Close() - c.Header("X-Mime-Type", st.Mimetype) c.Header("Content-Length", strconv.Itoa(int(st.Info.Size()))) @@ -68,7 +45,10 @@ func getServerFileContents(c *gin.Context) { c.Header("Content-Type", "application/octet-stream") } - bufio.NewReader(f).WriteTo(c.Writer) + if err := s.Filesystem().Readfile(p, c.Writer); err != nil { + TrackedServerError(err, s).AbortFilesystemError(c) + return + } } // Returns the contents of a directory for a server. diff --git a/server/filesystem/disk_space.go b/server/filesystem/disk_space.go index 4eb3273..4922b7a 100644 --- a/server/filesystem/disk_space.go +++ b/server/filesystem/disk_space.go @@ -199,7 +199,18 @@ func (fs *Filesystem) hasSpaceFor(size int64) error { // Updates the disk usage for the Filesystem instance. func (fs *Filesystem) addDisk(i int64) int64 { - size, _ := fs.DiskUsage(true) + var size = atomic.LoadInt64(&fs.diskUsed) + // Sorry go gods. This is ugly but the best approach I can come up with for right + // now without completely re-evaluating the logic we use for determining disk space. + // + // Normally I would just be using the atomic load right below, but I'm not sure about + // the scenarios where it is 0 because nothing has run that would trigger a disk size + // calculation? + // + // Perhaps that isn't even a concern for the sake of this? + if !fs.isTest { + size, _ = fs.DiskUsage(true) + } // If we're dropping below 0 somehow just cap it to 0. if (size + i) < 0 { diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index a98e0f9..dbe4363 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -2,7 +2,6 @@ package filesystem import ( "bufio" - "bytes" "fmt" "github.com/gabriel-vasile/mimetype" "github.com/karrick/godirwalk" @@ -33,6 +32,8 @@ type Filesystem struct { // The root data directory path for this Filesystem instance. root string + + isTest bool } // Creates a new Filesystem instance for a given server. @@ -53,18 +54,21 @@ func (fs *Filesystem) Path() string { // Reads a file on the system and returns it as a byte representation in a file // reader. This is not the most memory efficient usage since it will be reading the // entirety of the file into memory. -func (fs *Filesystem) Readfile(p string) (io.Reader, error) { +func (fs *Filesystem) Readfile(p string, w io.Writer) error { cleaned, err := fs.SafePath(p) if err != nil { - return nil, err + return err } - b, err := ioutil.ReadFile(cleaned) + f, err := os.Open(cleaned) if err != nil { - return nil, err + return err } + defer f.Close() - return bytes.NewReader(b), nil + _, err = bufio.NewReader(f).WriteTo(w) + + return err } // Writes a file to the system. If the file does not already exist one will be created. @@ -178,6 +182,10 @@ func (fs *Filesystem) Chown(path string) error { if err != nil { return errors.WithStack(err) } + + if fs.isTest { + return nil + } uid := config.Get().System.User.Uid gid := config.Get().System.User.Gid diff --git a/server/filesystem/filesystem_test.go b/server/filesystem/filesystem_test.go new file mode 100644 index 0000000..2f3a489 --- /dev/null +++ b/server/filesystem/filesystem_test.go @@ -0,0 +1,578 @@ +package filesystem + +import ( + "bytes" + "errors" + . "github.com/franela/goblin" + "github.com/pterodactyl/wings/config" + "io/ioutil" + "math/rand" + "os" + "path/filepath" + "strings" + "testing" + "unicode/utf8" +) + +func NewFs() (*Filesystem, *rootFs) { + config.Set(&config.Configuration{ + AuthenticationToken: "abc", + System: config.SystemConfiguration{ + RootDirectory: "/server", + DiskCheckInterval: 150, + }, + }) + + tmpDir, err := ioutil.TempDir(os.TempDir(), "pterodactyl") + if err != nil { + panic(err) + } + // defer os.RemoveAll(tmpDir) + + rfs := rootFs{root: tmpDir} + + rfs.reset() + + fs := New(filepath.Join(tmpDir, "/server"), 0) + fs.isTest = true + + return fs, &rfs +} + +type rootFs struct { + root string +} + +func (rfs *rootFs) CreateServerFile(p string, c string) error { + f, err := os.Create(filepath.Join(rfs.root, "/server", p)) + + if err == nil { + f.Write([]byte(c)) + f.Close() + } + + return err +} + +func (rfs *rootFs) StatServerFile(p string) (os.FileInfo, error) { + return os.Stat(filepath.Join(rfs.root, "/server", p)) +} + +func (rfs *rootFs) reset() { + if err := os.RemoveAll(filepath.Join(rfs.root, "/server")); err != nil { + if !os.IsNotExist(err) { + panic(err) + } + } + + if err := os.Mkdir(filepath.Join(rfs.root, "/server"), 0755); err != nil { + panic(err) + } +} + +func TestFilesystem_Path(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("Path", func() { + g.It("returns the root path for the instance", func() { + g.Assert(fs.Path()).Equal(filepath.Join(rfs.root, "/server")) + }) + }) +} + +func TestFilesystem_Readfile(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("Readfile", func() { + buf := &bytes.Buffer{} + + g.It("opens a file if it exists on the system", func() { + err := rfs.CreateServerFile("test.txt", "testing") + g.Assert(err).IsNil() + + err = fs.Readfile("test.txt", buf) + g.Assert(err).IsNil() + g.Assert(buf.String()).Equal("testing") + }) + + g.It("returns an error if the file does not exist", func() { + err := fs.Readfile("test.txt", buf) + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("returns an error if the \"file\" is a directory", func() { + err := rfs.CreateServerFile("test.txt", "testing") + g.Assert(err).IsNil() + + err = fs.Readfile("test.txt", buf) + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrIsDirectory)).IsTrue() + }) + + g.It("cannot open a file outside the root directory", func() { + err := rfs.CreateServerFile("/../test.txt", "testing") + g.Assert(err).IsNil() + + err = fs.Readfile("/../test.txt", buf) + g.Assert(err).IsNotNil() + g.Assert(strings.Contains(err.Error(), "file does not exist")).IsTrue() + }) + + g.AfterEach(func() { + buf.Truncate(0) + fs.diskUsed = 0 + rfs.reset() + }) + }) +} + +func TestFilesystem_Writefile(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("Open and WriteFile", func() { + buf := &bytes.Buffer{} + + // Test that a file can be written to the disk and that the disk space used as a result + // is updated correctly in the end. + g.It("can create a new file", func() { + r := bytes.NewReader([]byte("test file content")) + + g.Assert(fs.diskUsed).Equal(int64(0)) + + err := fs.Writefile("test.txt", r) + g.Assert(err).IsNil() + + err = fs.Readfile("test.txt", buf) + g.Assert(err).IsNil() + g.Assert(buf.String()).Equal("test file content") + g.Assert(fs.diskUsed).Equal(r.Size()) + }) + + g.It("can create a new file inside a nested directory with leading slash", func() { + r := bytes.NewReader([]byte("test file content")) + + err := fs.Writefile("/some/nested/test.txt", r) + g.Assert(err).IsNil() + + err = fs.Readfile("/some/nested/test.txt", buf) + g.Assert(err).IsNil() + g.Assert(buf.String()).Equal("test file content") + }) + + g.It("can create a new file inside a nested directory without a trailing slash", func() { + r := bytes.NewReader([]byte("test file content")) + + err := fs.Writefile("some/../foo/bar/test.txt", r) + g.Assert(err).IsNil() + + err = fs.Readfile("foo/bar/test.txt", buf) + g.Assert(err).IsNil() + g.Assert(buf.String()).Equal("test file content") + }) + + g.It("cannot create a file outside the root directory", func() { + r := bytes.NewReader([]byte("test file content")) + + err := fs.Writefile("/some/../foo/../../test.txt", r) + g.Assert(err).IsNotNil() + g.Assert(strings.Contains(err.Error(), "file does not exist")).IsTrue() + }) + + g.It("cannot write a file that exceedes the disk limits", func() { + fs.diskLimit = 1024 + + b := make([]byte, 1025) + _, err := rand.Read(b) + g.Assert(err).IsNil() + g.Assert(len(b)).Equal(1025) + + r := bytes.NewReader(b) + err = fs.Writefile("test.txt", r) + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrNotEnoughDiskSpace)).IsTrue() + }) + + g.It("updates the total space used when a file is appended to", func() { + fs.diskUsed = 100 + + b := make([]byte, 100) + _, _ = rand.Read(b) + + r := bytes.NewReader(b) + err := fs.Writefile("test.txt", r) + g.Assert(err).IsNil() + g.Assert(fs.diskUsed).Equal(int64(200)) + + // If we write less data than already exists, we should expect the total + // disk used to be decremented. + b = make([]byte, 50) + _, _ = rand.Read(b) + + r = bytes.NewReader(b) + err = fs.Writefile("test.txt", r) + g.Assert(err).IsNil() + g.Assert(fs.diskUsed).Equal(int64(150)) + }) + + g.It("truncates the file when writing new contents", func() { + r := bytes.NewReader([]byte("original data")) + err := fs.Writefile("test.txt", r) + g.Assert(err).IsNil() + + r = bytes.NewReader([]byte("new data")) + err = fs.Writefile("test.txt", r) + g.Assert(err).IsNil() + + err = fs.Readfile("test.txt", buf) + g.Assert(err).IsNil() + g.Assert(buf.String()).Equal("new data") + }) + + g.AfterEach(func() { + buf.Truncate(0) + rfs.reset() + fs.diskUsed = 0 + fs.diskLimit = 0 + }) + }) +} + +func TestFilesystem_CreateDirectory(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("CreateDirectory", func() { + g.It("should create missing directories automatically", func() { + err := fs.CreateDirectory("test", "foo/bar/baz") + g.Assert(err).IsNil() + + st, err := rfs.StatServerFile("foo/bar/baz/test") + g.Assert(err).IsNil() + g.Assert(st.IsDir()).IsTrue() + g.Assert(st.Name()).Equal("test") + }) + + g.It("should work with leading and trailing slashes", func() { + err := fs.CreateDirectory("test", "/foozie/barzie/bazzy/") + g.Assert(err).IsNil() + + st, err := rfs.StatServerFile("foozie/barzie/bazzy/test") + g.Assert(err).IsNil() + g.Assert(st.IsDir()).IsTrue() + g.Assert(st.Name()).Equal("test") + }) + + g.It("should not allow the creation of directories outside the root", func() { + err := fs.CreateDirectory("test", "e/../../something") + g.Assert(err).IsNotNil() + g.Assert(strings.Contains(err.Error(), "file does not exist")).IsTrue() + }) + + g.It("should not increment the disk usage", func() { + err := fs.CreateDirectory("test", "/") + g.Assert(err).IsNil() + g.Assert(fs.diskUsed).Equal(int64(0)) + }) + + g.AfterEach(func() { + rfs.reset() + }) + }) +} + +func TestFilesystem_Rename(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("Rename", func() { + g.BeforeEach(func() { + if err := rfs.CreateServerFile("source.txt", "text content"); err != nil { + panic(err) + } + }) + + g.It("returns an error if the target already exists", func() { + err := rfs.CreateServerFile("target.txt", "taget content") + g.Assert(err).IsNil() + + err = fs.Rename("source.txt", "target.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrExist)).IsTrue() + }) + + g.It("returns an error if the final destination is the root directory", func() { + err := fs.Rename("source.txt", "/") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrExist)).IsTrue() + }) + + g.It("returns an error if the source destination is the root directory", func() { + err := fs.Rename("source.txt", "/") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrExist)).IsTrue() + }) + + g.It("does not allow renaming to a location outside the root", func() { + err := fs.Rename("source.txt", "../target.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("does not allow renaming from a location outside the root", func() { + err := rfs.CreateServerFile("/../ext-source.txt", "taget content") + + err = fs.Rename("/../ext-source.txt", "target.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("allows a file to be renamed", func() { + err := fs.Rename("source.txt", "target.txt") + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("source.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + + st, err := rfs.StatServerFile("target.txt") + g.Assert(err).IsNil() + g.Assert(st.Name()).Equal("target.txt") + g.Assert(st.Size()).IsNotZero() + }) + + g.It("allows a folder to be renamed", func() { + err := os.Mkdir(filepath.Join(rfs.root, "/server/source_dir"), 0755) + g.Assert(err).IsNil() + + err = fs.Rename("source_dir", "target_dir") + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("source_dir") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + + st, err := rfs.StatServerFile("target_dir") + g.Assert(err).IsNil() + g.Assert(st.IsDir()).IsTrue() + }) + + g.It("returns an error if the source does not exist", func() { + err := fs.Rename("missing.txt", "target.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("creates directories if they are missing", func() { + err := fs.Rename("source.txt", "nested/folder/target.txt") + g.Assert(err).IsNil() + + st, err := rfs.StatServerFile("nested/folder/target.txt") + g.Assert(err).IsNil() + g.Assert(st.Name()).Equal("target.txt") + }) + + g.AfterEach(func() { + rfs.reset() + }) + }) +} + +func TestFilesystem_Copy(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("Copy", func() { + g.BeforeEach(func() { + if err := rfs.CreateServerFile("source.txt", "text content"); err != nil { + panic(err) + } + + fs.diskUsed = int64(utf8.RuneCountInString("test content")) + }) + + g.It("should return an error if the source does not exist", func() { + err := fs.Copy("foo.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("should return an error if the source is outside the root", func() { + err := rfs.CreateServerFile("/../ext-source.txt", "text content") + + err = fs.Copy("../ext-source.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("should return an error if the source directory is outside the root", func() { + err := os.MkdirAll(filepath.Join(rfs.root, "/nested/in/dir"), 0755) + g.Assert(err).IsNil() + + err = rfs.CreateServerFile("/../nested/in/dir/ext-source.txt", "external content") + g.Assert(err).IsNil() + + err = fs.Copy("../nested/in/dir/ext-source.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + + err = fs.Copy("nested/in/../../../nested/in/dir/ext-source.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("should return an error if the source is a directory", func() { + err := os.Mkdir(filepath.Join(rfs.root, "/server/dir"), 0755) + g.Assert(err).IsNil() + + err = fs.Copy("dir") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("should return an error if there is not space to copy the file", func() { + fs.diskLimit = 2 + + err := fs.Copy("source.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, ErrNotEnoughDiskSpace)).IsTrue() + }) + + g.It("should create a copy of the file and increment the disk used", func() { + err := fs.Copy("source.txt") + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("source.txt") + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("source copy.txt") + g.Assert(err).IsNil() + }) + + g.It("should create a copy of the file with a suffix if a copy already exists", func() { + err := fs.Copy("source.txt") + g.Assert(err).IsNil() + + err = fs.Copy("source.txt") + g.Assert(err).IsNil() + + r := []string{"source.txt", "source copy.txt", "source copy 1.txt"} + + for _, name := range r { + _, err = rfs.StatServerFile(name) + g.Assert(err).IsNil() + } + + g.Assert(fs.diskUsed).Equal(int64(utf8.RuneCountInString("test content")) * 3) + }) + + g.It("should create a copy inside of a directory", func() { + err := os.MkdirAll(filepath.Join(rfs.root, "/server/nested/in/dir"), 0755) + g.Assert(err).IsNil() + + err = rfs.CreateServerFile("source.txt", "test content") + g.Assert(err).IsNil() + + err = fs.Copy("nested/in/dir/source.txt") + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("nested/in/dir/source.txt") + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("nested/in/dir/source copy.txt") + g.Assert(err).IsNil() + }) + + g.AfterEach(func() { + rfs.reset() + fs.diskUsed = 0 + fs.diskLimit = 0 + }) + }) +} + +func TestFilesystem_Delete(t *testing.T) { + g := Goblin(t) + fs, rfs := NewFs() + + g.Describe("Delete", func() { + g.BeforeEach(func() { + if err := rfs.CreateServerFile("source.txt", "test content"); err != nil { + panic(err) + } + + fs.diskUsed = int64(utf8.RuneCountInString("test content")) + }) + + g.It("does not delete files outside the root directory", func() { + err := rfs.CreateServerFile("/../ext-source.txt", "external content") + + err = fs.Delete("../ext-source.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + }) + + g.It("does not allow the deletion of the root directory", func() { + err := fs.Delete("/") + g.Assert(err).IsNotNil() + g.Assert(err.Error()).Equal("cannot delete root server directory") + }) + + g.It("does not return an error if the target does not exist", func() { + err := fs.Delete("missing.txt") + g.Assert(err).IsNil() + + st, err := rfs.StatServerFile("source.txt") + g.Assert(err).IsNil() + g.Assert(st.Name()).Equal("source.txt") + }) + + g.It("deletes files and subtracts their size from the disk usage", func() { + err := fs.Delete("source.txt") + g.Assert(err).IsNil() + + _, err = rfs.StatServerFile("source.txt") + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + + g.Assert(fs.diskUsed).Equal(int64(0)) + }) + + g.It("deletes all items inside a directory if the directory is deleted", func() { + sources := []string{ + "foo/source.txt", + "foo/bar/source.txt", + "foo/bar/baz/source.txt", + } + + err := os.MkdirAll(filepath.Join(rfs.root, "/server/foo/bar/baz"), 0755) + g.Assert(err).IsNil() + + for _, s := range sources { + err = rfs.CreateServerFile(s, "test content") + g.Assert(err).IsNil() + } + + fs.diskUsed = int64(utf8.RuneCountInString("test content") * 3) + + err = fs.Delete("foo") + g.Assert(err).IsNil() + g.Assert(fs.diskUsed).Equal(int64(0)) + + for _, s := range sources { + _, err = rfs.StatServerFile(s) + g.Assert(err).IsNotNil() + g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + } + }) + + g.AfterEach(func() { + rfs.reset() + fs.diskUsed = 0 + fs.diskLimit = 0 + }) + }) +} From ee460686d670dba839ef79bf934af55243f1862c Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 30 Sep 2020 21:47:42 -0700 Subject: [PATCH 2/6] Make delete more synchronous --- server/filesystem/filesystem.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index dbe4363..76714cd 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -182,7 +182,7 @@ func (fs *Filesystem) Chown(path string) error { if err != nil { return errors.WithStack(err) } - + if fs.isTest { return nil } @@ -321,6 +321,7 @@ func (fs *Filesystem) Copy(p string) error { // Deletes a file or folder from the system. Prevents the user from accidentally // (or maliciously) removing their root server data directory. func (fs *Filesystem) Delete(p string) error { + wg := sync.WaitGroup{} // This is one of the few (only?) places in the codebase where we're explicitly not using // the SafePath functionality when working with user provided input. If we did, you would // not be able to delete a file that is a symlink pointing to a location outside of the data @@ -348,14 +349,18 @@ func (fs *Filesystem) Delete(p string) error { if !st.IsDir() { fs.addDisk(-st.Size()) } else { - go func(st os.FileInfo, resolved string) { + wg.Add(1) + go func(wg *sync.WaitGroup, st os.FileInfo, resolved string) { + defer wg.Done() if s, err := fs.DirectorySize(resolved); err == nil { fs.addDisk(-s) } - }(st, resolved) + }(&wg, st, resolved) } } + wg.Wait() + return os.RemoveAll(resolved) } From 367fdfad54fdeadaa21d954513d0960666d1209f Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Wed, 30 Sep 2020 21:53:50 -0700 Subject: [PATCH 3/6] Simplify copy file logic --- server/filesystem/filesystem.go | 91 ++++++++++++---------------- server/filesystem/filesystem_test.go | 8 +-- 2 files changed, 42 insertions(+), 57 deletions(-) diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index 76714cd..3892a82 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -2,7 +2,6 @@ package filesystem import ( "bufio" - "fmt" "github.com/gabriel-vasile/mimetype" "github.com/karrick/godirwalk" "github.com/pkg/errors" @@ -222,6 +221,42 @@ func (fs *Filesystem) Chown(path string) error { }) } +// Begin looping up to 50 times to try and create a unique copy file name. This will take +// an input of "file.txt" and generate "file copy.txt". If that name is already taken, it will +// then try to write "file copy 2.txt" and so on, until reaching 50 loops. At that point we +// won't waste anymore time, just use the current timestamp and make that copy. +// +// Could probably make this more efficient by checking if there are any files matching the copy +// pattern, and trying to find the highest number and then incrementing it by one rather than +// looping endlessly. +func (fs *Filesystem) findCopySuffix(dir string, name string, extension string) (string, error) { + var i int + var suffix = " copy" + + for i = 0; i < 51; i++ { + if i > 0 { + suffix = " copy " + strconv.Itoa(i) + } + + n := name + suffix + extension + // If we stat the file and it does not exist that means we're good to create the copy. If it + // does exist, we'll just continue to the next loop and try again. + if _, err := fs.Stat(path.Join(dir, n)); err != nil { + if !os.IsNotExist(err) { + return "", err + } + + break + } + + if i == 50 { + suffix = "copy." + time.Now().Format(time.RFC3339) + } + } + + return name + suffix + extension, nil +} + // Copies a given file to the same location and appends a suffix to the file to indicate that // it has been copied. func (fs *Filesystem) Copy(p string) error { @@ -257,65 +292,15 @@ func (fs *Filesystem) Copy(p string) error { name = strings.TrimSuffix(name, ".tar") } - // Begin looping up to 50 times to try and create a unique copy file name. This will take - // an input of "file.txt" and generate "file copy.txt". If that name is already taken, it will - // then try to write "file copy 2.txt" and so on, until reaching 50 loops. At that point we - // won't waste anymore time, just use the current timestamp and make that copy. - // - // Could probably make this more efficient by checking if there are any files matching the copy - // pattern, and trying to find the highest number and then incrementing it by one rather than - // looping endlessly. - var i int - copySuffix := " copy" - for i = 0; i < 51; i++ { - if i > 0 { - copySuffix = " copy " + strconv.Itoa(i) - } - - tryName := fmt.Sprintf("%s%s%s", name, copySuffix, extension) - tryLocation, err := fs.SafePath(path.Join(relative, tryName)) - if err != nil { - return errors.WithStack(err) - } - - // If the file exists, continue to the next loop, otherwise we're good to start a copy. - if _, err := os.Stat(tryLocation); err != nil && !os.IsNotExist(err) { - return errors.WithStack(err) - } else if os.IsNotExist(err) { - break - } - - if i == 50 { - copySuffix = "." + time.Now().Format(time.RFC3339) - } - } - - finalPath, err := fs.SafePath(path.Join(relative, fmt.Sprintf("%s%s%s", name, copySuffix, extension))) - if err != nil { - return errors.WithStack(err) - } - source, err := os.Open(cleaned) if err != nil { return errors.WithStack(err) } defer source.Close() - dest, err := os.Create(finalPath) - if err != nil { - return errors.WithStack(err) - } - defer dest.Close() + n, err := fs.findCopySuffix(relative, name, extension) - buf := make([]byte, 1024*4) - if _, err := io.CopyBuffer(dest, source, buf); err != nil { - return errors.WithStack(err) - } - - // Once everything is done, increment the disk space used. - fs.addDisk(s.Size()) - - return nil + return fs.Writefile(path.Join(relative, n), source) } // Deletes a file or folder from the system. Prevents the user from accidentally diff --git a/server/filesystem/filesystem_test.go b/server/filesystem/filesystem_test.go index 2f3a489..11dfa4e 100644 --- a/server/filesystem/filesystem_test.go +++ b/server/filesystem/filesystem_test.go @@ -405,7 +405,7 @@ func TestFilesystem_Copy(t *testing.T) { err = fs.Copy("../ext-source.txt") g.Assert(err).IsNotNil() - g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.It("should return an error if the source directory is outside the root", func() { @@ -417,11 +417,11 @@ func TestFilesystem_Copy(t *testing.T) { err = fs.Copy("../nested/in/dir/ext-source.txt") g.Assert(err).IsNotNil() - g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() err = fs.Copy("nested/in/../../../nested/in/dir/ext-source.txt") g.Assert(err).IsNotNil() - g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.It("should return an error if the source is a directory", func() { @@ -512,7 +512,7 @@ func TestFilesystem_Delete(t *testing.T) { err = fs.Delete("../ext-source.txt") g.Assert(err).IsNotNil() - g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.It("does not allow the deletion of the root directory", func() { From 90ae815b1d9df27519097837db64353136f5a450 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Thu, 1 Oct 2020 20:40:25 -0700 Subject: [PATCH 4/6] Return tests to passing state --- server/filesystem/filesystem.go | 8 +++++++- server/filesystem/filesystem_test.go | 15 +++++++-------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index 3892a82..b7ee4f0 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -59,6 +59,12 @@ func (fs *Filesystem) Readfile(p string, w io.Writer) error { return err } + if st, err := os.Stat(cleaned); err != nil { + return err + } else if st.IsDir() { + return ErrIsDirectory + } + f, err := os.Open(cleaned) if err != nil { return err @@ -164,7 +170,7 @@ func (fs *Filesystem) Rename(from string, to string) error { // Ensure that the directory we're moving into exists correctly on the system. Only do this if // we're not at the root directory level. if d != fs.Path() { - if mkerr := os.MkdirAll(d, 0644); mkerr != nil { + if mkerr := os.MkdirAll(d, 0755); mkerr != nil { return errors.Wrap(mkerr, "failed to create directory structure for file rename") } } diff --git a/server/filesystem/filesystem_test.go b/server/filesystem/filesystem_test.go index 11dfa4e..9494de0 100644 --- a/server/filesystem/filesystem_test.go +++ b/server/filesystem/filesystem_test.go @@ -9,7 +9,6 @@ import ( "math/rand" "os" "path/filepath" - "strings" "testing" "unicode/utf8" ) @@ -104,7 +103,7 @@ func TestFilesystem_Readfile(t *testing.T) { }) g.It("returns an error if the \"file\" is a directory", func() { - err := rfs.CreateServerFile("test.txt", "testing") + err := os.Mkdir(filepath.Join(rfs.root, "/server/test.txt"), 0755) g.Assert(err).IsNil() err = fs.Readfile("test.txt", buf) @@ -118,7 +117,7 @@ func TestFilesystem_Readfile(t *testing.T) { err = fs.Readfile("/../test.txt", buf) g.Assert(err).IsNotNil() - g.Assert(strings.Contains(err.Error(), "file does not exist")).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.AfterEach(func() { @@ -179,7 +178,7 @@ func TestFilesystem_Writefile(t *testing.T) { err := fs.Writefile("/some/../foo/../../test.txt", r) g.Assert(err).IsNotNil() - g.Assert(strings.Contains(err.Error(), "file does not exist")).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.It("cannot write a file that exceedes the disk limits", func() { @@ -269,7 +268,7 @@ func TestFilesystem_CreateDirectory(t *testing.T) { g.It("should not allow the creation of directories outside the root", func() { err := fs.CreateDirectory("test", "e/../../something") g.Assert(err).IsNotNil() - g.Assert(strings.Contains(err.Error(), "file does not exist")).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.It("should not increment the disk usage", func() { @@ -319,7 +318,7 @@ func TestFilesystem_Rename(t *testing.T) { g.It("does not allow renaming to a location outside the root", func() { err := fs.Rename("source.txt", "../target.txt") g.Assert(err).IsNotNil() - g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.It("does not allow renaming from a location outside the root", func() { @@ -327,7 +326,7 @@ func TestFilesystem_Rename(t *testing.T) { err = fs.Rename("/../ext-source.txt", "target.txt") g.Assert(err).IsNotNil() - g.Assert(errors.Is(err, os.ErrNotExist)).IsTrue() + g.Assert(errors.Is(err, ErrBadPathResolution)).IsTrue() }) g.It("allows a file to be renamed", func() { @@ -473,7 +472,7 @@ func TestFilesystem_Copy(t *testing.T) { err := os.MkdirAll(filepath.Join(rfs.root, "/server/nested/in/dir"), 0755) g.Assert(err).IsNil() - err = rfs.CreateServerFile("source.txt", "test content") + err = rfs.CreateServerFile("nested/in/dir/source.txt", "test content") g.Assert(err).IsNil() err = fs.Copy("nested/in/dir/source.txt") From e3e89a2ecc6d643d7970cd7819e34384b3ae88c0 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Thu, 1 Oct 2020 21:13:42 -0700 Subject: [PATCH 5/6] 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() From 244640d0c1f6214940d6f7e83c53a1c518bb32cc Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Thu, 1 Oct 2020 21:28:38 -0700 Subject: [PATCH 6/6] [security] remove "SafeJoin" function This function was not actually safe in theory. If an unknown stat source was passed in it would be possible for a symlinked file to not be detected as a symlink, thus skipping any safe path resolutions. This would happen if the stat source was a regular os.Stat call and not an os.Lstat call, but since there is no way to differentiate between those two in the code, it is safer to just manually apply this logic in the positions where we _know_ for certain that we're working with the results of an Lstat call. --- server/archiver.go | 17 ++++++++++++++--- server/filesystem/filesystem.go | 6 +++++- server/filesystem/path.go | 21 --------------------- 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/server/archiver.go b/server/archiver.go index 9626b4b..f757bb1 100644 --- a/server/archiver.go +++ b/server/archiver.go @@ -62,9 +62,20 @@ func (a *Archiver) Archive() error { } for _, file := range fileInfo { - f, err := a.Server.Filesystem().SafeJoin(path, file) - if err != nil { - return err + f := filepath.Join(path, file.Name()) + // If the file is a symlink we cannot safely assume that the result of a filepath.Join() will be + // a safe destination. We need to check if the file is a symlink, and if so pass off to the SafePath + // function to resolve it to the final destination. + // + // ioutil.ReadDir() calls Lstat, so this will work correctly. If it did not call Lstat, but rather + // just did a normal Stat call, this would fail since that would be looking at the symlink destination + // and not the actual file in this listing. + if file.Mode()&os.ModeSymlink != 0 { + f, err = a.Server.Filesystem().SafePath(filepath.Join(path, file.Name())) + + if err != nil { + return err + } } files = append(files, f) diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index c77e098..a6b89a5 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -412,7 +412,11 @@ func (fs *Filesystem) ListDirectory(p string) ([]*Stat, error) { var m *mimetype.MIME var d = "inode/directory" if !f.IsDir() { - cleanedp, _ := fs.SafeJoin(cleaned, f) + cleanedp := filepath.Join(cleaned, f.Name()) + if f.Mode()&os.ModeSymlink != 0 { + cleanedp, _ = fs.SafePath(filepath.Join(cleaned, f.Name())) + } + if cleanedp != "" { m, _ = mimetype.DetectFile(filepath.Join(cleaned, f.Name())) } else { diff --git a/server/filesystem/path.go b/server/filesystem/path.go index dd98022..6476a9d 100644 --- a/server/filesystem/path.go +++ b/server/filesystem/path.go @@ -92,27 +92,6 @@ func (fs *Filesystem) unsafeIsInDataDirectory(p string) bool { return strings.HasPrefix(strings.TrimSuffix(p, "/")+"/", strings.TrimSuffix(fs.Path(), "/")+"/") } -// Helper function to keep some of the codebase a little cleaner. Returns a "safe" version of the path -// joined with a file. This is important because you cannot just assume that appending a file to a cleaned -// path will result in a cleaned path to that file. For example, imagine you have the following scenario: -// -// my_bad_file -> symlink:/etc/passwd -// -// cleaned := SafePath("../../etc") -> "/" -// filepath.Join(cleaned, my_bad_file) -> "/my_bad_file" -// -// You might think that "/my_bad_file" is fine since it isn't pointing to the original "../../etc/my_bad_file". -// However, this doesn't account for symlinks where the file might be pointing outside of the directory, so -// calling a function such as Chown against it would chown the symlinked location, and not the file within the -// Wings daemon. -func (fs *Filesystem) SafeJoin(dir string, f os.FileInfo) (string, error) { - if f.Mode()&os.ModeSymlink != 0 { - return fs.SafePath(filepath.Join(dir, f.Name())) - } - - return filepath.Join(dir, f.Name()), nil -} - // Executes the fs.SafePath function in parallel against an array of paths. If any of the calls // fails an error will be returned. func (fs *Filesystem) ParallelSafePath(paths []string) ([]string, error) {