wasmtime / Issue #1359 Sanitize file/directory symlinks i... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / Issue #1359 Sanitize file/directory symlinks i...

Wasmtime GitHub notifications bot (Mar 19 2020 at 08:33):

Do you have any idea if we can add any regression test here?

Wasmtime GitHub notifications bot (Mar 19 2020 at 08:33):

marmistrz edited a comment on Issue #1359:

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.

Wasmtime GitHub notifications bot (Mar 24 2020 at 12:54):

marmistrz commented on Issue #1359:

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

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:13):

sunfishcode commented on Issue #1359:

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?

Wasmtime GitHub notifications bot (Mar 25 2020 at 13:03):

kubkon commented on Issue #1359:

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 :+1:

Wasmtime GitHub notifications bot (Mar 25 2020 at 13:26):

github-actions[bot] commented on Issue #1359:

Subscribe to Label Action

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

<details> <summary>Users Subscribed to "wasi"</summary>

@kubkon

</details>

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

Learn more.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:05):

marmistrz commented on Issue #1359:

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.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:13):

kubkon commented on Issue #1359:

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.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:38):

marmistrz commented on Issue #1359:

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.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:42):

kubkon commented on Issue #1359:

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.

Wasmtime GitHub notifications bot (Mar 25 2020 at 15:34):

marmistrz commented on Issue #1359:

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.

Wasmtime GitHub notifications bot (Mar 25 2020 at 16:12):

kubkon commented on Issue #1359:

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.

Wasmtime GitHub notifications bot (Mar 25 2020 at 18:47):

marmistrz commented on Issue #1359:

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?

Wasmtime GitHub notifications bot (Mar 29 2020 at 08:24):

marmistrz commented on Issue #1359:

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)

Wasmtime GitHub notifications bot (Apr 01 2020 at 06:55):

kubkon commented on Issue #1359:

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!

Last updated: Nov 22 2024 at 16:03 UTC