Repository: pkg/sftp
Pull request #526 (2022-09-29 10:02:23)

request server: handle relative symlinks
The request server implementation does not handle symlinks correctly. If the link target is relative it is rewritten to an absolute path. The first commit fixes that.

The second commit is just adding a testcase for linking to non-existent files.

drakkan: I don't have time to test this change now (I'll do it later today or this weekend), I think this change will allow symlinks outside the base directory. Applications enforcing a chroot such as SFTPGo should check the symlink target and normalize it themselves after this change. This is acceptable but maybe should be documented as a backward incompatible change.
georgmu: Just as a note: The last comment and tests were about the current SFTPGo implementation without the relative symlink patch applied to sftp.
drakkan: I think this was broken by me [here](https://github.com/pkg/sftp/commit/98b35dcfc3d7db2830c9b69a4ab0bed0049e01c9#diff-3c2feffcf57ffb6c596dc5608186c518252092b7017be5189d3dc12aa3c45cecL171). @georgmu  ccan you please revert that commit (or revert to the previous behavior) and confirm that your use case is working?
drakkan: I've done some testing and this patch looks good to me. I think we just need to document somewhere that Filepath can be relative for symbolic links now. @puellanivis do you have any suggestions other than the above for test cases?
drakkan: @georgmu please add some test cases for relative links from a dir different from `/`, something like this:

```
sftp> pwd
Remote working directory: /dir
sftp> symlink link1.txt sub/file.txt
sftp> symlink sub/link2.txt file.txt
```

Thanks
puellanivis: > I've done some testing and this patch looks good to me. I think we just need to document somewhere that Filepath can be relative for symbolic links now. @puellanivis do you have any suggestions other than the above for test cases?

Nope, not really. Tried my hardest, and only the test code was something I could see anything at all in.
georgmu: I will add some more testcases, also the one mentioned in my comment about moving the parent directory. I will also try the testcases with 98b35dcf reverted (just to check that it worked before).
georgmu: ```
sftp> pwd
Remote working directory: /dir
sftp> symlink link1.txt sub/file.txt
sftp> symlink sub/link2.txt file.txt
```

just to make sure what these commands do as checked with sftp CLI client and standard sftp server:
* `symlink link1.txt sub/file.txt` will create symlink `/dir/sub/file.txt`
  * `/dir/sub` has to exist
  * link target: `link1.txt` (which would effectively be file `/dir/sub/link1.txt`)
* `symlink sub/link2.txt file.txt` will create symlink `/dir/file.txt`
  * `/dir/sub` does not have to exist
  *  link target: `sub/link2.txt` (which would effectively be file `/dir/sub/link2.txt`)

I would reverse the names so that the symlinks are named `link*.txt` and the files they are pointing to named `file*.txt`
drakkan: > ```
> sftp> pwd
> Remote working directory: /dir
> sftp> symlink link1.txt sub/file.txt
> sftp> symlink sub/link2.txt file.txt
> ```
> 
> just to make sure what these commands do as checked with sftp CLI client and standard sftp server:
> 
> * `symlink link1.txt sub/file.txt` will create symlink `/dir/sub/file.txt`
>   
>   * `/dir/sub` has to exist
>   * link target: `link1.txt` (which would effectively be file `/dir/sub/link1.txt`)
> * `symlink sub/link2.txt file.txt` will create symlink `/dir/file.txt`
>   
>   * `/dir/sub` does not have to exist
>   * link target: `sub/link2.txt` (which would effectively be file `/dir/sub/link2.txt`)
> 
> I would reverse the names so that the symlinks are named `link*.txt` and the files they are pointing to named `file*.txt`

Yes, reverse the name, thanks
georgmu: > @georgmu please add some test cases for relative links from a dir different from `/`, something like this:
> 
> ```
> sftp> pwd
> Remote working directory: /dir
> 
> Thanks

As I understand sftp, `pwd` is solely a client feature keeping track of the current directory. So the unit tests can only use `/dir/sub/...` etc. ...
drakkan: > > @georgmu please add some test cases for relative links from a dir different from `/`, something like this:
> > ```
> > sftp> pwd
> > Remote working directory: /dir
> > 
> > Thanks
> > ```
> 
> As I understand sftp, `pwd` is solely a client feature keeping track of the current directory. So the unit tests can only use `/dir/sub/...` etc. ...

Yes, you are right, you already included a test like this:

```
err = p.cli.Symlink("f1", "/subdir/linked_f1")
require.NoError(t, err)
```

I added such tests to SFTPGo because I have to handle other things there
georgmu: One more thing here:
I was also testing Readlink() results and they are manipulated as well for absolute links.

From my understanding, a symlink is just a textfile whose content gets interpreted by the virtual file system. So if I write `/foo/bar` into a textfile I expect the content to be `/foo/bar` whenever I read it.

chroot'ing does not alter the content ("target") of the link, only the interpretation of the content.
* lets assume a symlink `/subdir/link.txt` with link target `/foo/bar`
* when reading `/subdir/link.txt` the content of `/foo/bar` is returned
* when in a `chroot /subdir`, reading `/link.txt` returns the content of `/foo/bar` in the chroot, which is effectively `/subdir/foo/bar` from outside of the chroot

This is how symlinks work and I think the library should do the same: not interpreting the link target.

What a chroot-in-software (what SFTPGo does) should do sanity checks whenever reading a link (like interpreting absolute links as links from the chroot and check that relative links do not escape the chroot), but the library should return them as is.

drakkan: > One more thing here: I was also testing Readlink() results and they are manipulated as well for absolute links.
> 
> From my understanding, a symlink is just a textfile whose content gets interpreted by the virtual file system. So if I write `/foo/bar` into a textfile I expect the content to be `/foo/bar` whenever I read it.
> 
> chroot'ing does not alter the content ("target") of the link, only the interpretation of the content.
> 
> * lets assume a symlink `/subdir/link.txt` with link target `/foo/bar`
> * when reading `/subdir/link.txt` the content of `/foo/bar` is returned
> * when in a `chroot /subdir`, reading `/link.txt` returns the content of `/foo/bar` in the chroot, which is effectively `/subdir/foo/bar` from outside of the chroot
> 
> This is how symlinks work and I think the library should do the same: not interpreting the link target.
> 

The library should pass the cleaned path for readlink requests, the request server implementations (SFTPGo or others) can handle the request according to their own logic, which could be the one you describe or a different one

> What a chroot-in-software (what SFTPGo does) should do sanity checks whenever reading a link (like interpreting absolute links as links from the chroot and check that relative links do not escape the chroot), but the library should return them as is.

SFTPGo resolves the links and if it is outside the chroot directory it will return an error like this:

```
"level":"error","time":"2022-09-30T15:56:24.580","sender":"osfs","connection_id":"43bea5101a92edede8edbeebdec1ded65406186c4b875013eed73d1c332de9c5","message":"Invalid path resolution, path \"/tmp/test_home/foo/bar\" original path \"/link.txt\" resolved \"/tmp/test_home/subdir/link.txt\" err: Path resolution error: path \"/tmp/test_home/foo/bar\" is not inside \"/tmp/test_home/subdir\""}
```
georgmu: > The library should pass the cleaned path for readlink requests

Currently, it returns a relative path, even if an absolute path is generated.
```
p.cli.Symlink("/foo", "/bar")
...
p.cli.Readlink("/bar") // <- "foo" instead of "/foo" 
```

I just checked the code: openssh sftp-server also just returns the content of `readlink(2)` and `ln` does not clean the symlink when generating one (`ln -s /a/b/../c foo` will have exactly `/a/b/../c` as target instead of cleaned `/a/c`.

georgmu: The problem with `ReadLink` is here: https://github.com/pkg/sftp/blob/3aa4378d78d53fd5b7fad0b4ad0f0cd61ef2fee3/request.go#L584

file.Name() only returns the base name of the file (as specified in docs for os.FileInfo), but this is wrong here.
drakkan: ah ok, now I better understand what you mean. In SFTPGo I use this workaround

https://github.com/drakkan/sftpgo/blob/main/internal/sftpd/handler.go#L263

I have custom fileinfo and for the Readlink case I set fullName to `true` and so `Name()` returns the full path

https://github.com/drakkan/sftpgo/blob/1e21aa945338dae031fcf49495f2f802fcc0ffd6/internal/vfs/fileinfo.go#L37

I agree we should fix this issue in a better way (I use the same hack also for FTP).
drakkan: > > The library should pass the cleaned path for readlink requests
> 
> Currently, it returns a relative path, even if an absolute path is generated.
> 
> ```
> p.cli.Symlink("/foo", "/bar")
> ...
> p.cli.Readlink("/bar") // <- "foo" instead of "/foo" 
> ```
> 
> I just checked the code: openssh sftp-server also just returns the content of `readlink(2)` and `ln` does not clean the symlink when generating one (`ln -s /a/b/../c foo` will have exactly `/a/b/../c` as target instead of cleaned `/a/c`.

As for the server side path normalization, the library always cleaned paths (I'm not the original author), the only expection is the request.Filepath for symlinks introduced in this PR. I will discuss this in a separate issue/PR 
georgmu: I just uploaded a solution for both symlink and readlink stuff with extended test cases.

The readlink fix is a bit hacky and requires some documentation and/or another type.
drakkan: I'm not sure about the readlink fix. You have to use a custom fileinfo implementing the new `LinkReader` interface.

Readlink is currently broken, I think we can break backward compatibility for something that does not work or that works with hacks like the one I used in SFTPGo. @puellanivis what do you think about? (I already know you disagree :smile: )
drakkan: @georgmu I'm now using your initial patch in SFTPGo

https://github.com/drakkan/sftpgo/commit/0e8c41bbd1e861d3261852536d408ac88c7eba47#diff-d48bed73cb243b5300108a318eea6494652d40093322e498e5fd5d098725ef33
https://github.com/drakkan/sftp/commit/e8c89afc13a78badebfd48f90b3ccefb5223017f

thanks!
drakkan: We could do something like this:

```
diff --git a/request-interfaces.go b/request-interfaces.go
index 2e5ee6b..ab5f5a1 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -94,7 +94,7 @@ type LstatFileLister interface {
 //
 // Up to v1.13.5 the signature for the RealPath method was:
 //
-// RealPath(string) string
+// # RealPath(string) string
 //
 // we have added a legacyRealPathFileLister that implements the old method
 // to ensure that your code does not break.
@@ -104,6 +104,13 @@ type RealPathFileLister interface {
        RealPath(string) (string, error)
 }
 
+// ReadlinkFileLister is a FileLister that implements the Readlink method.
+// TODO: complete docs
+type ReadlinkFileLister interface {
+       FileLister
+       Readlink(string) (string, error)
+}
+
 // This interface is here for backward compatibility only
 type legacyRealPathFileLister interface {
        FileLister
diff --git a/request.go b/request.go
index 6a7e6cf..4db3f54 100644
--- a/request.go
+++ b/request.go
@@ -295,7 +295,25 @@ func (r *Request) call(handlers Handlers, pkt requestPacket, alloc *allocator, o
                return filecmd(handlers.FileCmd, r, pkt)
        case "List":
                return filelist(handlers.FileList, r, pkt)
-       case "Stat", "Lstat", "Readlink":
+       case "Stat", "Lstat":
+               return filestat(handlers.FileList, r, pkt)
+       case "Readlink":
+               if readlinkFileLister, ok := handlers.FileList.(ReadlinkFileLister); ok {
+                       resolved, err := readlinkFileLister.Readlink(r.Filepath)
+                       if err != nil {
+                               return statusFromError(pkt.id(), err)
+                       }
+                       return &sshFxpNamePacket{
+                               ID: pkt.id(),
+                               NameAttrs: []*sshFxpNameAttr{
+                                       {
+                                               Name:     resolved,
+                                               LongName: resolved,
+                                               Attrs:    emptyFileStat,
+                                       },
+                               },
+                       }
+               }
                return filestat(handlers.FileList, r, pkt)
        default:
                return statusFromError(pkt.id(), fmt.Errorf("unexpected method: %s", r.Method))
```

@puellanivis do you have other/better suggestions? thanks
drakkan: I try to summarize:

- the initial patch from @georgmu (https://github.com/georgmu/sftp/commit/9bc346c344e7dff1c6531883c265a294a040d4ea) looks ok to me and to @puellanivis 
- while writing test cases @georgmu discovered a bug in Readlink handling that I was aware of but I was too lazy to report and fix in the past (I applied a hacky workaround in SFTPGo)
- to fix Readlink in a backward compatible way I proposed to add this optional interface:

```
type ReadlinkFileLister interface {
	FileLister
	Readlink(string) (string, error)
}
```

we'll break backward compatibility in v2

drakkan: @georgmu are you interested to add the `ReadlinkFileLister` interface as discussed? I'm busy with many other things and I don't care about the authorship of the code pasted above. Thanks
georgmu: > @georgmu are you interested to add the `ReadlinkFileLister` interface as discussed? I'm busy with many other things and I don't care about the authorship of the code pasted above. Thanks

Sure. I will prepare the commits.
drakkan: > I prepared a commit for the `ReadlinkFileLister`. I am not the best when it comes to documentation. If I should extend the notes, please just give me a hint on what to improve.

Thanks! You are much better than me at writing docs :smile: The PR LGTM. Maybe you can also test returning an error from a readlink request. There is no coverage:

![Schermata del 2022-10-12 12-28-28](https://user-images.githubusercontent.com/553263/195319690-b53403f0-8d07-4d97-878c-77df567356c0.png)



Patch:
diff --git a/request-example.go b/request-example.go
index ab6c675..519b3b7 100644
--- a/request-example.go
+++ b/request-example.go
@@ -391,21 +391,6 @@ func (fs *root) Filelist(r *Request) (ListerAt, error) {
 			return nil, err
 		}
 		return listerat{file}, nil
-
-	case "Readlink":
-		symlink, err := fs.readlink(r.Filepath)
-		if err != nil {
-			return nil, err
-		}
-
-		// SFTP-v2: The server will respond with a SSH_FXP_NAME packet containing only
-		// one name and a dummy attributes value.
-		return listerat{
-			&memFile{
-				name: symlink,
-				err:  os.ErrNotExist, // prevent accidental use as a reader/writer.
-			},
-		}, nil
 	}
 
 	return nil, errors.New("unsupported")
@@ -434,7 +419,7 @@ func (fs *root) readdir(pathname string) ([]os.FileInfo, error) {
 	return files, nil
 }
 
-func (fs *root) readlink(pathname string) (string, error) {
+func (fs *root) Readlink(pathname string) (string, error) {
 	file, err := fs.lfetch(pathname)
 	if err != nil {
 		return "", err
@@ -525,8 +510,8 @@ func (fs *root) exists(path string) bool {
 	return err != os.ErrNotExist
 }
 
-func (fs *root) fetch(path string) (*memFile, error) {
-	file, err := fs.lfetch(path)
+func (fs *root) fetch(pathname string) (*memFile, error) {
+	file, err := fs.lfetch(pathname)
 	if err != nil {
 		return nil, err
 	}
@@ -537,7 +522,12 @@ func (fs *root) fetch(path string) (*memFile, error) {
 			return nil, errTooManySymlinks
 		}
 
-		file, err = fs.lfetch(file.symlink)
+		linkTarget := file.symlink
+		if !path.IsAbs(linkTarget) {
+			linkTarget = path.Join(path.Dir(file.name), linkTarget)
+		}
+
+		file, err = fs.lfetch(linkTarget)
 		if err != nil {
 			return nil, err
 		}
diff --git a/request-interfaces.go b/request-interfaces.go
index 2e5ee6b..75c6c39 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -74,6 +74,11 @@ type StatVFSFileCmder interface {
 // FileLister should return an object that fulfils the ListerAt interface
 // Note in cases of an error, the error text will be sent to the client.
 // Called for Methods: List, Stat, Readlink
+//
+// Since Filelist returns an os.FileInfo, this can make it non-ideal for impelmenting Readlink.
+// This is because the Name receiver method defined by that interface defines that it should only return the base name.
+// However, Readlink is required to be capable of returning essentially any arbitrary valid path relative or absolute.
+// In order to implement this more expressive requirement, implement [ReadlinkFileLister] which will then be used instead.
 type FileLister interface {
 	Filelist(*Request) (ListerAt, error)
 }
@@ -94,7 +99,7 @@ type LstatFileLister interface {
 //
 // Up to v1.13.5 the signature for the RealPath method was:
 //
-// RealPath(string) string
+// # RealPath(string) string
 //
 // we have added a legacyRealPathFileLister that implements the old method
 // to ensure that your code does not break.
@@ -104,6 +109,14 @@ type RealPathFileLister interface {
 	RealPath(string) (string, error)
 }
 
+// ReadlinkFileLister is a FileLister that implements the Readlink method.
+// By implementing the Readlink method, it is possible to return any arbitrary valid path relative or absolute.
+// This allows giving a better response than via the default FileLister (which is limited to os.FileInfo, whose Name method should only return the base name of a file)
+type ReadlinkFileLister interface {
+	FileLister
+	Readlink(string) (string, error)
+}
+
 // This interface is here for backward compatibility only
 type legacyRealPathFileLister interface {
 	FileLister
diff --git a/request.go b/request.go
index 116c27a..57d788d 100644
--- a/request.go
+++ b/request.go
@@ -187,6 +187,7 @@ func requestFromPacket(ctx context.Context, pkt hasPath, baseDir string) *Reques
 		// NOTE: given a POSIX compliant signature: symlink(target, linkpath string)
 		// this makes Request.Target the linkpath, and Request.Filepath the target.
 		request.Target = cleanPathWithBase(baseDir, p.Linkpath)
+		request.Filepath = p.Targetpath
 	case *sshFxpExtendedPacketHardlink:
 		request.Target = cleanPathWithBase(baseDir, p.Newpath)
 	}
@@ -294,7 +295,12 @@ func (r *Request) call(handlers Handlers, pkt requestPacket, alloc *allocator, o
 		return filecmd(handlers.FileCmd, r, pkt)
 	case "List":
 		return filelist(handlers.FileList, r, pkt)
-	case "Stat", "Lstat", "Readlink":
+	case "Stat", "Lstat":
+		return filestat(handlers.FileList, r, pkt)
+	case "Readlink":
+		if readlinkFileLister, ok := handlers.FileList.(ReadlinkFileLister); ok {
+			return readlink(readlinkFileLister, r, pkt)
+		}
 		return filestat(handlers.FileList, r, pkt)
 	default:
 		return statusFromError(pkt.id(), fmt.Errorf("unexpected method: %s", r.Method))
@@ -598,6 +604,23 @@ func filestat(h FileLister, r *Request, pkt requestPacket) responsePacket {
 	}
 }
 
+func readlink(readlinkFileLister ReadlinkFileLister, r *Request, pkt requestPacket) responsePacket {
+	resolved, err := readlinkFileLister.Readlink(r.Filepath)
+	if err != nil {
+		return statusFromError(pkt.id(), err)
+	}
+	return &sshFxpNamePacket{
+		ID: pkt.id(),
+		NameAttrs: []*sshFxpNameAttr{
+			{
+				Name:     resolved,
+				LongName: resolved,
+				Attrs:    emptyFileStat,
+			},
+		},
+	}
+}
+
 // init attributes of request object from packet data
 func requestMethod(p requestPacket) (method string) {
 	switch p.(type) {


Test patch:
diff --git a/request-server_test.go b/request-server_test.go
index 8eb0916..ab74de6 100644
--- a/request-server_test.go
+++ b/request-server_test.go
@@ -568,27 +568,67 @@ func TestRequestSymlink(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
 
-	_, err := putTestFile(p.cli, "/foo", "hello")
-	require.NoError(t, err)
+	const CONTENT_FOO = "hello"
+	const CONTENT_DIR_FILE_TXT = "file"
+	const CONTENT_SUB_FILE_TXT = "file-in-sub"
 
-	err = p.cli.Symlink("/foo", "/bar")
+	// prepare all files
+	_, err := putTestFile(p.cli, "/foo", CONTENT_FOO)
 	require.NoError(t, err)
-	err = p.cli.Symlink("/bar", "/baz")
+	err = p.cli.Mkdir("/dir")
+	require.NoError(t, err)
+	err = p.cli.Mkdir("/dir/sub")
+	require.NoError(t, err)
+	_, err = putTestFile(p.cli, "/dir/file.txt", CONTENT_DIR_FILE_TXT)
+	require.NoError(t, err)
+	_, err = putTestFile(p.cli, "/dir/sub/file-in-sub.txt", CONTENT_SUB_FILE_TXT)
 	require.NoError(t, err)
 
-	r := p.testHandler()
+	type symlink struct {
+		name   string // this is the filename of the symbolic link
+		target string // this is the file or directory the link points to
 
-	fi, err := r.lfetch("/bar")
-	require.NoError(t, err)
-	assert.True(t, fi.Mode()&os.ModeSymlink == os.ModeSymlink)
+		//for testing
+		expectsNotExist     bool
+		expectedFileContent string
+	}
 
-	fi, err = r.lfetch("/baz")
-	require.NoError(t, err)
-	assert.True(t, fi.Mode()&os.ModeSymlink == os.ModeSymlink)
+	symlinks := []symlink{
+		{name: "/bar", target: "/foo", expectedFileContent: CONTENT_FOO},
+		{name: "/baz", target: "/bar", expectedFileContent: CONTENT_FOO},
+		{name: "/link-to-non-existent-file", target: "non-existent-file", expectsNotExist: true},
+		{name: "/dir/rel-link.txt", target: "file.txt", expectedFileContent: CONTENT_DIR_FILE_TXT},
+		{name: "/dir/abs-link.txt", target: "/dir/file.txt", expectedFileContent: CONTENT_DIR_FILE_TXT},
+		{name: "/dir/rel-subdir-link.txt", target: "sub/file-in-sub.txt", expectedFileContent: CONTENT_SUB_FILE_TXT},
+		{name: "/dir/abs-subdir-link.txt", target: "/dir/sub/file-in-sub.txt", expectedFileContent: CONTENT_SUB_FILE_TXT},
+		{name: "/dir/sub/parentdir-link.txt", target: "../file.txt", expectedFileContent: CONTENT_DIR_FILE_TXT},
+	}
 
-	content, err := getTestFile(p.cli, "/baz")
-	require.NoError(t, err)
-	assert.Equal(t, []byte("hello"), content)
+	for _, s := range symlinks {
+		err := p.cli.Symlink(s.target, s.name)
+		require.NoError(t, err, "Creating symlink %q with target %q failed", s.name, s.target)
+
+		rl, err := p.cli.ReadLink(s.name)
+		require.NoError(t, err, "ReadLink(%q) failed", s.name)
+		require.Equal(t, s.target, rl, "Unexpected result when reading symlink %q", s.name)
+	}
+
+	// test fetching via symlink
+	r := p.testHandler()
+
+	for _, s := range symlinks {
+		fi, err := r.lfetch(s.name)
+		require.NoError(t, err, "lfetch(%q) failed", s.name)
+		require.True(t, fi.Mode()&os.ModeSymlink == os.ModeSymlink, "Expected %q to be a symlink but it is not.", s.name)
+
+		content, err := getTestFile(p.cli, s.name)
+		if s.expectsNotExist {
+			require.True(t, os.IsNotExist(err), "Reading symlink %q expected os.ErrNotExist", s.name)
+		} else {
+			require.NoError(t, err, "getTestFile(%q) failed", s.name)
+			require.Equal(t, []byte(s.expectedFileContent), content, "Reading symlink %q returned unexpected content", s.name)
+		}
+	}
 
 	checkRequestServerAllocator(t, p)
 }
@@ -714,9 +754,17 @@ func TestRequestReadlink(t *testing.T) {
 	require.NoError(t, err)
 	err = p.cli.Symlink("/foo", "/bar")
 	require.NoError(t, err)
+
 	rl, err := p.cli.ReadLink("/bar")
 	assert.NoError(t, err)
-	assert.Equal(t, "foo", rl)
+	assert.Equal(t, "/foo", rl)
+
+	_, err = p.cli.ReadLink("/foo")
+	assert.Error(t, err, "Readlink on non-symlink should fail")
+
+	_, err = p.cli.ReadLink("/foobar")
+	assert.Error(t, err, "Readlink on non-existent file should fail")
+
 	checkRequestServerAllocator(t, p)
 }