fd_fdstat_set_flags can silently change a fd's number

[neild]

Describe the bug *osFile.reopen doesn't set f.fd, leaving the file with potentially the wrong file descriptor.

To Reproduce

package main

import (
        "io"
        "log"
        "os"
        "syscall"
)

func main() {
        f1, err := os.Open("/file")
        if err != nil {
                log.Fatal(err)
        }
        f2, err := os.Open("/file")
        if err != nil {
                log.Fatal(err)
        }
        f1.Close()
        syscall.SetNonblock(int(f2.Fd()), true)
        if _, err := io.ReadAll(f2); err != nil {
                log.Fatal(err)
        }
}

$ echo foo > file
$ wazero run -mount=.:/ main.wasm
2024/09/20 23:50:44 read /file: Bad file number

This program should read from "file", but the read fails because f2's FD has changed. (The "syscall.SetNonblock" is necessary to trigger the correct write path here.)

I initially thought this might be exploitable to write to files in read-only mapped directories, but that doesn't seem to be the case. You can write to an unexpected writeable file, though.

Environment (please complete the relevant information):

• Go version: master
• wazero Version: master
• Host architecture: darwin_arm64

[ncruces]

Thanks for the reproducer.

From the title you seem to have more info than you shared, though? If you suspect fd_fdstat_set_flags, could you get us a code pointer?

[ncruces]

It seems the issue is that we reopen the file here? I don't know that that's why. Do you have more info? https://github.com/tetratelabs/wazero/blob/1353ca24fef0a57a3a342d75f20357a6e9d3be35/internal/sysfs/osfile.go#L79-L92

Do we know which OSes we intend to support? fcntl(fd, F_SETFL, ...) might fix this: https://man7.org/linux/man-pages/man2/fcntl.2.html

[neild]

From the title you seem to have more info than you shared, though? If you suspect fd_fdstat_set_flags, could you get us a code pointer?

Sorry, I didn't describe the relationship between the high-level operations in the reproduction case and the WASI operations clearly.

I believe the syscall.SetNonblock translates to a fd_fdstat_set_flags call, which then (inside wazero) reopens the underlying file, and leaves the *osFile with a mismatched file and fd. The problem is here:

https://github.com/tetratelabs/wazero/blob/1353ca24fef0a57a3a342d75f20357a6e9d3be35/internal/sysfs/osfile.go#L119-L122

Note that f.file is updated, but f.fd is not.

I'm also suspicious of the reopen in general: Opening a file normally gives you a FD that tracks the file across renames/deletes, but the reopen here looks like it will reopen the original path of the file, possibly changing which file the FD refers to.

[ncruces]

Yes, I think reopening should be avoided if possible, but may be necessary if we want to support certain OSes (just Windows?).

[ncruces]

The immediate one-line fix is 2dfadcb, but there's more we could (should?) do.

Reopening the file is less than ideal, because it may have moved (on POSIX, and Windows, because we do use FILE_SHARE_DELETE). We could detect this and error without either updating the file.

On POSIX we can also specialise updating flags to use fcntl(fd, F_SETFL, ...).

What do you guys think? @evacchi @achille-roussel

[evacchi]

so this was indeed introduced because of Windows support, and if I recall correctly, it's because Seek(0) on a directory won't work on Windows, so the only way to reset the cursor in a directory is to close and reopen.

related, but this is not the sole change: https://github.com/tetratelabs/wazero/blob/6bb9899ea4c745a4ae23179a34fac0540c2ce551/internal/sysfs/osfile.go#L31-L35

probably it would be best to separate directories from regular files and handle them separately (at least on Windows) and/or special-case only this situation.

Again, if my memory serves me well :)

[ncruces]

Yeah, so reopen seems to be a fallback that's only needed on non-POSIX (for both setting the append flag and rewinding directories). Both things can be done without reopening on Unix.

[ncruces]

Well, that was a rabbit hole. But I think this should improve things. Feel free to test @neild.

[neild]

That PR fixes my test cases; thanks!

FWIW, wazero differs from wasmtime in that path_open on a directory seems to return a reference to the filename of the directory, not the fd. That is, wasmtime's path_open behaves like openat: You can open a directory, rename or unlink the directory, and then continue to operate on the original directory in its new location using the existing fd. wazero, in contrast, seems to use the original path the directory was opened under.

• path_open(rootfd, "d") => dfd
• rename d to d2
• path_open(dfd, "file")
  • wazero: opens d/file
  • wasmtime: opens d2/file

I don't know if that's intentional; path_open looks a lot like openat, but I can't find any specification on what the expected behavior here is and I haven't tested any other implementations.

[ncruces]

Do you have a reproducer? I think that might depend on how you mount the file system.

[neild]

It's not a small reproducer, but os/TestRootRenameAfterOpen in https://go.dev/cl/612136. (There's a t.Skip for that test when GOWASIRUNTIME=wazero which you might need to remove, depending on how you run the test.)

[ncruces]

I guess you're right. There was #2254 and follow ups to work on that. But they stalled.