-
Notifications
You must be signed in to change notification settings - Fork 0
🛡️ Sentinel: [CRITICAL] Fix command injection in docker-logs API #41
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-28 - [CRITICAL] Command Injection via `execSync` in Docker Logs API | ||
| **Vulnerability:** In `src/pages/api/docker-logs.ts`, `execSync` was used with a string interpolation of `containerId` and `tail` query parameters (`docker logs --tail ${tail} ${containerId}`). This allowed arbitrary command execution and argument injection if a user passed `tail=100; whoami` or a container ID starting with `-`. Additionally, API error messages leaked raw application error details which could be used for fingerprinting. | ||
| **Learning:** Shell-based command execution (like `execSync` and `exec` with a single command string) is highly dangerous when any part of the command contains user-controlled input, as input can break out of the command. | ||
| **Prevention:** Always use `execFile` or `execFileAsync` where the base executable (e.g., `'docker'`) is strictly separated from its arguments array (e.g., `['logs', '--tail', tail, containerId]`). Also, validate all user input to ensure they don't start with a hyphen (`-`) to prevent argument/flag injection, and sanitize error messages in API endpoints to not leak internal application stack traces. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import type { APIRoute } from 'astro'; | ||
| import { execSync } from 'child_process'; | ||
| import { execFile } from 'child_process'; | ||
| import { promisify } from 'util'; | ||
|
|
||
| const execFileAsync = promisify(execFile); | ||
|
|
||
| export const GET: APIRoute = async ({ url }) => { | ||
| try { | ||
|
|
@@ -21,18 +24,27 @@ export const GET: APIRoute = async ({ url }) => { | |
| }); | ||
| } | ||
|
|
||
| // Commande Docker pour récupérer les logs | ||
| const command = `docker logs --tail ${tail} ${containerId}`; | ||
| let logs = []; | ||
| // Validation des entrées pour prévenir l'injection d'arguments | ||
| if (containerId.startsWith('-') || tail.startsWith('-')) { | ||
| return new Response(JSON.stringify({ error: "Format d'entrée invalide" }), { | ||
| status: 400, | ||
| headers: { 'Content-Type': 'application/json' } | ||
| }); | ||
| } | ||
|
|
||
| let logs: string[] = []; | ||
| try { | ||
| const output = execSync(command, { stdio: ['pipe', 'pipe', 'pipe'] }).toString(); | ||
| logs = output.trim().split('\n'); | ||
| const { stdout, stderr } = await execFileAsync('docker', ['logs', '--tail', tail, containerId]); | ||
| // Certains logs sortent sur stderr, fusionnons ou prenons stdout si dispo | ||
| const output = stdout.trim() || stderr.trim(); | ||
| logs = output ? output.split('\n') : []; | ||
| } catch (err: any) { | ||
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||
| // En cas d'erreur de commande (ex: conteneur introuvable), docker renvoie les infos sur stderr | ||
| if (err.stderr) { | ||
| logs = err.stderr.toString().trim().split('\n'); | ||
|
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. This line returns the raw |
||
| } else { | ||
| throw err; | ||
| // Ne pas fuiter le message d'erreur brut au client pour des raisons de sécurité | ||
| throw new Error("Erreur d'exécution de la commande"); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -41,7 +53,8 @@ export const GET: APIRoute = async ({ url }) => { | |
| headers: { 'Content-Type': 'application/json' } | ||
| }); | ||
| } catch (error: any) { | ||
| return new Response(JSON.stringify({ error: "Logs indisponibles: " + error.message }), { | ||
| // Ne pas fuiter le message d'erreur brut au client pour des raisons de sécurité | ||
| return new Response(JSON.stringify({ error: "Logs indisponibles suite à une erreur interne" }), { | ||
| 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.
The code uses the logical OR operator (
||), which means ifstdouthas any content,stderris completely ignored. This contradicts the comment on line 38 ("fusionnons") and leads to incomplete logs, as many Dockerized applications distribute their output across both streams (e.g., access logs to stdout and error logs to stderr). To ensure all logs are captured, you should concatenate both streams.