Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection in terminal service #169

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
+10 −2
Open

🛡️ Sentinel: [CRITICAL] Fix command injection in terminal service #169

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.

## 2025-05-14 - [CRITICAL] Fix command injection in sudo chown
**Vulnerability:** A critical command injection vulnerability in `apps/agor-daemon/src/services/terminals.ts` where user-controlled input (`chownTo`) was interpolated directly into a shell command executed via `execSync`. Even with double quotes, shell execution permits injection via backticks or escaped quotes, potentially leading to root privilege escalation because of the `sudo` command.
**Learning:** Double-quoting dynamic arguments in `execSync` is insufficient to prevent command injection in a shell environment.
**Prevention:** Always use `execFileSync` with an array of arguments, prefixing dynamic parameters with `--`, and casting dynamic parameters to `String()` to avoid shell injection entirely.
7 changes: 5 additions & 2 deletions apps/agor-daemon/src/services/terminals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
* - xterm.js frontend for rendering
*/

import { execSync } from 'node:child_process';
import { execFileSync, execSync } from 'node:child_process';
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
Expand Down Expand Up @@ -109,7 +109,10 @@ ${exportLines.join('\n')}
try {
// CRITICAL: Use -n flag to prevent password prompts that freeze the system
// Also add timeout to prevent any hangs
execSync(`sudo -n chown "${chownTo}" "${envFile}"`, { stdio: 'pipe', timeout: 2000 });
execFileSync('sudo', ['-n', 'chown', '--', String(chownTo), String(envFile)], {
stdio: 'pipe',
timeout: 2000,
});
} catch (chownError) {
console.warn(`Failed to chown env file to ${chownTo}:`, chownError);
// Continue anyway - file may still be readable in some configurations
Expand Down