Skip to content

Commit

Permalink
server(filesystem): SafePath tweaks
Browse files Browse the repository at this point in the history
  • Loading branch information
matthewpi committed Feb 8, 2023
1 parent 984bd10 commit 9f6548e
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 41 deletions.
14 changes: 9 additions & 5 deletions server/filesystem/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand All @@ -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")
}

Expand Down
48 changes: 12 additions & 36 deletions server/filesystem/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package filesystem

import (
"context"
iofs "io/fs"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -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)

Expand All @@ -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
}
}
}

// 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)
// 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 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)
Expand Down
24 changes: 24 additions & 0 deletions server/filesystem/path_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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"))

Expand Down

0 comments on commit 9f6548e

Please sign in to comment.