cyphar/filepath-securejoin

Proposed filepath.SecureJoin implementation

filepath-securejoin

An implementation of SecureJoin, a candidate for inclusion in the Go standard library. The purpose of this function is to be a "secure" alternative to filepath.Join, and in particular it provides certain guarantees that are not provided by filepath.Join.

NOTE: This code is only safe if you are not at risk of other processes modifying path components after you've used SecureJoin. If it is possible for a malicious process to modify path components of the resolved path, then you will be vulnerable to some fairly trivial TOCTOU race conditions. There are some Linux kernel patches I'm working on which might allow for a better solution.

In addition, with a slightly modified API it might be possible to use O_PATH and verify that the opened path is actually the resolved one -- but I have not done that yet. I might add it in the future as a helper function to help users verify the path (we can't just return /proc/self/fd/<foo> because that doesn't always work transparently for all users).

This is the function prototype:

func SecureJoin(root, unsafePath string) (string, error)

This library guarantees the following:

• If no error is set, the resulting string must be a child path of root and will not contain any symlink path components (they will all be expanded).

• When expanding symlinks, all symlink path components must be resolved relative to the provided root. In particular, this can be considered a userspace implementation of how chroot(2) operates on file paths. Note that these symlinks will not be expanded lexically (filepath.Clean is not called on the input before processing).

• Non-existent path components are unaffected by SecureJoin (similar to filepath.EvalSymlinks's semantics).

• The returned path will always be filepath.Cleaned and thus not contain any .. components.

A (trivial) implementation of this function on GNU/Linux systems could be done with the following (note that this requires root privileges and is far more opaque than the implementation in this library, and also requires that readlink is inside the root path):

package securejoin

import (
	"os/exec"
	"path/filepath"
)

func SecureJoin(root, unsafePath string) (string, error) {
	unsafePath = string(filepath.Separator) + unsafePath
	cmd := exec.Command("chroot", root,
		"readlink", "--canonicalize-missing", "--no-newline", unsafePath)
	output, err := cmd.CombinedOutput()
	if err != nil {
		return "", err
	}
	expanded := string(output)
	return filepath.Join(root, expanded), nil
}

License

The license of this project is the same as Go, which is a BSD 3-clause license available in the LICENSE file.

filepath-securejoin's Issues

loop error message contains a non-existent path

The error message that is triggered by too many symlink dereferences contains a non-existent path instead of the path to the symlink that causes the loop.

The path is set here:

filepath-securejoin/join.go

Line 58 in b69b737

return "", &os.PathError{Op: "SecureJoin", Path: root + string(filepath.Separator) + unsafePath, Err: syscall.ELOOP}

Steps to reproduce the issue

On Fedora CoreOS perform the following steps

1. mkdir ~/test
2. cd ~/test
3. create the file Dockerfile with this contents

   FROM docker.io/library/fedora:38
   RUN dnf install -y golang
   

4. podman build -t go .
5. create the file reproducer.go with this contents

   package main
   import (
       "os"
       "errors"
       "fmt"
       securejoin "github.com/cyphar/filepath-securejoin"
   )
   func main() {
     path := "/root/sub"
     os.MkdirAll(path, 0755)
     target := "symlink"
     symlink := "/root/sub/symlink"
     os.Symlink(target, symlink)
     _, err := securejoin.SecureJoin("/root","sub/symlink")
     if (err != nil) {
       var pErr *os.PathError
       if errors.As(err, &pErr) {
         fmt.Printf("Error = %v\n", pErr.Err)
         fmt.Printf("Path = %v\n", pErr.Path)
       }
     }
   }

6. podman run --rm -v ./:/src:Z -w /src localhost/go go mod init reproducer
7. podman run --rm -v ./:/src:Z -w /src localhost/go go mod tidy
8. podman run --rm -v ./:/src:Z -w /src localhost/go go run .
   The command prints

   go: downloading github.com/cyphar/filepath-securejoin v0.2.4
   Error = too many levels of symbolic links
   Path = /root/symlink/
   

Describe the results you received

At step 8 I see

Path = /root/symlink/

Describe the results you expected

At step 8 I would have expected to see

Path = /root/sub/symlink

as /root/sub/symlink is the path to the symlink that points to itself. The path /root/symlink/ does not exist.

Comment

I'm not blocked by this issue in any way. Probably this issue is of low importance.

Error not being raised when expected for unsafePath of `../upperfile.txt`

This might be more of a question than an issue, but I'll log it anyway and see what the expectation is.

In https://github.com/cyphar/filepath-securejoin/blob/master/join.go#L61, if the values passed in to SecureJoinVFS are:

(/root/testdir, ../upperfile.txt, nil) and on the machine the file /root/upperfile.txt exists, but the file /root/testdir/upperfile.txt does not, SecureJoinVFS() is not returning an error as I would expect.

Instead, it's cleaning up the .. and returning (/root/testdir/upperfile.txt, nil)

If by chance I had a /root/testdir/upperfile.txt I still would not expect to get a pointer to it instead of /root/upperfile.txt.

I think the correct behavior would be to return an error in this circumstance. Thoughts?

FWIW, this came up due to an investigation into a Podman issue logged here: containers/podman#5035

cut a release

Can we have a tagged release? Mostly interested in having #7 included into a released version.

consider migrating to io/fs.StatFS for SecureJoinVFS

Now the Go has their own "generic" interfaces for filesystem roots, maybe it would make sense to migrate to using that rather than our own home-brew thing. Note that we want to be given the equivalent of fs.DirFS("/") not fs.DirFS(root) -- we are implementing our own resolution logic, and the unsafe path.Join logic in io/fs is not going to work.