Repository: tetratelabs/wazero
Pull request #1266 (2023-03-21 18:53:55)

wasi: enforce EPERM on malformed paths
It seems WASI now forbids root and relative paths on the at file descriptor and returns EPERM otherwise. This enforces the following:

* require EPERM when you escape a directory (../)
* require EPERM if you add a leading slash (absolute path)
* require ENOENT if you add a trailing slash to a file
* require success if you add a trailing slash to a directory

See https://github.com/WebAssembly/wasi-testsuite/pull/67

codefromthecrypt: @jerbob92 @achille-roussel @Pryz @johanbrandhorst so, here's the deal please correct me if I'm wrong.

The new check added to wasi-testsuite seems to:
* require EPERM when you escape a directory (../)
* require EPERM if you add a leading slash (absolute path)
* require ENOENT if you add a trailing slash to a file
* require success if you add a trailing slash to a directory

There are users who won't be affected:
* wazero users of fs.FS (as we clean the path before doing anything to remove any relative stuff). These users would already break because fs.FS requires you are not allowed to escape your directory (fs.ValidPath)
* users of compilers who use wasi-libc directly (e.g. rust and tinygo), as internally wasi-libc cleans paths

There are users who will be affected:
* direct users of wasi, even wasi-libc who avoid the logic that cleans the path for them
* compilers who don't use wasi-libc or don't use wasi-libc to handle pre-opens (notably zig)

We should make a call whether to enforce this or not. There are some downsides:
* EPERM is not an intuitive error code, so would be confusing for people especially when they mount root and have a root path (e.g. using Zig)
* The cost to enforce this exists, as we have to both clean the path and also run the fs.ValidPath, while at the same time placing the trailing slash back. This will end up allocating strings for compilers that don't clean ahead of time.

If folks say we do it, I can get the tests passing here even work around the broken wasi-testsuite until they fix their repo.
codefromthecrypt: For Zig users main thing is it will break their existing wasm to enforce this, especially if they have code like in our example. Maybe in our release notes we can suggest an alternative to using absolute path, as there may be other runtimes besides wasmtime that intentionally break on leading slash (even when the pre-open is root!) cc @knqyf263

```zig
        // open the file from its absolute path, as "/" is pre-opened.
        const file = std.fs.openFileAbsolute(arg, .{ .mode = .read_only }) catch |err| {
            warn("Unable to open file {s}: {s}\n", .{ arg, @errorName(err) });
            return err;
        };
        defer file.close();
```
achille-roussel: It looks like atPath isn't used on the new path in path_symlink?

Beyond the scope of this change but somewhat related, wasmtime also gives EPERM if the old path in path_symlink is absolute... but this is rabbit hole that brings so many usability questions that it probably deserves a thoughtful discussion dedicated to it. 
codefromthecrypt: @achille-roussel maybe you are hinting that a future change could restrict more and we'd not comply again, right? specifically on the symlink target, but possibly something else.

I think for now, main thing is helping users making guests know that wasi might break them. seems @jerbob92 already worked around this. We have nagged zig folks. Hopefully wasi will start engaging wider ecosystem instead of relying on us to figure out the issues and do it on their behalf.
codefromthecrypt: so on Zig, basically it doesn't strip the leading slash (I also tried switching to cwd).

If a user passes an absolute path for a file under the root pre-open, that path would fail with EPERM after this as the stdlib doesn't remove the slash.

We can fix our examples to stop people from passing absolute paths, but really I think this is a bad user experience to fail like this test assumes we should. I think it would be somewhat common for someone to mount root and pass an absolute path, iotw.

One way we can handle this is to strip the leading slash on behalf of users, but if we do that, we'll fail this wasi-testsuite test because we are tolerating something they want us to break users over.
jerbob92: @codefromthecrypt I don't really see why Zig couldn't do the same as wasi-libc or the same as Emscripten and me. 

