File operations do not defend against symlink traversal, permitting trivial escape from read-write mounts

[neild]

(I have been asked to report this bug via the public issue tracker.)

wazero does not appear to defend against symlink traversal in its filesystem APIs.

Since the WASI filesystem API permits creating symlinks, this allows for trivial access to the full filesystem from any program with a read-write mount. For example:

package main

import (
	"fmt"
	"io"
	"log"
	"os"
)

func main() {
	if err := os.Symlink("../../../../../../etc/passwd", "a"); err != nil {
		log.Fatal(err)
	}
	f, err := os.Open("a")
	if err != nil {
		log.Fatal(err)
	}
	b, _ := io.ReadAll(f)
	fmt.Println(string(b))
}

Run this program with read-write access to some directory:

wazero run -mount=restricted:/ main.wasm

It prints out the contents of /etc/passwd. /etc/passwd hasn't been exciting for a couple decades, admittedly, but this can be used to read or write any file anywhere on the filesystem that the account running wazero has access to.

[ncruces]

We need to decide if the issue is traversing the root at all, or if the issue is creating new symlinks that do.

What does WASI specify? Does it even specify this at all? It does:
https:/WebAssembly/wasi-filesystem/blob/main/path-resolution.md#host-implementation

W.r.t. security, I don't speak for the team, but I'd like to say that I consider (1) sandboxing WASI a much (much!) harder effort than (2) sandboxing Wasm. And I feel that the level of effort that was put into (1) is lower than (2), both within wazero development, but also in terms of WASI and Wasm design to begin with.

So if you really care about security, yeah, you maybe shouldn't be using WASI preview 1.

[neild]

Note that simply preventing creating symlinks that traverse the root isn't going to suffice: An attacker can create a symlink that doesn't traverse the root (a/b/c/d/../../../etc/passwd), and then rename the containing directory (rename a/b/c/d => d).

Preventing creation of symlinks with any .. elements might suffice to prevent creating new escapes.

[ncruces]

Just leaving the code pointer here:

wazero/internal/sysfs/dirfs_supported.go

Lines 36 to 42 in dc058c0

	func (d *dirFS) Symlink(oldName, link string) experimentalsys.Errno {
	// Note: do not resolve `oldName` relative to this dirFS. The link result is always resolved
	// when dereference the `link` on its usage (e.g. readlink, read, etc).
	// https:/bytecodealliance/cap-std/blob/v1.0.4/cap-std/src/fs/dir.rs#L404-L409
	err := os.Symlink(oldName, d.join(link))
	return experimentalsys.UnwrapOSError(err)
	}

We can probably enforce some minimal rules here, like that running path.Clean(link) doesn't change link, and that link doesn't start with ../.

It's likely that these checks should be inside join. I find the rational that ../ for path should be supported rather jarring:

wazero/internal/sysfs/dirfs.go

Lines 87 to 99 in dc058c0

	func (d *dirFS) join(path string) string {
	switch path {
	case "", ".", "/":
	if d.cleanedDir == "/" {
	return "/"
	}
	// cleanedDir includes an unnecessary delimiter for the root path.
	return d.cleanedDir[:len(d.cleanedDir)-1]
	}
	// TODO: Enforce similar to safefilepath.FromFS(path), but be careful as
	// relative path inputs are allowed. e.g. dir or path == ../
	return d.cleanedDir + path
	}

I mean: do we even need symlinks to evade the "root" at this point (the same join is used for OpenFile)?

[evacchi]

if we do I think we might have to skip some wasi tests (but that might be ok)

[ncruces]

Would you mind testing b36a8c8, @neild, @evacchi?

I think all our current tests pass. This only allows creating symlinks that go down, not up (returns EFAULT otherwise, which seems appropriate).

Please discuss. This is just food for thought, not even a PR.

[ncruces]

Having slept through this (not much admittedly), b36a8c8 has two issues with it.

Symlink must also check that the cleaned path doesn't start with / either. Also, it doesn't need to modify the path, I think: leaving the path as is is fine, as long as the symlink always points down (actually, if we go with the lexical solution, it does, or we get into the same issues as below).

For join, we need to make a judgement call.

