-
Notifications
You must be signed in to change notification settings - Fork 0
🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs API #74
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.
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| ## 2025-02-23 - Command injection in Docker Logs API | ||
| **Vulnerability:** A critical command injection existed in `src/pages/api/docker-logs.ts` where unvalidated query parameters (`id` and `tail`) were directly interpolated into an `execSync` shell command. An attacker could bypass API validation to run arbitrary commands on the system. | ||
| **Learning:** `execSync` and `exec` interpret their string arguments via a shell, making them inherently vulnerable to injection if user input is not strictly validated and sanitized. Merely checking for existence is not sufficient. | ||
| **Prevention:** Use `execFileSync` or `execFile` with an argument array instead of string concatenation to prevent the shell from interpreting operators (like `;`, `|`, `&&`). Add strong input validation using regex (e.g. `/^[a-zA-Z0-9_-]+$/`) and type conversion (`parseInt`) to ensure arguments match expected formats. Add `--` separator to avoid argument injection where inputs might be treated as CLI flags. Sanitize server error messages returned to clients so that internals (like shell commands or stack traces) aren't leaked. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,49 +1,81 @@ | ||||||
| import type { APIRoute } from 'astro'; | ||||||
| import { execSync } from 'child_process'; | ||||||
| import type { APIRoute } from "astro"; | ||||||
| import { execFileSync } from "child_process"; | ||||||
|
|
||||||
| export const GET: APIRoute = async ({ url }) => { | ||||||
| try { | ||||||
| const isVercel = !!process.env.VERCEL || !!process.env.VERCEL_ENV; | ||||||
| if (isVercel) { | ||||||
| return new Response(JSON.stringify({ logs: ["Docker logs non disponibles sur Vercel"] }), { | ||||||
| status: 200, | ||||||
| headers: { 'Content-Type': 'application/json' } | ||||||
| }); | ||||||
| return new Response( | ||||||
| JSON.stringify({ logs: ["Docker logs non disponibles sur Vercel"] }), | ||||||
| { | ||||||
| status: 200, | ||||||
| headers: { "Content-Type": "application/json" }, | ||||||
| }, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| const containerId = url.searchParams.get('id'); | ||||||
| const tail = url.searchParams.get('tail') || '100'; | ||||||
| const containerId = url.searchParams.get("id"); | ||||||
| const tailInput = url.searchParams.get("tail") || "100"; | ||||||
|
|
||||||
| if (!containerId) { | ||||||
| return new Response(JSON.stringify({ error: "ID du conteneur manquant" }), { | ||||||
| status: 400, | ||||||
| headers: { 'Content-Type': 'application/json' } | ||||||
| }); | ||||||
| if (!containerId || typeof containerId !== "string") { | ||||||
| return new Response( | ||||||
| JSON.stringify({ error: "ID du conteneur manquant" }), | ||||||
| { | ||||||
| status: 400, | ||||||
| headers: { "Content-Type": "application/json" }, | ||||||
| }, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| // Input validation: prevent command argument injection (no flags/unsupported chars) | ||||||
| if (!/^[a-zA-Z0-9_-]+$/.test(containerId)) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docker container names can contain dots (
Suggested change
|
||||||
| return new Response( | ||||||
| JSON.stringify({ error: "Format ID conteneur invalide" }), | ||||||
| { | ||||||
| status: 400, | ||||||
| headers: { "Content-Type": "application/json" }, | ||||||
| }, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| const tail = parseInt(tailInput, 10); | ||||||
| if (isNaN(tail) || tail < 1 || tail > 10000) { | ||||||
| return new Response( | ||||||
| JSON.stringify({ error: "Paramètre tail invalide" }), | ||||||
| { | ||||||
| status: 400, | ||||||
| headers: { "Content-Type": "application/json" }, | ||||||
| }, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| // Commande Docker pour récupérer les logs | ||||||
| const command = `docker logs --tail ${tail} ${containerId}`; | ||||||
| let logs = []; | ||||||
| try { | ||||||
| const output = execSync(command, { stdio: ['pipe', 'pipe', 'pipe'] }).toString(); | ||||||
| logs = output.trim().split('\n'); | ||||||
| // Using execFileSync with an argument array to prevent shell injection | ||||||
| const output = execFileSync( | ||||||
| "docker", | ||||||
| ["logs", "--tail", String(tail), "--", containerId], | ||||||
| { stdio: ["pipe", "pipe", "pipe"] }, | ||||||
| ).toString(); | ||||||
| logs = output.trim().split("\n"); | ||||||
| } catch (err: any) { | ||||||
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||||||
| if (err.stderr) { | ||||||
| logs = err.stderr.toString().trim().split('\n'); | ||||||
| } else { | ||||||
| throw err; | ||||||
| } | ||||||
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||||||
| if (err.stderr) { | ||||||
| logs = err.stderr.toString().trim().split("\n"); | ||||||
| } else { | ||||||
| throw err; | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
52
to
68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace let logs = [];
try {
// Using execFileAsync with an argument array to prevent shell injection and avoid blocking the event loop
const { stdout } = await execFileAsync(
"docker",
["logs", "--tail", String(tail), "--", containerId]
);
logs = stdout.toString().trim().split("\n");
} catch (err: any) {
// Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur
if (err.stderr) {
logs = err.stderr.toString().trim().split("\n");
} else {
throw err;
}
} |
||||||
|
|
||||||
| return new Response(JSON.stringify({ logs }), { | ||||||
| status: 200, | ||||||
| headers: { 'Content-Type': 'application/json' } | ||||||
| return new Response(JSON.stringify({ logs }), { | ||||||
| status: 200, | ||||||
| headers: { "Content-Type": "application/json" }, | ||||||
| }); | ||||||
| } catch (error: any) { | ||||||
| return new Response(JSON.stringify({ error: "Logs indisponibles: " + error.message }), { | ||||||
| status: 500, | ||||||
| headers: { 'Content-Type': 'application/json' } | ||||||
| // Return a sanitized error message | ||||||
| return new Response(JSON.stringify({ error: "Logs indisponibles" }), { | ||||||
| status: 500, | ||||||
| headers: { "Content-Type": "application/json" }, | ||||||
| }); | ||||||
| } | ||||||
| }; | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using synchronous process execution (
execFileSync) in an API route blocks the Node.js event loop. This can lead to severe performance degradation or Denial of Service (DoS) when multiple requests are handled concurrently, especially since container logs can be large or slow to retrieve. Switch to the asynchronousexecFilewrapped in a Promise instead.