Put dangerous use cases behind feature flags

[insanitybit]

Hi, just stumbled across this crate. Very cool and flexible, not judging what you've done here.

I do think though that it's a bit scary though.

For example:

“(…)” runs a command with a pipe from the child process’ stdout, on platforms whch support it.

https://docs.rs/nameless/0.7.1/nameless/struct.InputByteStream.html

An attacker who controls a command line argument to a program now has full RCE.

Similarly, with the networking support, the attacker can perform network requests, which is less scary but still pretty spooky.

I would suggest two things.

1. Put support for different sources and sinks into feature flags and disable the features like command execution by default. I think that users generally don't expect "command line argument can execute arbitrary code" to be the case.
2. Add a security warning in the repo clarifying that this library is not suitable for cases where untrusted input is provided to the command line.

If I've misunderstood anything about how this crate works feel free to let me know. I just took a quick look and noticed this.

[sunfishcode]

Thanks for the report! I'd like to be specific about attack vectors here.

Something like "user copies some text from a web page and pastes it into their terminal" seems to already be an indefensible situation. Untrusted input interpreted by the user's shell can execute arbitrary commands via |, ;, or several other common shell features. And it can employ escape sequences to hide everything it's doing, so the user wouldn't even know anything was happening.

There are other possibilities of course; I'd be curious about what specific situations you have in mind.

[sunfishcode]

@insanitybit Would you be able to comment on specific scenarios where allowing command execution or network connections would be a problem?

[insanitybit]

The problem to me is that it's not expected for a command line argument to potentially execute arbitrary code. So one could imagine this program

use std::process::Command;
    
fn main() {
    let mut arg = String::new();
    io::stdin().read_line(&mut arg)?;
    
    let output = Command::new("some_program")
                         .arg(arg)
                         .output()
                         .expect("Failed to execute command");
}

If some_program is using nameless and an attacker controls arg this is code execution. People execute subprocesses fairly often and it isn't expected that it would lead to arbitrary code execution if an attacker controlled any single parameter.

[insanitybit]

Maybe as a further example, let's imagine that ripgrep used nameless. Since vscode shells out to it an attacker would only need to control a file name argument that gets passed along. So "clone git repo, open in vscode" would mean code execution.

There are probably much more serious circumstances, like if someone takes a web request and passes some part of it to a command line program - again, this isn't something that would generally be thought of as extremely dangerous (as long as you don't pass through a shell), but would mean remote code execution if nameless were used.

[sunfishcode]

Thanks! The direct Command invocation is a good point.

It's my understanding that "open in vscode" will automatically run the project's build.rs file, and may also run proc macros, which already means code execution. And I would generally not recommend taking an arbitrary web request and passing it verbatim to a command-line program. That sounds risky if the request could name things like ~/.ssh/id_rsa or other things.

But bigger picture, you're right. The ideal of nameless is to factor out the opening of files/URLs/etc. out as far as possible, while the nameless crate in its current form only achieves factoring them out to the point of the command-line argument parsing logic around the main call. It isn't ideal, and there are other projects which will hopefully do this in a more comprehensive way, but here, for now, the goal is to see if there's something in this space which makes sense.

One option I'm now considering here is to add something like "--enable-execution" and/or "--enable-network", which might look like this:

$ cat https://example.com
cat: Run `cat --enable-network` to enable network URLs
$ cat --enable-network https://example.com
...

And maybe in --enable-network mode, it doesn't accept bare filesystem paths at all; everything must be explicit URLs or other special syntax. Also, maybe nameless should try to open strings as files first, and only fall back to URL/command parsing and resolution/execution if opening it as a file fails.

That still may not be completely watertight, but this is the general direction I'm inclined to think in‐making it have all its main features by default, even if they need more syntax, rather than hiding them behind cargo features.

[insanitybit]

Yeah, my examples are sort of contrived. A more brutal example of this sort of thing would be log4j, where an attacker controlled a string and, unexpectedly, that string was treated as code by a logging framework. I think that treating args as code is definitely unexpected, albeit quite cool and powerful.

Adding an explicit argument could help. It's up to you what the best ergonomics for your library are - I just think that a build arg is a lot simpler since users who don't want the feature won't have it and those who will will understand the consequences.

I think the "file first, then url" would probably be more confusing and unexpected for users, but again, the ergonomic choices are your call.

Even if you move things behind features, you could have those features enabled by default.

[sunfishcode]

One of my underlying observations here is that many CLI programs don't know the security and ergonomics landscape of end users. For example, a grep program may process public data or private data; it may run on a secure machine or an unsecure one. CLI tools aren't told the difference. And consequently, it's awkward to ask developers of CLI tools to decide for their users what kind of security vs. ergonomics tradeoffs make sense. So I'm hoping to find a reasonable default that doesn't require build flags to enable all the useful functionality.

Another part of the bigger picture is also that instead of passing strings around, what we really want is to also extend nameless with a way to pass file handles in an ergonomic way. In theory, this should be transparent to the main application code.