cyphar/filepath-securejoin

Proposed filepath.SecureJoin implementation

filepath-securejoin's Issues

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

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.

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.

Provide examples how to do `os.OpenFile` securely after `SecureJoin()`

Thanks for this package!

As you noted in the README, using SecureJoin() before os.OpenFile() is not always enough if the path components are modified between the 2 functions.

Do you know of Go projects doing this safely?

cut a release

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