Disallow more characters in arguments for internal cmd commands (#1…

…3009)

# 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.

crates/nu-command/src/system/run_external.rs

@@ -565,12 +565,20 @@ fn has_cmd_special_character(s: &str) -> bool {
  SPECIAL_CHARS.iter().any(|c| s.contains(*c))
 }
 
-/// Escape an argument for CMD internal commands. The result can be safely
-/// passed to `raw_arg()`.
+/// Escape an argument for CMD internal commands. The result can be safely passed to `raw_arg()`.
 #[cfg(windows)]
 fn escape_cmd_argument(arg: &Spanned<String>) -> Result<Cow<'_, str>, ShellError> {
  let Spanned { item: arg, span } = arg;
- if arg.contains('"') {
+ if arg.contains(['\r', '\n', '%']) {
+ // \r and \n trunacte the rest of the arguments and % can expand environment variables
+ Err(ShellError::ExternalCommand {
+ label:
+ "Arguments to CMD internal commands cannot contain new lines or percent signs '%'"
+ .into(),
+ help: "some characters currently cannot be securely escaped".into(),
+ span: *span,
+ })
+ } else if arg.contains('"') {
  // If `arg` is already quoted by double quotes, confirm there's no
  // embedded double quotes, then leave it as is.
  if arg.chars().filter(|c| *c == '"').count() == 2
@@ -582,14 +590,15 @@ fn escape_cmd_argument(arg: &Spanned<String>) -> Result<Cow<'_, str>, ShellError
  Err(ShellError::ExternalCommand {
  label: "Arguments to CMD internal commands cannot contain embedded double quotes"
  .into(),
- help: "CMD doesn't support escaping double quotes inside double quotes".into(),
+ help: "this case currently cannot be securely handled".into(),
  span: *span,
  })
  }
  } else if arg.contains(' ') || has_cmd_special_character(arg) {
  // If `arg` contains space or special characters, quote the entire argument by double quotes.
  Ok(Cow::Owned(format!("\"{arg}\"")))
  } else {
+ // FIXME?: what if `arg.is_empty()`?
  Ok(Cow::Borrowed(arg))
  }
 }