wasmtime / PR #1359 Sanitize file/directory symlinks in p... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1359 Sanitize file/directory symlinks in p...

Wasmtime GitHub notifications bot (Mar 18 2020 at 19:55):

marmistrz requested peterhuene, kubkon, and sunfishcode for a review on PR #1359.

Wasmtime GitHub notifications bot (Mar 18 2020 at 19:55):

marmistrz requested peterhuene, kubkon, and sunfishcode for a review on PR #1359.

Wasmtime GitHub notifications bot (Mar 18 2020 at 19:55):

marmistrz requested peterhuene, kubkon, and sunfishcode for a review on PR #1359.

Wasmtime GitHub notifications bot (Mar 18 2020 at 19:55):

marmistrz opened PR #1359 from dir_symlink to master:

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?

Wasmtime GitHub notifications bot (Mar 18 2020 at 22:49):

peterhuene submitted PR Review.

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

marmistrz updated PR #1359 from dir_symlink to master:

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?

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

kubkon created PR Review Comment:

Is this change only targeting the old snapshot?

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

kubkon submitted PR Review.

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

marmistrz updated PR #1359 from dir_symlink to master:

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?

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

marmistrz submitted PR Review.

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

marmistrz created PR Review Comment:

Whoops, git rebase went wrong. It should be fine now.

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

marmistrz edited PR Review Comment.

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

marmistrz updated PR #1359 from dir_symlink to master:

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?

Wasmtime GitHub notifications bot (Mar 24 2020 at 14:00):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 14:00):

kubkon created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Mar 24 2020 at 16:33):

marmistrz created PR Review Comment:

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.

Wasmtime GitHub notifications bot (Mar 24 2020 at 16:33):

marmistrz submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:20):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:20):

kubkon created PR Review Comment:

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?

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

kubkon created PR Review Comment:

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?

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

kubkon submitted PR Review.

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

marmistrz created PR Review Comment:

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

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

marmistrz submitted PR Review.

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

kubkon created PR Review Comment:

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

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

kubkon submitted PR Review.

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

marmistrz submitted PR Review.

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

marmistrz created PR Review Comment:

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

Wasmtime GitHub notifications bot (Mar 29 2020 at 07:32):

marmistrz updated PR #1359 from dir_symlink to master:

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?

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

kubkon merged PR #1359.

Last updated: Nov 22 2024 at 16:03 UTC