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) {