wasi-libc doesn't block users from using absolute paths, but it translates it into a relative path + FD based on the pre-opens, it tries to match the longest prefix in the pre-opens. See [this](https://github.com/WebAssembly/wasi-libc/blob/a02298043ff551ce1157bc2ee7ab74c3bffe7144/libc-bottom-half/sources/preopens.c#L172) code how it does that. Eventually I want to implement something like that in my Emscripten WASI patch, but for that I also need to implement the WASI fd_prestat_get/fd_prestat_dir_name methods, which is a bit more work.

Me (Emscripten WASI) and Emscripten (JS) just removes the leading slash and assume the root FD (3).
codefromthecrypt: @jerbob92 main issue is the way to avoid this is to both update zig, and use that to re-compile existing code.

I might have lost track of emscripten, but I thought they didn't actually implement WASI and it was an open PR. So, basically it feels using WASI at all is quite custom in emscripten, so people doing that would be more used to recompiling as they are using a custom build in the first place.

I feel it is a bit different thing in Zig as it is built-in and this is behavior affecting also was never discussed. So, basically  Zig users will have to use an unreleased compiler to address this or change their call sites meanwhile. Their code will break until that happens, and I think that breaking third party code is generally not something people should feel ok doing unless for very good reason.

Regardless, of all of this, we still have a choice which is do we break with the currently known rules, or do we add the yet to be enforced by testsuite behaviors of wasmtime, knowing none of this is documented anywhere.
jerbob92: > @jerbob92 main issue is the way to avoid this is to both update zig, and use that to re-compile existing code.

Yes, that is the way if the Zig implementation is wrong, wasi `path_open` docs says that path should be relative to FD. 

> I might have lost track of emscripten, but I thought they didn't actually implement WASI and it was an open PR. So, basically it feels using WASI at all is quite custom in emscripten, so people doing that would be more used to recompiling as they are using a custom build in the first place.

They don't, I implemented that in my Emscripten patch, but they do have file handling (WASMFS) in their libc implementation, and they strip of the root and assume the root FD. 

What I am doing right now in my WASI patch is assuming root FD 3 when it starts with / and then strip of the /. I want to implement proper pre-opens later like wasi-libc.

> I feel it is a bit different thing in Zig as it is built-in and this is behavior affecting also was never discussed. So, basically Zig users will have to use an unreleased compiler to address this or change their call sites meanwhile. Their code will break until that happens, and I think that breaking third party code is generally not something people should feel ok doing unless for very good reason.

We could wait with releasing this until Zig has a proper patch + release in place. But don't other runtimes have checks for this already though? (otherwise they would also fail on the wasi tests) So does their code with absolute paths even work on those runtimes (like wasmtime)? 

> Regardless, of all of this, we still have a choice which is do we break with the currently known rules, or do we add the yet to be enforced by testsuite behaviors of wasmtime, knowing none of this is documented anywhere.

It appears that it is documented, it doesn't tell absolute paths are forbidden, but it does say it should be relative.
codefromthecrypt: @jerbob92 what I feel I can summarize your comment is that all the things in that wasi test are implied by 

> path: string The relative path of the file or directory to open, relative to the path_open::fd directory.

* require EPERM when you escape a directory (../)
* require EPERM if you add a leading slash (absolute path)
* require ENOENT if you add a trailing slash to a file
* require success if you add a trailing slash to a directory

The main issue is that "/" is not allowed to express a path relative to the dir FD. I think it is a bit of a stretch and just like you didn't initially guess this, and zig didn't either, it isn't 100pct cut and dry that the one sentence in wasi indicates all the above and the errno to return.
codefromthecrypt: I'm here with @ShivanshVij and @evacchi and I think that realistically speaking people in zig are more likely to be able to recompile. Hopefully, other languages don't complain, but in any case if things break the answer will be "talk to wasi"

I'll tidy this up and enforce EPERM and dodge the symlink issue until it becomes a part of the testsuite.
codefromthecrypt: windows is slightly different on the trailing slash rule... might need to figure out what is behind EIO as that is also used as a catch-all for unknown syscall.Errno

