🛡️ Sentinel: [CRITICAL] Fix command injection vulnerability in validation command#600
🛡️ Sentinel: [CRITICAL] Fix command injection vulnerability in validation command#600mudcube wants to merge 1 commit into
Conversation
…tion command - Refactored `run_validation` to parse the incoming command string into its program and arguments prior to security validation. - Validates the parsed program and arguments against parsed safe prefixes, preventing argument injection attacks that abuse `starts_with`. - Added test cases and security journal entry. 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: |
ec19e50
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8a088c2a.typemill.pages.dev |
| Branch Preview URL: | https://sentinel-fix-argument-inject.typemill.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec19e508a9
ℹ️ 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".
| return false; | ||
| } | ||
| } | ||
| true |
There was a problem hiding this comment.
When validation_config.command is attacker-controlled, this prefix-only approval still treats make test -f /dev/null --eval="test:; touch /tmp/pwned" as safe because the parsed program is make and the first arg is test, so execution reaches Command::new. Checked make --help: -f FILE reads an arbitrary makefile and --eval=STRING evaluates a makefile statement, so the injected recipe is executed despite the new allowlist parsing. This leaves an argument-injection path in the security fix; either reject extra args for make or validate allowed options per command.
Useful? React with 👍 / 👎.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
run_validationcommand used an insecure permissivestarts_withcheck to validate commands against an allowlist. This allowed an attacker to bypass the allowlist via argument injection (e.g.cargo check; echo pwnedstarts withcargo checkbut allows arbitrary code execution).🎯 Impact: Allowed arbitrary code execution within the worker context via manipulated project configuration files.
🔧 Fix: Refactored the validation logic to parse the user-supplied command into a
programand list ofargsusingparse_command_linebefore the security check. The parsed program and its arguments are then strictly verified against the parsed elements of the allowed prefixes. Added inline security context comments.✅ Verification: Created test
test_command_injection_reproinsecurity_test.rswhich verifies that the vulnerability is neutralized. Rancargo test,cargo fmt, andcargo clippy. Added critical learning to.jules/sentinel.md.PR created automatically by Jules for task 7202719202249299254 started by @mudcube