From 244640d0c1f6214940d6f7e83c53a1c518bb32cc Mon Sep 17 00:00:00 2001
From: Dane Everitt <dane@daneeveritt.com>
Date: Thu, 1 Oct 2020 21:28:38 -0700
Subject: [PATCH] [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) {