sysfs: clean paths lexically before use

[ncruces]

This is intended to close #2321.

W.r.t. security, after consulting the other maintainers, we must say that fully sandboxing our WASI implementation is beyond the current scope of the project. This will be made explicit in the documentation.

We consider Wasm a mature specification, and security issues against our implementation of it will be taken seriously. WASI is not as mature, nor is our implementation of WASI preview 1.

We also provide advanced enough APIs that you can use to implement your own, better locked down, alternative to our WASI implementation.

That said, this PR still sides with breaking POSIX path resolution semantics in WASI vs. allowing trivial root mount escapes, by using path.Clean to lexically clean all paths before use.

This is a judgement call.

Handling paths lexically is easier to reason about, easier to port, and idiomatic Go. Go provides path.Clean and filepath.Clean; fs.FS is even stricter disallowing .. entirely. It's also unlikely to break well, behaving, useful applications.

[anuraaga]

Sorry for coming to this so late, was just randomly thinking about this for some reason even before seeing this new PR.

I think to correctly handle symlinks, we would need to recognize them when opening/statting and use the resolved name recursively through the WASI apis rather than directly opening, likely with a recursion limit if not real cycle detector though that's likely too much.

I think cleaning paths still doesn't prevent the case where a symlink outside the sandbox is present in the mounted files. Naturally this is a far smaller issue than being to create the symlink in wasm itself.

I understand that the WASI implementation won't be fully hardened which makes sense, so the above might be going too far. But just wanted to share in case it didn't seem too hard, but probably is.

[anuraaga]

Sorry I noticed the comment is about 3) in the issue, and is probably too much so no worries

[ncruces]

No worries. Yeah, that sounds like doing path resolution ourselves, maybe with some shortcut. But I honestly don't think there's any shortcut possible.

ISTM that to do path resolution, you really need to start at the root (there's no CWD in WASI preview 1), then for each path element do readlinkat to possibly resolve a symlink, then openat(NO_FOLLOW), then fstat to see if it's a directory, and on to the next element. If you don't have NO_FOLLOW, you have a race. I have no idea about Windows.

That's a lot of syscalls, and a lot of stuff to get wrong, especially if you're doing this for sandboxing/security sake.

[anuraaga]

ISTM that to do path resolution, you really need to start at the root (there's no CWD in WASI preview 1)

I might not understand the syscalls right, but I didn't really expect CWD to be needed. I thought it could be something like this pseudocode similar to checking whether opening a file matches isDir

https://github.com/tetratelabs/wazero/blob/main/imports/wasi_snapshot_preview1/fs.go#L1619

f, ok := fsc.LookupFile(newFD)
if symlinkTarget := f.File.SymlinkTarget(); symlinkTarget != nil { // Calls `os.Lstat` or `os.Readlink`
  _ = fsc.CloseFile(newFD)
  return doPathOpen(symlinkTarget, etc...) // The contents of pathOpen after pathName is computed from wasm arg
}

The biggest problem is that it seems like a fairly large change to have to add symlink lstat to the File interface, and anyways indeed could be missing something here so something simpler seems fine, just wanted to make sure the idea got across.

[ncruces]

CWD is not needed because WASI preview 1 has no concept of CWD.

By the time you get to those lines the file is already opened, and path resolution already happened. The file is opened here:

wazero/imports/wasi_snapshot_preview1/fs.go

Line 1610 in 111c51a

newFD, errno := fsc.OpenFile(preopen, pathName, fileOpenFlags, 0o600)

The lines you're quoting are really only checking if you opened what you wanted to open (a directory) after the fact. That's good because “ask for forgiveness instead of permission” avoids a race.

But this doesn't work for symlinks for two reasons:

1. You don't open a symlink; you stat it with lstat, or read it with readlink, but you can't open and then fstat it, because by then you opened the target; on some platforms, you can say you don't want to open the target, with O_NOFOLLOW, on others not even that.
2. Symlinks in the last path element are the least of your worries; what you should really be worrying about is directory symlinks in the middle; and those can be symlinks that have been moved around to point to different things than when they were created.