Are there security issues with Command::new("cmd").arg("/C").args(user_input)?

help
1

I'm building a program that needs access to the elm compiler, which may be installed via npm. In such cases, it seems the only way to execute it in Windows is via Command::new("cmd").arg("/C").arg("elm"). The thing is, users of my tests runner can pass as argument the path to their executable if needed. Concretely, this is the current code, which works on linux/mac and windows for actual executables (not cmd shells):

    let executable = which::CanonicalPath::new(compiler)?;
    Command::new(executable)
        .arg("make")
        .current_dir(current_dir)
        .status()

where compiler comes from a CLI --compiler argument so may contain anything that the user gives. I'm no security expert but usually, we should never trust user inputs for things that get passed to an evaluation mechanism. So if I add a special case for Windows doing something like

Command::new("cmd")
    .arg("/C")
    .arg(compiler)
    .args(some_args)
    .status()

when the executable is not found, am I exposed to security threats? Is there a documentation somewhere on how to handle that safely? Or am I being over-cautious for nothing?

2

The status function returns a Result<ExitStatus>, which you don't appear to be checking in your example code, so this could allow for undefined behavior. What you could use is its success function in order to verify that the command exited successfully. There's also other associated functions implemented for ExitStatus that may or may not be better suited to your exact purpose. The examples in the docs should be helpful enough, but here's an example using your code:

    let status = Command::new(executable)
        .arg("make")
        .current_dir(current_dir)
        .status()
        .expect("failed to execute make");

    assert!(status.success(), "make did not succeed");
3

@camden-smallwood Thanks I do check it but I just didn't want to put too many things which seemed unrelated to my concern. Here is the exact code I'm currently using (not handling the special Windows case) https://github.com/mpizenberg/elm-test-rs/blob/bb73ed5e6a9533fc54c51ef71552058a65c1c9a2/src/make.rs#L291

4

Can you clarify what you mean here? Sure, you should check for errors, but that shouldn't otherwise be undefined behavior (ref 1, 2) in the language.

2 Likes
5

Even if found, nothing says the compiler is truly a compiler at all! But if you're running in the same context as the user, that's not really a problem, because they could already run that potentially-evil command themselves anyway. Trusting user input is more of an issue when you cross some privilege context, like in an elevated process or over a network.

2 Likes
6

I'm confused as to why that's the only way?

7

That's because when you install things with npm, it creates some thing.cmd that you cannot run via Command::new("thing"). As to why windows and npm work this way, I've no idea ^^.

8

Sorry, that wasn't very clear. It's definitely not a language problem, it's an implementation problem. Without checking the result, the program will continue to execute, presumably expecting the command to succeed, which may rely on artifacts that wouldn't exist in the case that the command failed to execute.

1 Like
9

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.