🛡️ Sentinel: [CRITICAL] Fix command injection in unix user utilities#74
Conversation
In `packages/core/src/unix/id-lookups.ts` and `user-manager.ts`, `execSync` was being used with raw string interpolation containing dynamic values (username, groupName). This exposes a serious Command Injection vulnerability because `execSync` evaluates the command in a shell environment, meaning any shell metacharacters could be maliciously executed. This PR remediates this by switching to `execFileSync` and passing command arguments as an array, completely bypassing shell evaluation. Furthermore, `--` is added before the dynamic variables to ensure they cannot be parsed as unexpected flags. Test mocks are also appropriately updated. Co-authored-by: Donach <39565367+Donach@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. |
1 participant
Severity
CRITICAL
Vulnerability
Command Injection vulnerability in core Unix utilities (
packages/core/src/unix/id-lookups.tsanduser-manager.ts).execSyncwith raw string interpolation was used to construct commands with dynamic variables (username, groupName), which allowed for shell metacharacters to execute arbitrary commands as the application user.Impact
If an attacker could control the
usernameorgroupNameinputs passed to these low-level core utilities, they could gain arbitrary command execution on the host machine running Agor daemon.Fix
Swapped
execSyncinstances that invoke shell commands toexecFileSync. We passed arguments via the array parameter method, taking away the shell evaluation context entirely. The fix also included['--', dynamicVar]which acts as a robust double-layer defense against argument injection. Lastly, updated the Vitest specs to properly mock the new behavior.Verification
@agor/corepass flawlessly (vitest run "src/unix/").PR created automatically by Jules for task 6203574043206537204 started by @Donach