Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection in Unix utilities #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Donach wants to merge 1 commit into from
Closed

🛡️ Sentinel: [CRITICAL] Fix command injection in Unix utilities #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@
**Vulnerability:** A potential SQL injection vulnerability in `createMany` inside `packages/core/src/db/repositories/tasks.ts`. The code used `sql.raw` to interpolate task IDs into an `IN` clause manually without parameterization.
**Learning:** Using `sql.raw` to join dynamically generated strings (like UUID arrays) bypasses Drizzle's parameterization, exposing the app to SQL injection if any ID string is manipulated.
**Prevention:** Always use Drizzle ORM's built-in parameterization functions like `inArray` instead of manual string concatenation or `sql.raw`.
## 2025-05-16 - [Command Injection Risk via execSync in Unix Lookups]
**Vulnerability:** A critical command injection vulnerability existed in `packages/core/src/unix/id-lookups.ts` and `apps/agor-cli/src/commands/admin/sync-unix.ts` where unvalidated user input (`username`, `groupName`, `sudoUser`) was concatenated into a string passed to `execSync`.
**Learning:** Functions that execute shell commands using `execSync` (`node:child_process`) are vulnerable to command injection if unvalidated strings are embedded within them, because `execSync` spins up a shell process.
**Prevention:** Avoid `execSync` or `exec` completely when handling dynamic variables. Always use `execFileSync` or `spawnSync` instead, passing the executable name and an array of distinct arguments. This prevents the command from being run inside an interactive shell, mitigating shell metacharacter injection.
4 changes: 2 additions & 2 deletions apps/agor-cli/src/commands/admin/sync-unix.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* @see context/guides/rbac-and-unix-isolation.md
*/

import { execSync } from 'node:child_process';
import { execFileSync } from 'node:child_process';
import { existsSync, readlinkSync } from 'node:fs';
import { homedir } from 'node:os';
import { join } from 'node:path';
Expand Down Expand Up @@ -236,7 +236,7 @@ export default class SyncUnix extends Command {
// Running under sudo - use the invoking user's home directory
// Try to get home directory from passwd entry
try {
const passwdEntry = execSync(`getent passwd ${sudoUser}`, {
const passwdEntry = execFileSync('getent', ['passwd', sudoUser], {
encoding: 'utf-8',
stdio: ['pipe', 'pipe', 'ignore'],
}).trim();
Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/unix/id-lookups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* Supports both Linux (using getent) and macOS (parsing /etc/group and /etc/passwd)
*/

import { execSync } from 'node:child_process';
import { execFileSync } from 'node:child_process';
import fs from 'node:fs';

/**
Expand All @@ -26,7 +26,7 @@ export function getGidFromGroupName(groupName: string | undefined | null): numbe
try {
// Try getent first (Linux, some BSD)
try {
const result = execSync(`getent group "${groupName}"`, {
const result = execFileSync('getent', ['group', groupName], {
encoding: 'utf-8',
stdio: 'pipe',
timeout: 2000,
Expand Down Expand Up @@ -91,7 +91,7 @@ export function getUidFromUsername(username: string | undefined | null): number
try {
// Try `id -u username` first (most reliable)
try {
const result = execSync(`id -u "${username}"`, {
const result = execFileSync('id', ['-u', username], {
encoding: 'utf-8',
stdio: 'pipe',
timeout: 2000,
Expand All @@ -107,7 +107,7 @@ export function getUidFromUsername(username: string | undefined | null): number

// Try getent (Linux, some BSD)
try {
const result = execSync(`getent passwd "${username}"`, {
const result = execFileSync('getent', ['passwd', username], {
encoding: 'utf-8',
stdio: 'pipe',
timeout: 2000,
Expand Down Expand Up @@ -171,7 +171,7 @@ export function getHomedirFromUsername(username: string | undefined | null): str
try {
// Try getent first (Linux, some BSD)
try {
const result = execSync(`getent passwd "${username}"`, {
const result = execFileSync('getent', ['passwd', username], {
encoding: 'utf-8',
stdio: 'pipe',
timeout: 2000,
Expand Down