π‘οΈ Sentinel: [CRITICAL] Fix command and argument injection in run_validation#584
π‘οΈ Sentinel: [CRITICAL] Fix command and argument injection in run_validation#584mudcube wants to merge 1 commit into
Conversation
Refactored validation execution in `utils.rs` and `post_apply.rs` to securely parse commands and strictly match element-by-element against an allowlist, preventing command injection via shell interpreters and argument injection via permissive string checks. Co-authored-by: mudcube <101564+mudcube@users.noreply.github.com>
|
π Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a π emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Deploying typemill with Β
|
| Latest commit: |
e5faf10
|
| Status: | Β β Β Deploy successful! |
| Preview URL: | https://ce4e6d08.typemill.pages.dev |
| Branch Preview URL: | https://sentinel-fix-command-injecti-5808.typemill.pages.dev |
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5faf10976
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Check exact element-by-element match to prevent argument injection | ||
| prefix_elements | ||
| .iter() | ||
| .enumerate() | ||
| .all(|(i, &p)| parsed_args[i] == p) |
There was a problem hiding this comment.
Reject unsafe suffix arguments
When the validation command comes from untrusted or user-editable config, matching only the first tokens still permits dangerous tool-specific arguments after an allowed prefix. For example, cargo check --manifest-path /tmp/evil/Cargo.toml passes this check (I checked cargo check --help, which documents --manifest-path <PATH>), and make test -f /tmp/Makefile/make test --eval=... also pass (GNU make help documents -f and --eval); those tools can then execute attacker-controlled build scripts or recipes even though the command is considered safe. The allowlist needs to constrain or reject unsafe extra arguments, not only verify the prefix.
Useful? React with πΒ / π.
π¨ Severity: CRITICAL
π‘ Vulnerability: The
run_validationfunctions inmill-serviceswere vulnerable to command and argument injection.post_apply.rsexecuted commands usingsh -candcmd.exe /Cwithout any validation, allowing arbitrary shell command injection (e.g.,cargo check ; rm -rf /).utils.rsimplemented a security policy but used a permissivestarts_withcheck on the raw string, allowing argument injection (e.g.,cargo checkmatebypassing thecargo checkallowed prefix).π― Impact: An attacker or malicious configuration could execute arbitrary system commands on the server or developer's machine with the privileges of the running application, leading to full system compromise, data exfiltration, or denial of service.
π§ Fix:
sh -candcmd.exe /Cexecution inpost_apply.rs.shlexto safely parse raw command strings into program and arguments.starts_withcheck with strict, element-by-element matching of the parsed command against the allowed safe prefixes in bothutils.rsandpost_apply.rs.Command::new(program).args(args).β Verification: Verified by running the test suite (
cargo test -p mill-services) which confirms that valid commands (likecargo checkandcargo test --help) are allowed and executed correctly, while invalid or potentially malicious commands (likenonexistent_command_12345orsleep 10) are strictly blocked by the security policy.PR created automatically by Jules for task 2452698341072403271 started by @mudcube