Lexically cleaning the path with path.Clean violates semantics: /a/../b is only the same as /b if /a exists and is a directory; if /a is a symlink to a directory the path really means ${target}/../b; and if it's neither it's an invalid path.

Not lexically cleaning the path, however, is a security issue if (e.g.) /a is a symlink to our root, because then ${target}/../b escapes the root.

BTW, unless I'm missing something, I don't think the openat family of calls protects against either of these issues.

The only complete solution ISTM would be for our file system to implement path resolution all by itself.

[ncruces]

So, our options seem to be:

1. semantically wrong, lexical path resolution (path.Clean)
2. possible root escapes in the presence of symlinks (even if we don't allow creating symlinks at all)
3. a bunch of syscalls on every file system access to do path resolution ourselves (openat family of syscalls may help here)

I'm inclined to go with 1, b36a8c8 is a very close start.
I can't really justify the effort of 3 at this point.

[ncruces]

I'm starting to think I jumped the gun on this.

We explicitly document that escaping the mount point via relative path lookups is allowed:

wazero/fsconfig.go

Lines 64 to 88 in 111c51a

	type FSConfig interface {
	// WithDirMount assigns a directory at `dir` to any paths beginning at
	// `guestPath`.
	//
	// For example, `dirPath` as / (or c:\ in Windows), makes the entire host
	// volume writeable to the path on the guest. The `guestPath` is always a
	// POSIX style path, slash (/) delimited, even if run on Windows.
	//
	// If the same `guestPath` was assigned before, this overrides its value,
	// retaining the original precedence. See the documentation of FSConfig for
	// more details on `guestPath`.
	//
	// # Isolation
	//
	// The guest will have full access to this directory including escaping it
	// via relative path lookups like "../../". Full access includes operations
	// such as creating or deleting files, limited to any host level access
	// controls.
	//
	// # os.DirFS
	//
	// This configuration optimizes for WASI compatibility which is sometimes
	// at odds with the behavior of os.DirFS. Hence, this will not behave
	// exactly the same as os.DirFS. See /RATIONALE.md for more.
	WithDirMount(dir, guestPath string) FSConfig

If you have any suggestions on how we could make this more clear, I'm happy to hear them.
But at this point, changing this would be a compatibility break.

#2322 will remain a draft a little longer as we think about this, but I'm now inclined to just withdraw it.

You can use WithFSMount to get a more locked down alternative (although, please read the docs).

Or you can type assert sysfs.FSConfig and use WithSysFSMount. Then you can provide your own sys.FS implementation that implements your own security policies. You could, for instance, wrap our implementation and lexically clean and or validate paths, and disallow symlink creation (or implement the logic I did in #2322).

[ncruces]

In general the doc we should be reading about path resolution semantics we should be reading is:
https:/WebAssembly/wasi-filesystem/blob/main/path-resolution.md#host-implementation

I hate it that I didn't find it sooner, but I hadn't worked on WASI filesystem directly myself before.

[tsmethurst]

Bit tangential perhaps but if you know ahead of time what your filenames will be, and what permissions you want to give your wasm application on them, you can implement quite a strict filesystem like this:

https:/superseriousbusiness/gotosocial/blob/53ee6aef0885b4055ef95bf4f20ee78fd381e333/internal/media/util.go#L34-L77

And then give a config like this:

https:/superseriousbusiness/gotosocial/blob/53ee6aef0885b4055ef95bf4f20ee78fd381e333/internal/media/ffmpeg.go#L242-L256

Or like this:

https:/superseriousbusiness/gotosocial/blob/53ee6aef0885b4055ef95bf4f20ee78fd381e333/internal/media/ffmpeg.go#L158-L192

That's how we locked this down as much as possible in GtS anyway, though it may not apply for your use-case.

[ncruces]

Yep. I have this extreme example I used for a tool that reads from a single file (because it wants to seek, so doesn't like stdin, but is willing to output to stdout):
https:/ncruces/RethinkRAW/blob/master/pkg/dcraw/fs.go

[ncruces]

As discussed above, this is working as specified in the documentation.

I opened #2323 to track implementing path resolution, which would fix this.