Disallow more characters in arguments for internal cmd commands#13009
Merged
IanManske merged 3 commits intonushell:mainfrom May 30, 2024
Merged
Disallow more characters in arguments for internal cmd commands#13009IanManske merged 3 commits intonushell:mainfrom
cmd commands#13009IanManske merged 3 commits intonushell:mainfrom
Conversation
Contributor
|
+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. |
Member
Author
|
Thanks for testing it @fdncred! I'll land this for now and we can add more fixes on top of this if necessary. |
Contributor
Otherwise this looks good. I'm not sure if newlines actually truncate arguments either, but better be safe than sorry. |
Contributor
They do. You can test this by using Test programuse 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,
);
}
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Description
Makes
run-externalerror if arguments tocmd.exeinternal 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
cmd.exedirectly, then this bypasses our handling/checking for internalcmdcommands. 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 runcmdwith 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 incmd echo /c '""'.cmdis called indirectly via a.bator.cmdfile, 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 handleIt looks like we use thePATHEXT, 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.whichcrate to do the lookup usingPATHEXT. Then, we pass the exe path from that to the Rust std libCommand, which should be safe (except for the firstcmd.exenote).So, in the future we need to unify and/or fix these different implementations, including our own special handling for internal
cmdcommands that this PR tries to fix.