Sanitize file/directory symlinks in path_symlinks on Windows

[marmistrz]

Based from my experience, symlink_file will not return an error if we try to create a file symlink to a directory. I used the following Rust program to verify it:

use std::env;
use std::io::Result;
use std::os::windows::fs::symlink_file;

pub(crate) fn path_symlink(src: &str, dst: &str) -> Result<()> {
    // try creating a file symlink
    symlink_file(src, dst).map_err(|e| {
        println!("ERROR: {}", e);
        e
    })
}

fn main() {
    let args: Vec<_> = env::args().collect();
    path_symlink(&args[1], &args[2]).expect("path_symlink failed");
}

If dir is an existing directory, and the binary as executed with cargo run dir target, the program will succeed and create a file symlink.

For details on file vs. directory symlinks on Windows see #1358.

@kubkon: I'm also unsure about the correctness of the following error mappings:
https://github.com/marmistrz/wasmtime/blob/1c61210025c00f0d397be86d752717ab20642f53/crates/wasi-common/src/sys/windows/hostcalls_impl/fs.rs#L527-L536
In which cases are they expected to be encountered? What's the scenario they're there for?

[marmistrz]

Do you have any idea if we can add any regression test here? We'd probably need to invoke some low-level Windows API, I think.

[marmistrz]

@sunfishcode @kubkon ping, can you please review the change?

[kubkon]

Hmm, I'm somewhat confused here. Why do we need this check? Or put it another way, if we're doing this check already here, is creating a dir symlink on error ERROR_NOT_A_REPARSE_POINT still needed? Is the current approach of try and fail and try something else not working?

Some context on why we've originally had symlink_dir call after a failing symlink_file call. As @sunfishcode explained a long time ago, from atomicity point of view, it seems virtually always better to try and fail and then correct, rather than invoke another syscall (in this case via fs::metadata) which would check what the resource (in this case the symlink) is since in between the check and actual call to symlink_file etc. the path might change, or whatnot.

[marmistrz]

I was unable to encounter ERROR_NOT_A_REPARSE_POINT while using symlink_file with the symlink target being a directory. As far as I have tested, the current approach of try and fail doesn't work and a file symlink will always be created.

I agree that try and fail would be preferable, but it appears that CreateSymbolicLinkA doesn't check if the symlink target is a file or directory. I don't know if ERROR_NOT_A_REPARSE_POINT may be returned by CreateSymbolicLinkA at all.

[kubkon]

Hmm, sounds like a bug then. I thought that ERROR_NOT_A_REPARSE_POINT played some role in this, but alas I cannot remember the exact test conditions now. Anyhow, I thought it was covered by our test programs, but from what I understand, you've proved that they were not properly testing this behaviour on Windows? Would the test work fine if we got rid ERROR_NOT_A_REPARSE_POINT match altogether? And would they still work without both the match and your upfront check?

[kubkon]

Oh, and here's another thought. What would happen if we reversed the order of ops, i.e., started with symlink_dir, and if failed, tried symlink_file. Would that make it possible to apply the "try-catch" approach rather than an upfront check?

[marmistrz]

Trying to create a symlink to a file using symlink_dir succeeds too.

[kubkon]

Interesting, but then the symlinks are obviously invalid if the user would try to act on them in say Windows Explorer or whatnot?

[marmistrz]

Yes, Windows Explorer will show an error, claiming that the directory is invalid.

[sunfishcode]

The patch looks reasonable within the current WASI API.

What should we do in WASI itself here? One option I'm thinking about is to replace WASI's symlink function with symlink_file and symlink_directory, and moving the logic for deciding which one to use into WASI libc. That way, POSIX applications would get the same behavior, but it would allow other users to avoid "automatically pick which one or guess" if they wanted to. Thoughts?

[kubkon]

The patch looks reasonable within the current WASI API.

What should we do in WASI itself here? One option I'm thinking about is to replace WASI's symlink function with symlink_file and symlink_directory, and moving the logic for deciding which one to use into WASI libc. That way, POSIX applications would get the same behavior, but it would allow other users to avoid "automatically pick which one or guess" if they wanted to. Thoughts?

FWIW, I think that this is the best way to go 👍

[github-actions]

Subscribe to Label Action

This issue or pull request has been labeled: "wasi"

Users Subscribed to "wasi"

• @kubkon

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[marmistrz]

I'm not quite sure if it's a good idea.

