From 429ac62dba22997a278bc709df5ac00a5a25d83d Mon Sep 17 00:00:00 2001 From: Matthew Penner Date: Wed, 8 Feb 2023 14:20:13 -0700 Subject: [PATCH] server(filesystem): Delete tweaks --- server/filesystem/filesystem.go | 67 +++++++++++++++++++------ server/filesystem/filesystem_test.go | 74 ++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 14 deletions(-) diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index 1d906f0..ccc2daa 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -387,10 +387,9 @@ func (fs *Filesystem) TruncateRootDirectory() error { // Delete removes 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 + // not be able to delete a file that is a symlink pointing to a location outside the data // directory. // // We also want to avoid resolving a symlink that points _within_ the data directory and thus @@ -407,25 +406,65 @@ func (fs *Filesystem) Delete(p string) error { return errors.New("cannot delete root server directory") } - if st, err := os.Lstat(resolved); err != nil { + st, err := os.Lstat(resolved) + if err != nil { if !os.IsNotExist(err) { fs.error(err).Warn("error while attempting to stat file before deletion") + return err } - } else { - if !st.IsDir() { - fs.addDisk(-st.Size()) - } else { - 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) + + // The following logic is used to handle a case where a user attempts to + // delete a file that does not exist through a directory symlink. + // We don't want to reveal that the file does not exist, so we validate + // the path of the symlink and return a bad path error if it is invalid. + + // The requested file or directory doesn't exist, so at this point we + // need to iterate up the path chain until we hit a directory that + // _does_ exist and can be validated. + parts := strings.Split(filepath.Dir(resolved), "/") + + // Range over all the path parts and form directory paths from the end + // moving up until we have a valid resolution, or we run out of paths to + // try. + for k := range parts { + try := strings.Join(parts[:(len(parts)-k)], "/") + if !fs.unsafeIsInDataDirectory(try) { + break + } + + t, err := filepath.EvalSymlinks(try) + if err == nil { + if !fs.unsafeIsInDataDirectory(t) { + return NewBadPathResolution(p, t) } - }(&wg, st, resolved) + break + } + } + + // Always return early if the file does not exist. + return nil + } + + // If the file is not a symlink, we need to check that it is not within a + // symlinked directory that points outside the data directory. + if st.Mode()&os.ModeSymlink == 0 { + ep, err := filepath.EvalSymlinks(resolved) + if err != nil { + if !os.IsNotExist(err) { + return err + } + } else if !fs.unsafeIsInDataDirectory(ep) { + return NewBadPathResolution(p, ep) } } - wg.Wait() + if st.IsDir() { + if s, err := fs.DirectorySize(resolved); err == nil { + fs.addDisk(-s) + } + } else { + fs.addDisk(-st.Size()) + } return os.RemoveAll(resolved) } diff --git a/server/filesystem/filesystem_test.go b/server/filesystem/filesystem_test.go index 7a175c0..23f43bf 100644 --- a/server/filesystem/filesystem_test.go +++ b/server/filesystem/filesystem_test.go @@ -537,6 +537,80 @@ func TestFilesystem_Delete(t *testing.T) { } }) + g.It("deletes a symlink but not it's target within the root directory", func() { + // Symlink to a file inside the root directory. + err := os.Symlink(filepath.Join(rfs.root, "server/source.txt"), filepath.Join(rfs.root, "server/symlink.txt")) + g.Assert(err).IsNil() + + // Delete the symlink itself. + err = fs.Delete("symlink.txt") + g.Assert(err).IsNil() + + // Ensure the symlink was deleted. + _, err = os.Lstat(filepath.Join(rfs.root, "server/symlink.txt")) + g.Assert(err).IsNotNil() + + // Ensure the symlink target still exists. + _, err = os.Lstat(filepath.Join(rfs.root, "server/source.txt")) + g.Assert(err).IsNil() + }) + + g.It("does not delete files symlinked outside of the root directory", func() { + // Create a file outside the root directory. + err := rfs.CreateServerFileFromString("/../source.txt", "test content") + g.Assert(err).IsNil() + + // Create a symlink to the file outside the root directory. + err = os.Symlink(filepath.Join(rfs.root, "source.txt"), filepath.Join(rfs.root, "/server/symlink.txt")) + g.Assert(err).IsNil() + + // Delete the symlink. (This should pass as we will delete the symlink itself, not it's target) + err = fs.Delete("symlink.txt") + g.Assert(err).IsNil() + + // Ensure the file outside the root directory still exists. + _, err = os.Lstat(filepath.Join(rfs.root, "source.txt")) + g.Assert(err).IsNil() + }) + + g.It("does not delete files symlinked through a directory outside of the root directory", func() { + // Create a directory outside the root directory. + err := os.Mkdir(filepath.Join(rfs.root, "foo"), 0o755) + g.Assert(err).IsNil() + + // Create a file inside the directory that is outside the root. + err = rfs.CreateServerFileFromString("/../foo/source.txt", "test content") + g.Assert(err).IsNil() + + // Symlink the directory that is outside the root to a file inside the root. + err = os.Symlink(filepath.Join(rfs.root, "foo"), filepath.Join(rfs.root, "server/symlink")) + g.Assert(err).IsNil() + + // Delete a file inside the symlinked directory. + err = fs.Delete("symlink/source.txt") + g.Assert(err).IsNotNil() + g.Assert(IsErrorCode(err, ErrCodePathResolution)).IsTrue() + + // Ensure the file outside the root directory still exists. + _, err = os.Lstat(filepath.Join(rfs.root, "foo/source.txt")) + g.Assert(err).IsNil() + }) + + g.It("returns an error when trying to delete a non-existent file symlinked through a directory outside of the root directory", func() { + // Create a directory outside the root directory. + err := os.Mkdir(filepath.Join(rfs.root, "foo2"), 0o755) + g.Assert(err).IsNil() + + // Symlink the directory that is outside the root to a file inside the root. + err = os.Symlink(filepath.Join(rfs.root, "foo2"), filepath.Join(rfs.root, "server/symlink")) + g.Assert(err).IsNil() + + // Delete a file inside the symlinked directory. + err = fs.Delete("symlink/source.txt") + g.Assert(err).IsNotNil() + g.Assert(IsErrorCode(err, ErrCodePathResolution)).IsTrue() + }) + g.AfterEach(func() { rfs.reset()