Disallow more characters in arguments for internal cmd commands

[IanManske]

Description

Makes run-external error if arguments to cmd.exe internal commands contain newlines or a percent sign. This is because the percent sign can expand environment variables, potentially? allowing command injection. Newlines I think will truncate the rest of the arguments and should probably be disallowed to be safe.

After Submitting

• If the user calls cmd.exe directly, then this bypasses our handling/checking for internal cmd commands. Instead, we use the handling from the Rust std lib which, in this case, does not do special handling and is potentially unsafe. Then again, it could be the user's specific intention to run cmd with whatever trusted input. The problem is that since we use the std lib handling, it assumes the exe uses the C runtime escaping rules and will perform some unwanted escaping. E.g., it will add backslashes to the quotes in cmd echo /c '""'.
• If cmd is called indirectly via a .bat or .cmd file, then we use the Rust std lib which has separate handling for bat files that should be safe, but will reject some inputs.
• I'm not sure how we handle PATHEXT, that can also cause a file without an extension to be run as a bat file. If so, I don't know where the handling, if any, is done for that. It looks like we use the which crate to do the lookup using PATHEXT. Then, we pass the exe path from that to the Rust std lib Command, which should be safe (except for the first cmd.exe note).

So, in the future we need to unify and/or fix these different implementations, including our own special handling for internal cmd commands that this PR tries to fix.

[fdncred]

+1 for working on this so fast. Thanks! I did some preliminary testsing with this PR using assoc, ftype, and mklink using double quotes and everything worked as expected. I vote to land and get on with our lives.

[IanManske]

Thanks for testing it @fdncred! I'll land this for now and we can add more fixes on top of this if necessary.

[YizhePKU]

// \r and \n trunacte the rest of the arguments and % can expand environment variables

trunacte is a typo. How come the CI didn't catch that?

Otherwise this looks good. I'm not sure if newlines actually truncate arguments either, but better be safe than sorry.

[ChrisDenton]

I'm not sure if newlines actually truncate arguments

They do. You can test this by using CreateProcess directly. The following program just prints "hello":

Test program

use std::mem;
use std::ptr::null_mut as null;
use windows_sys::Win32::System::Threading::CreateProcessA;
use windows_sys::Win32::System::Threading::PROCESS_INFORMATION;
use windows_sys::Win32::System::Threading::STARTUPINFOA;

fn main() {
    unsafe {
        let process = String::from("C:\\Windows\\System32\\cmd.exe\0");
        let mut cmd = String::from("cmd /c echo hello\nworld\0");
        let mut info: PROCESS_INFORMATION = mem::zeroed();
        let mut startup: STARTUPINFOA = mem::zeroed();
        startup.cb = mem::size_of::<STARTUPINFOA>() as _;
        CreateProcessA(
            process.as_ptr(),
            cmd.as_mut_ptr(),
            null(),
            null(),
            0,
            0,
            null(),
            null(),
            &startup,
            &mut info,
        );
    }
}