feat: add ChrootHandler() for specific root path

[UnightSun]

1. As drakkan's suggestion, refactor to RequestServer and Handlers, take example by InMemHandler
2. Optimized the test code refer to puellanivis' advice.
3. Tested on windows 10 and WSL ubuntu.

[dmlb2000]

This had been idle for almost a year now? What is the current state of this?

[puellanivis]

The PR still pollutes the root level namespace, and does not prevent traversal of symlinks outside of the chroot. I suppose we could just close this PR as “will not merge without rewrite”.

[nevun]

The PR still pollutes the root level namespace, and does not prevent traversal of symlinks outside of the chroot. I suppose we could just close this PR as “will not merge without rewrite”.

Regarding the symlink issue it seems to me is works as it should?

In the chroot I created (as root in a normal shell) the following:

[root@comp upload3923341595]# ls -latr
total 4
drwx------. 3 user user  30 Jan 16 11:28 ..
-rw-r--r--. 1 user user 260 Jan 16 11:28 watermarked_files.yar
lrwxrwxrwx. 1 root root  11 Jan 16 11:28 passwd -> /etc/passwd
drwx------. 2 user user  71 Jan 16 11:39 .

Connected to the sftp chroot daemon:

sftp> ls -latr
-rw-r--r--    1 1002     1002          260 Jan 16 10:28 watermarked_files.yar
lrwxrwxrwx    1 0        0              11 Jan 16 10:28 passwd
sftp> get passwd
Fetching /passwd to passwd
stat remote: No such file or directory
sftp>

sftp> symlink /etc/nonexisting foo
sftp> symlink /etc/passwd bar
sftp> ls -latr
-rw-r--r--    1 1002     1002          260 Jan 16 10:28 watermarked_files.yar
lrwxrwxrwx    1 0        0              11 Jan 16 10:28 passwd
lrwxrwxrwx    1 1002     1002           42 Jan 16 10:51 foo
lrwxrwxrwx    1 1002     1002           37 Jan 16 10:51 bar
sftp> get foo
Fetching /foo to foo
stat remote: No such file or directory
sftp> get bar
Fetching /bar to bar
stat remote: No such file or directory
sftp>

As per your comment on the PR where stat() returns an error but lstat() does not: stat should return info on the symlink and not the file linked to so it seems fine to me.

But I am probably misunderstanding something..

[puellanivis]

Could you have some sort of logging of what files it’s actually trying to use on the server side? I’m curious to know if the fetching is failing because of a convenient misalignment, or if it’s genuinely preventing access.

[nevun]

Seems you were right.

I could not trigger the Stat() call from my sftp client but running the test with some logging turned on shows that the Stat implementation in the chroot code is incorrect. It should not allow Stat to do os.Stat() directly, it needs something like this that resolves the actual file and checks whether or not it is inside the chroot:

 func (fs *chroot) Filelist(r *Request) (ListerAt, error) {
        _ = r.WithContext(r.Context()) // initialize context for deadlock testing
        realPath, err := fs.getRealPath(r.Filepath)
        if err != nil {
                return nil, err
        }
        switch r.Method {
        case "List":
                files, err := os.ReadDir(realPath)
                if err != nil {
                        return nil, err
                }
                var res listerat = make(listerat, 0, len(files))
                for _, file := range files {
                        stat, err := file.Info()
                        if err != nil {
                                return nil, err
                        }
                        res = append(res, stat)
                }
                return res, nil

        case "Stat":
+               fmt.Printf("Stat: %s\n", realPath)
+               f, err := os.Lstat(realPath)
+               if f.Mode()&os.ModeSymlink == os.ModeSymlink {
+                       symlink, err := os.Readlink(realPath)
+                       if err != nil {
+                               fmt.Printf("Stat: could not resolve link %s: %v\n", realPath, err)
+                               return nil, err
+                       }
+                       res, err := fs.getRelativePath(symlink)
+                       if err != nil {
+                               fmt.Printf("Stat: symlink %s could not be looked up on chroot: %v\n", symlink, err)
+                               return nil, err
+                       }
+                       realPath = res
+               }
+
                file, err := os.Stat(realPath)
                if err != nil {
+                       fmt.Printf("Stat %s returning error\n", realPath)
                        return nil, err
                }
+               fmt.Printf("Stat %s returning %v\n", realPath, file)
                return listerat{file}, nil

        case "Readlink":
                symlink, err := os.Readlink(realPath)
                if err != nil {
                        return nil, err
                }
                res, err := fs.getRelativePath(symlink)

With that the TestChrootStat test fails as one would expect:

$ go test -run "TestChrootStat*"
[ .. snipped other test output .. ]
TestChrootStat: stat outlink
getRealPath /outlink
getRealPath with chroot: /tmp/sftp1226763008/sftp/outlink
getRealPath filePath.Rel -> outlink
getRealPath: strings.HasPrefix(outlink, ../) -> false
getRealPath returning /tmp/sftp1226763008/sftp/outlink
Stat: /tmp/sftp1226763008/sftp/outlink
getRelativePath: err: <nil> and strings.HasPrefix(../outbound, ../) == true
Stat: symlink /tmp/sftp1226763008/outbound could not be looked up on chroot: invalid argument
--- FAIL: TestChrootStat (0.00s)
    request-chroot_test.go:101:
            Error Trace:    /user/kode/sftp/request-chroot_test.go:101
            Error:          Received unexpected error:
                            file does not exist
            Test:           TestChrootStat
    request-chroot_test.go:115:
            Error Trace:    /user/kode/sftp/request-chroot_test.go:115
            Error:          Received unexpected error:
                            sftp: "invalid argument" (SSH_FX_FAILURE)
            Test:           TestChrootStat
FAIL
exit status 1
$

Sorry for the noise. I think your arguments for closing this PR stands.

[puellanivis]

Thanks for the check! It’s always valuable to validate assumptions!

I’ll go ahead and close this, but if anyone wants to pick it up again knowing just how complex the problem is, and willing to deal with all the seriously nitpicky pedantic nonsense required, I’ll be happy to take another look.