```
==> wasi_snapshot_preview1.path_open(fd=5,dirflags=,path=dir/nested/file/,oflags=,fs_rights_base=,fs_rights_inheriting=,fdflags=)
<== (opened_fd=,errno=EIO)
thread 'main' panicked at 'expected errno NOTDIR or NOENT; got IO', src/bin/interesting_paths.rs:48:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
codefromthecrypt: still hunting the windows syscall mismatch but hopefully get it soon
jerbob92: FYI for people reading along, me and @codefromthecrypt had a more in depth conversation on Slack.

> @jerbob92 what I feel I can summarize your comment is that all the things in that wasi test are implied by

No, not really, for me only the absolute path really matters. In my opinion is only says that the path should be relative, and thus not be absolute. It doesn't say anything about the other test cases and also not about the error that should be returned.

Basically we agree that wasmtime shouldn't define the specific behavior of these wasm methods in wasi-testsuite, but they do have to start from somewhere as there is no full spec yet, but there are implementations out there already so it probably best to spec that behaviour out of the different runtimes/compilers that currently exist. The wasi-testsuite can then be used to base the spec (in text) on. 

When new tests are added to the testsuite they should be validated against multiple runtimes/compilers to see if there is a difference in implementation out there, and then start up a discussion about what the actual implementation should be, as the docs leave a lot of room for interpretation and imagination. 
codefromthecrypt: I'm English as a first language, and I think that saying a path is relative to something else doesn't imply it must be expressed without a slash. To me, it means that the path is resolved relative to the FD. so "/foo" against the fd "/tmp" would be "/tmp/foo", "../foo" would be "/foo". Things beyond that aren't implied.

It is possible for wasi to document things, we spend hours on every thing and the answer is never to document. Go is far better and I hope the bar can raise. I like how they describe a similar concern. 
```
// ValidPath reports whether the given path name
// is valid for use in a call to Open.
//
// Path names passed to open are UTF-8-encoded,
// unrooted, slash-separated sequences of path elements, like “x/y/z”.
// Path names must not contain an element that is “.” or “..” or the empty string,
// except for the special case that the root directory is named “.”.
// Paths must not start or end with a slash: “/x” and “x/” are invalid.
```


Patch:
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index 9569bfe9..5cc4cb85 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -8,6 +8,7 @@ import (
 	"math"
 	"path"
 	"reflect"
+	"strings"
 	"syscall"
 	"unsafe"
 
@@ -1648,13 +1649,32 @@ func pathOpenFn(_ context.Context, mod api.Module, params []uint64) syscall.Errn
 //
 // See https://github.com/WebAssembly/wasi-libc/blob/659ff414560721b1660a19685110e484a081c3d4/libc-bottom-half/sources/at_fdcwd.c
 // See https://linux.die.net/man/2/openat
-func atPath(fsc *sys.FSContext, mem api.Memory, fd, path, pathLen uint32) (sysfs.FS, string, syscall.Errno) {
-	b, ok := mem.Read(path, pathLen)
+func atPath(fsc *sys.FSContext, mem api.Memory, fd, p, pathLen uint32) (sysfs.FS, string, syscall.Errno) {
+	b, ok := mem.Read(p, pathLen)
 	if !ok {
 		return nil, "", syscall.EFAULT
 	}
 	pathName := string(b)
 
+	// interesting_paths wants us to break on trailing slash if the input ends
+	// up a file, not a directory!
+	hasTrailingSlash := strings.HasSuffix(pathName, "/")
+
+	// interesting_paths includes paths that include relative links but end up
+	// not escaping
+	pathName = path.Clean(pathName)
+
+	// interesting_paths wants to break on root paths or anything that escapes.
+	// This part is the same as fs.FS.Open()
+	if !fs.ValidPath(pathName) {
+		return nil, "", syscall.EPERM
+	}
+
+	// add the trailing slash back
+	if hasTrailingSlash {
+		pathName = pathName + "/"
+	}
+
 	if f, ok := fsc.LookupFile(fd); !ok {
 		return nil, "", syscall.EBADF // closed
 	} else if _, ft, err := f.CachedStat(); err != nil {
diff --git a/internal/platform/errno_windows.go b/internal/platform/errno_windows.go
index 7cba50d4..e89b763e 100644
--- a/internal/platform/errno_windows.go
+++ b/internal/platform/errno_windows.go
@@ -16,6 +16,10 @@ const (
 	// instead of syscall.EEXIST
 	ERROR_FILE_EXISTS = syscall.Errno(0x50)
 
+	// ERROR_INVALID_NAME is a Windows error returned by open when a file
+	// path has a trailing slash
+	ERROR_INVALID_NAME = syscall.Errno(0x7B)
+
 	// ERROR_NEGATIVE_SEEK is a Windows error returned by os.Truncate
 	// instead of syscall.EINVAL
 	ERROR_NEGATIVE_SEEK = syscall.Errno(0x83)
@@ -57,7 +61,7 @@ func adjustErrno(err syscall.Errno) syscall.Errno {
 		return syscall.EBADF
 	case ERROR_ACCESS_DENIED, ERROR_PRIVILEGE_NOT_HELD:
 		return syscall.EPERM
-	case ERROR_NEGATIVE_SEEK:
+	case ERROR_NEGATIVE_SEEK, ERROR_INVALID_NAME:
 		return syscall.EINVAL
 	}
 	return err
diff --git a/internal/platform/open_file_windows.go b/internal/platform/open_file_windows.go
index 65484558..c17853d1 100644
--- a/internal/platform/open_file_windows.go
+++ b/internal/platform/open_file_windows.go
@@ -3,6 +3,7 @@ package platform
 import (
 	"io/fs"
 	"os"
+	"strings"
 	"syscall"
 	"unsafe"
 )
@@ -53,6 +54,11 @@ func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno)
 	}
 
 	switch errno {
+	case syscall.EINVAL:
+		// WASI expects ENOTDIR for a file path with a trailing slash.
+		if strings.HasSuffix(path, "/") {
+			errno = syscall.ENOTDIR
+		}
 	// To match expectations of WASI, e.g. TinyGo TestStatBadDir, return
 	// ENOENT, not ENOTDIR.
 	case syscall.ENOTDIR:


Test patch:
diff --git a/.github/wasi_testsuite_skip.json b/.github/wasi_testsuite_skip.json
index 2b403282..8595d331 100644
--- a/.github/wasi_testsuite_skip.json
+++ b/.github/wasi_testsuite_skip.json
@@ -1,5 +1,4 @@
 {
   "WASI Rust tests": {
-    "interesting_paths": "config is broken, and after fixing it forbids leading slash with EPERM"
   }
 }
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index 8e0963ab..56bf42c7 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -4007,6 +4007,14 @@ func Test_pathOpen_Errors(t *testing.T) {
 	err = os.Mkdir(joinPath(tmpDir, dir), 0o700)
 	require.NoError(t, err)
 
+	nested := "dir/nested"
+	err = os.Mkdir(joinPath(tmpDir, nested), 0o700)
+	require.NoError(t, err)
+
+	nestedFile := "dir/nested/file"
+	err = os.WriteFile(joinPath(tmpDir, nestedFile), []byte{}, 0o700)
+	require.NoError(t, err)
+
 	tests := []struct {
 		name, pathName                            string
 		fd, path, pathLen, oflags, resultOpenedFd uint32
@@ -4066,6 +4074,54 @@ func Test_pathOpen_Errors(t *testing.T) {
 			expectedLog: `
 ==> wasi_snapshot_preview1.path_open(fd=3,dirflags=,path=di,oflags=,fs_rights_base=,fs_rights_inheriting=,fdflags=)
 <== (opened_fd=,errno=ENOENT)
