Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection in unix system queries #165

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

Open
Donach wants to merge 1 commit into
base: cfx/android-hermes-client
Choose a base branch
from
Open

🛡️ Sentinel: [CRITICAL] Fix command injection in unix system queries #165

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
5 changes: 5 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,8 @@
**Vulnerability:** The daemon configuration file (`~/.agor/config.yaml`) and its parent directory (`~/.agor`) were created with default file permissions (e.g., `0o755`/`0o644`), which made them readable by other users on the system. This file stores extremely sensitive information such as API keys and master JWT secrets.
**Learning:** Default Node.js filesystem operations (`fs.writeFile` and `fs.mkdir`) do not enforce strict permissions unless explicitly specified with a `mode` parameter. When handling sensitive files, relying on the system `umask` is insufficient.
**Prevention:** Always specify `mode: 0o600` for sensitive files and `mode: 0o700` for their parent directories. Additionally, use `fs.chmod` to retroactively secure existing files and directories that might have been created with permissive defaults.

## 2024-05-22 - [CRITICAL] Fix command injection vulnerability in unix system queries
**Vulnerability:** The daemon used `execSync` from `node:child_process` in `packages/core/src/unix/system-queries.ts` to execute shell commands like `getent` and `id` which took dynamic parameters like `username` or `groupName`. Due to the usage of double quotes around parameters being interpreted by a shell environment, this resulted in a command injection vulnerability.
**Learning:** Avoid using `execSync` with string interpolation for dynamic arguments (like usernames, group names, or paths). Always use `execFileSync` to prevent shell command injection, as double quoting in `execSync` does not fully protect against backticks or escaped quotes.
**Prevention:** Always use `execFileSync` with its arguments array whenever possible instead of formatting command strings for the implicit shell environment to process.
22 changes: 9 additions & 13 deletions packages/core/src/unix/system-queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
* @see context/guides/rbac-and-unix-isolation.md
*/

import { execSync } from 'node:child_process';
import { UnixGroupCommands } from './group-manager.js';
import { UnixUserCommands } from './user-manager.js';
import { execFileSync } from 'node:child_process';

// ============================================================
// USER QUERIES
Expand All @@ -27,7 +25,7 @@ import { UnixUserCommands } from './user-manager.js';
*/
export function getUserGroups(username: string): string[] {
try {
const output = execSync(UnixUserCommands.getUserGroups(username), {
const output = execFileSync('id', ['-nG', '--', String(username)], {
encoding: 'utf-8',
stdio: ['pipe', 'pipe', 'ignore'],
});
Expand All @@ -37,6 +35,8 @@ export function getUserGroups(username: string): string[] {
}
}

import { execSync } from 'node:child_process';

/**
* List all agor_* users on the system (auto-generated format: agor_<8-hex>)
*
Expand Down Expand Up @@ -69,7 +69,7 @@ export function listAgorUsers(): string[] {
*/
export function groupExists(groupName: string): boolean {
try {
execSync(UnixGroupCommands.groupExists(groupName), { stdio: 'ignore' });
execFileSync('getent', ['group', '--', String(groupName)], { stdio: 'ignore' });
return true;
} catch {
return false;
Expand All @@ -84,12 +84,7 @@ export function groupExists(groupName: string): boolean {
* @returns true if user is in group
*/
export function isUserInGroup(username: string, groupName: string): boolean {
try {
execSync(UnixGroupCommands.isUserInGroup(username, groupName), { stdio: 'ignore' });
return true;
} catch {
return false;
}
return getUserGroups(username).includes(groupName);
}

/**
Expand All @@ -100,11 +95,12 @@ export function isUserInGroup(username: string, groupName: string): boolean {
*/
export function getGroupMembers(groupName: string): string[] {
try {
const output = execSync(UnixGroupCommands.listGroupMembers(groupName), {
const output = execFileSync('getent', ['group', '--', String(groupName)], {
encoding: 'utf-8',
stdio: ['pipe', 'pipe', 'ignore'],
});
return output.trim().split(',').filter(Boolean);
const parts = output.trim().split(':');
return parts[3] ? parts[3].split(',').filter(Boolean) : [];
} catch {
return [];
}
Expand Down