Such API change will make it non-trivial to compile POSIX programs to WASM and run them on Windows. Suppose with have a program written for Linux using symlink(2). Should such call be translated to symlink_dir or symlink_file? I think that a WASM counterpart of this shell script should continue to work under WASM on Windows:

touch foo
mkdir bar
ln -s bar baz # directory symlink should be inferred
ln -s foo fuz # file symlink should be inferred

Instead, we could allow an optional hint argument to path_symlink, for instance a: 0x0 - no hint, 0x1 - file, 0x2 - directory.
It's not clear what we should do if a dangling symlink without a hint is requested. We could either (a) fall back to file symlinks (b) check if the target exists and return EINVAL if it doesn't.

[kubkon]

I'm not quite sure if it's a good idea.

Such API change will make it non-trivial to compile POSIX programs to WASM and run them on Windows. Suppose with have a program written for Linux using symlink(2). Should such call be translated to symlink_dir or symlink_file? I think that a WASM counterpart of this shell script should continue to work under WASM on Windows:

touch foo
mkdir bar
ln -s bar baz # directory symlink should be inferred
ln -s foo fuz # file symlink should be inferred

Hmm, I don't think that's what @sunfishcode meant. While WASI embedder would have to provide two syscalls namely symlink_file and symlink_dir, WASI libc would offer symlink(2) which would at the libc level work out which syscall to use.

[marmistrz]

Hmm, I don't think that's what @sunfishcode meant. While WASI embedder would have to provide two syscalls namely symlink_file and symlink_dir, WASI libc would offer symlink(2) which would at the libc level work out which syscall to use.

Isn't wasi-libc eventually compiled to WebAssembly and doesn't it eventually become a part of the resulting wasm binary? With this solution we'd need different syscall selection logic depending on the OS used by the host executing the wasm binary.

[kubkon]

Hmm, I don't think that's what @sunfishcode meant. While WASI embedder would have to provide two syscalls namely symlink_file and symlink_dir, WASI libc would offer symlink(2) which would at the libc level work out which syscall to use.

Isn't wasi-libc eventually compiled to WebAssembly and doesn't it eventually become a part of the resulting wasm binary? With this solution we'd need different syscall selection logic depending on the OS used by the host executing the wasm binary.

It is, but why would you need different logic at the WASI libc level? I assumed that the libc symlink(2) would first try delegating to symlink_file syscall implemented by the embedder, and if that fails, try symlink_dir, and if that fails, error out. Then, the host-dependent logic would be entirely encapsulated within the syscalls themselves. libc would then only rely on the return values to work out whether the call succeeded or not.

[marmistrz]

It is, but why would you need different logic at the WASI libc level? I assumed that the libc symlink(2) would first try delegating to symlink_file syscall implemented by the embedder, and if that fails, try symlink_dir, and if that fails, error out.

Why should symlink_file fail if the target is a directory? I have never managed to reproduce such behavior on Windows. I assume that the libc implementation will need to check the file type on Windows, as implemented in this patch.

[kubkon]

It is, but why would you need different logic at the WASI libc level? I assumed that the libc symlink(2) would first try delegating to symlink_file syscall implemented by the embedder, and if that fails, try symlink_dir, and if that fails, error out.

Why should symlink_file fail if the target is a directory? I have never managed to reproduce such behavior on Windows. I assume that the libc implementation will need to check the file type on Windows, as implemented in this patch.

Oh sorry, what I meant is that both symlink_file and symlink_dir would perform whatever checks are required to determine if creating this type of symlink on the host is valid or not for the given entities. In essence, the logic of path_symlink in this PR would be preserved but split into two separate calls. This way, libc's symlink would simpy check the return values to work out if it succeeded or not, and would be host-independent.

[marmistrz]

Hmmm, but then, shouldn't we require the implementation of symlink_file on Linux to check if we actually have a file, so that the use of symlink_file is what intuition suggests?

[marmistrz]

My suggestion is that we:

• remove the possibly unnecessary checks (as done in 0d3a173), since we don't see any cases which they account for
• merge the change as-is and create a separate issue in WebAssembly/WASI to discuss possible API changes (unless there are some changes in the code to be addressed)

[kubkon]

My suggestion is that we:

* remove the possibly unnecessary checks (as done in [0d3a173](https://github.com/bytecodealliance/wasmtime/commit/0d3a17331f8b98c026805003a4973ccbf3b89696)), since we don't see any cases which they account for

* merge the change as-is and create a separate issue in WebAssembly/WASI to discuss possible API changes (unless there are some changes in the code to be addressed)

Sounds good!