+`,
+		},
+		{
+			name:          "trailing slash on directory",
+			fd:            sys.FdPreopen,
+			pathName:      nested + "/",
+			path:          0,
+			pathLen:       uint32(len(nested)) + 1,
+			expectedErrno: ErrnoSuccess,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_open(fd=3,dirflags=,path=dir/nested/,oflags=,fs_rights_base=,fs_rights_inheriting=,fdflags=)
+<== (opened_fd=5,errno=ESUCCESS)
+`,
+		},
+		{
+			name:          "path under preopen",
+			fd:            sys.FdPreopen,
+			pathName:      "../" + file,
+			path:          0,
+			pathLen:       uint32(len(file)) + 3,
+			expectedErrno: ErrnoPerm,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_open(fd=3,dirflags=,path=../file,oflags=,fs_rights_base=,fs_rights_inheriting=,fdflags=)
+<== (opened_fd=,errno=EPERM)
+`,
+		},
+		{
+			name:          "rooted path",
+			fd:            sys.FdPreopen,
+			pathName:      "/" + file,
+			path:          0,
+			pathLen:       uint32(len(file)) + 1,
+			expectedErrno: ErrnoPerm,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_open(fd=3,dirflags=,path=/file,oflags=,fs_rights_base=,fs_rights_inheriting=,fdflags=)
+<== (opened_fd=,errno=EPERM)
+`,
+		},
+		{
+			name:          "trailing slash on file",
+			fd:            sys.FdPreopen,
+			pathName:      nestedFile + "/",
+			path:          0,
+			pathLen:       uint32(len(nestedFile)) + 1,
+			expectedErrno: ErrnoNotdir,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_open(fd=3,dirflags=,path=dir/nested/file/,oflags=,fs_rights_base=,fs_rights_inheriting=,fdflags=)
+<== (opened_fd=,errno=ENOTDIR)
 `,
 		},
 		{
diff --git a/internal/platform/open_file_test.go b/internal/platform/open_file_test.go
index fc2a1c09..553dbccb 100644
--- a/internal/platform/open_file_test.go
+++ b/internal/platform/open_file_test.go
@@ -13,6 +13,16 @@ import (
 func TestOpenFile(t *testing.T) {
 	tmpDir := t.TempDir()
 
+	t.Run("directory with a trailing slash", func(t *testing.T) {
+		path := path.Join(tmpDir, "dir", "nested")
+		err := os.MkdirAll(path, 0o700)
+		require.NoError(t, err)
+
+		f, errno := OpenFile(path+"/", os.O_RDONLY, 0)
+		require.Zero(t, errno)
+		require.NoError(t, f.Close())
+	})
+
 	// from os.TestDirFSPathsValid
 	if runtime.GOOS != "windows" {
 		t.Run("strange name", func(t *testing.T) {
@@ -26,6 +36,19 @@ func TestOpenFile(t *testing.T) {
 func TestOpenFile_Errors(t *testing.T) {
 	tmpDir := t.TempDir()
 
+	t.Run("file with a trailing slash is ENOTDIR", func(t *testing.T) {
+		nested := path.Join(tmpDir, "dir", "nested")
+		err := os.MkdirAll(nested, 0o700)
+		require.NoError(t, err)
+
+		nestedFile := path.Join(nested, "file")
+		err = os.WriteFile(nestedFile, nil, 0o700)
+		require.NoError(t, err)
+
+		_, errno := OpenFile(nestedFile+"/", os.O_RDONLY, 0)
+		require.EqualErrno(t, syscall.ENOTDIR, errno)
+	})
+
 	t.Run("not found must be ENOENT", func(t *testing.T) {
 		_, errno := OpenFile(path.Join(tmpDir, "not-really-exist.txt"), os.O_RDONLY, 0o600)
 		require.EqualErrno(t, syscall.ENOENT, errno)