diff --git a/server/filesystem/filesystem.go b/server/filesystem/filesystem.go index cb70b44..0fd051b 100644 --- a/server/filesystem/filesystem.go +++ b/server/filesystem/filesystem.go @@ -159,7 +159,7 @@ func (fs *Filesystem) Writefile(p string, r io.Reader) error { // Adjust the disk usage to account for the old size and the new size of the file. fs.addDisk(sz - currentSize) - return fs.Chown(cleaned) + return fs.unsafeChown(cleaned) } // Creates a new directory (name) at a specified path (p) for the server. @@ -217,7 +217,12 @@ func (fs *Filesystem) Chown(path string) error { if err != nil { return err } + return fs.unsafeChown(cleaned) +} +// unsafeChown chowns the given path, without checking if the path is safe. This should only be used +// when the path has already been checked. +func (fs *Filesystem) unsafeChown(path string) error { if fs.isTest { return nil } @@ -226,19 +231,19 @@ func (fs *Filesystem) Chown(path string) error { gid := config.Get().System.User.Gid // Start by just chowning the initial path that we received. - if err := os.Chown(cleaned, uid, gid); err != nil { + if err := os.Chown(path, uid, gid); err != nil { return errors.Wrap(err, "server/filesystem: chown: failed to chown path") } // If this is not a directory we can now return from the function, there is nothing // left that we need to do. - if st, err := os.Stat(cleaned); err != nil || !st.IsDir() { + if st, err := os.Stat(path); err != nil || !st.IsDir() { return nil } // If this was a directory, begin walking over its contents recursively and ensure that all // of the subfiles and directories get their permissions updated as well. - err = godirwalk.Walk(cleaned, &godirwalk.Options{ + err := godirwalk.Walk(path, &godirwalk.Options{ Unsorted: true, Callback: func(p string, e *godirwalk.Dirent) error { // Do not attempt to chown a symlink. Go's os.Chown function will affect the symlink @@ -255,7 +260,6 @@ func (fs *Filesystem) Chown(path string) error { return os.Chown(p, uid, gid) }, }) - return errors.Wrap(err, "server/filesystem: chown: failed to chown during walk function") } diff --git a/server/filesystem/path.go b/server/filesystem/path.go index 8a439c3..3952e5d 100644 --- a/server/filesystem/path.go +++ b/server/filesystem/path.go @@ -2,6 +2,7 @@ package filesystem import ( "context" + iofs "io/fs" "os" "path/filepath" "strings" @@ -33,8 +34,6 @@ func (fs *Filesystem) IsIgnored(paths ...string) error { // This logic is actually copied over from the SFTP server code. Ideally that eventually // either gets ported into this application, or is able to make use of this package. func (fs *Filesystem) SafePath(p string) (string, error) { - var nonExistentPathResolution string - // Start with a cleaned up path before checking the more complex bits. r := fs.unsafeFilePath(p) @@ -44,47 +43,24 @@ func (fs *Filesystem) SafePath(p string) (string, error) { if err != nil && !os.IsNotExist(err) { return "", errors.Wrap(err, "server/filesystem: failed to evaluate symlink") } else if os.IsNotExist(err) { - // The requested 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(r), "/") - - var try string - // Range over all of the path parts and form directory pathings 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 { - nonExistentPathResolution = t - break - } + // The target of one of the symlinks (EvalSymlinks is recursive) does not exist. + // So we get what target path does not exist and check if it's within the data + // directory. If it is, we return the original path, otherwise we return an error. + pErr, ok := err.(*iofs.PathError) + if !ok { + return "", errors.Wrap(err, "server/filesystem: failed to evaluate symlink") } - } - - // If the new path doesn't start with their root directory there is clearly an escape - // attempt going on, and we should NOT resolve this path for them. - if nonExistentPathResolution != "" { - if !fs.unsafeIsInDataDirectory(nonExistentPathResolution) { - return "", NewBadPathResolution(p, nonExistentPathResolution) - } - - // If the nonExistentPathResolution variable is not empty then the initial path requested - // did not exist and we looped through the pathway until we found a match. At this point - // we've confirmed the first matched pathway exists in the root server directory, so we - // can go ahead and just return the path that was requested initially. - return r, nil + ep = pErr.Path } // If the requested directory from EvalSymlinks begins with the server root directory go // ahead and return it. If not we'll return an error which will block any further action // on the file. if fs.unsafeIsInDataDirectory(ep) { - return ep, nil + // Returning the original path here instead of the resolved path ensures that + // whatever the user is trying to do will work as expected. If we returned the + // resolved path, the user would be unable to know that it is in fact a symlink. + return r, nil } return "", NewBadPathResolution(p, r) diff --git a/server/filesystem/path_test.go b/server/filesystem/path_test.go index ebdb71a..ecb9627 100644 --- a/server/filesystem/path_test.go +++ b/server/filesystem/path_test.go @@ -115,6 +115,14 @@ func TestFilesystem_Blocks_Symlinks(t *testing.T) { panic(err) } + if err := os.Symlink(filepath.Join(rfs.root, "malicious_does_not_exist.txt"), filepath.Join(rfs.root, "/server/symlinked_does_not_exist.txt")); err != nil { + panic(err) + } + + if err := os.Symlink(filepath.Join(rfs.root, "/server/symlinked_does_not_exist.txt"), filepath.Join(rfs.root, "/server/symlinked_does_not_exist2.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) } @@ -128,6 +136,22 @@ func TestFilesystem_Blocks_Symlinks(t *testing.T) { g.Assert(IsErrorCode(err, ErrCodePathResolution)).IsTrue() }) + g.It("cannot write to a non-existent file symlinked outside the root", func() { + r := bytes.NewReader([]byte("testing what the fuck")) + + err := fs.Writefile("symlinked_does_not_exist.txt", r) + g.Assert(err).IsNotNil() + g.Assert(IsErrorCode(err, ErrCodePathResolution)).IsTrue() + }) + + g.It("cannot write to chained symlinks with target that does not exist outside the root", func() { + r := bytes.NewReader([]byte("testing what the fuck")) + + err := fs.Writefile("symlinked_does_not_exist2.txt", r) + g.Assert(err).IsNotNil() + g.Assert(IsErrorCode(err, ErrCodePathResolution)).IsTrue() + }) + g.It("cannot write a file to a directory symlinked outside the root", func() { r := bytes.NewReader([]byte("testing"))