Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for securing tty I/O. #684

Merged
merged 15 commits into from
Jan 2, 2020
Merged

Initial support for securing tty I/O. #684

merged 15 commits into from
Jan 2, 2020

Conversation

sunfishcode
Copy link
Member

This implements an initial implementation of a secure TTY writer, following the first step of the plan discussed here:

WebAssembly/WASI#162

This neutralizes all escape sequences when the output is a terminal, defusing the security issues such as those described here, including reading back output from previous commands, or potentially even tricking users into executing arbitrary commands.

Introducing support for escape sequences, allowing colors and cursor positioning, is the second step of the plan, which I expect to implement in a separate PR.

@sunfishcode
Copy link
Member Author

Thanks for the review! I've now addressed all the comments.

Descriptor::Stdin => return Err(Error::EBADF),
Descriptor::Stdout => {
// lock for the duration of the scope
let stdout = io::stdout();
let mut stdout = stdout.lock();
let nwritten = stdout.write_vectored(&iovs)?;
let nwritten = if isatty {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't only sanitize TTY output resp. handle terminal output in a different way than other file based output, because malicious control sequences e.g. in log files will do exactly the same harm to the host system, whenever they are viewed by a cat command etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the PR here as fixing what we can given the current WASI API. A more complete fix will require changing the WASI spec, which I'm also looking into.

@mash-graz
Copy link

mash-graz commented Dec 16, 2019 via email

@sunfishcode
Copy link
Member Author

The Emscripten failure looks like it's hitting a compiler bug: rust-lang/rust#66308.

@kubkon
Copy link
Member

kubkon commented Dec 20, 2019

The Emscripten failure looks like it's hitting a compiler bug: rust-lang/rust#66308.

I'm happy to either disable the Emscripten job or use nightly to run the check (hopefully 66308 lands there soonish, or is there already?).

EDIT: Oh, oops, I mixed rust-lang/rust#66308 with rust-lang/rust#67363. If the latter doesn't fix Emscripten, I think we should disable the job until rust-lang/rust#66308 is resolved.

In terms of failing WASI tests, #743 should fix it.

@kubkon
Copy link
Member

kubkon commented Dec 20, 2019

@sunfishcode OK I've now submitted #744 which disables the Emscripten job until rust-lang/rust#66308 is resolved.

@kubkon kubkon merged commit 1d810a5 into master Jan 2, 2020
@kubkon kubkon deleted the escapes branch January 2, 2020 10:53
x if x.is_control() => '�',
x => x,
}
.encode_utf8(&mut [0; 4]) // UTF-8 encoding of a `char` is at most 4 bytes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI Limit is only 4 if character code follows Unicode std limit 0x10ffff. But ISO std defines limit as 0x7ffffff. Which would require 6 bytes. So depends which limit the implementation ensures. But for simple pass-through encoding 6 bytes would be the safest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust documents that 4 is enough. If 5-byte or 6-byte UTF-8 encodings should ever be revived, we can expect Rust to add new functions rather than change such an invariant in existing functions.

Descriptor::Stdin => return Err(Error::EBADF),
Descriptor::Stdout => {
// lock for the duration of the scope
let stdout = io::stdout();
let mut stdout = stdout.lock();
let nwritten = stdout.write_vectored(&iovs)?;
let nwritten = if isatty {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be an option to wasmtime (security strict mode vs lesser safe mode). Having the asymmetry behavior between stdout/stderr seems weird. stderr could still be used for binary data.

I do see it could be common to pipe raw/binary data into a file from wasmtime - but maybe it could be ok to require the wasmtime caller to explicitly enable this with a command line option.

I just fell security should take precedence over convenience. I guess wasmtime could just fail with nice error message when it sees escape codes to stdout/stderr (maybe also tty) in the output so the caller at least gets an explanation of why it failed.

Also, we do NOT know what the calling process of wasmtime does with the stdout/stderr handed to wasmtime. Could end up forwarding the raw data to its terminal stdout/stderr which is hidden to wasmtime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that there's room for improvement in these areas. I'm working on a more comprehensive design, including changes to WASI itself, which I hope will address these concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants