Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial support for securing tty I/O. #684
Changes from all commits
bddb692
710b8fb
84d122c
612fdea
d459401
c31a9a3
7ba4968
2d8bbbd
badda16
25234d3
ee476c8
f8047c0
ae2a60f
cc3cf24
26be703
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.