Skip to content

Potential fix for code scanning alert no. 2: Uncontrolled command line#17

Draft
TheInfamousToTo wants to merge 1 commit intomainfrom
alert-autofix-2
Draft

Potential fix for code scanning alert no. 2: Uncontrolled command line#17
TheInfamousToTo wants to merge 1 commit intomainfrom
alert-autofix-2

Conversation

@TheInfamousToTo
Copy link
Copy Markdown
Owner

Potential fix for https://github.com/TheInfamousToTo/PiHoleVault/security/code-scanning/2

In general, this should be fixed by eliminating shell interpretation of the user-controlled host value. Instead of constructing a single command string for execSync, use a function such as child_process.execFileSync that takes the program and its arguments as an array, so the host value is passed as a literal argument and not parsed by a shell. Optionally, add simple validation (e.g. allow only typical hostname/IP characters) to further reduce risk, but the primary requirement is to stop feeding user input to a shell.

The single best fix here, without changing existing behavior, is to replace the execSync call on line 235 with execFileSync('ping', ['-c', '1', '-W', '3', host], { timeout: 5000 }). This preserves the same command semantics (ping -c 1 -W 3 <host>), but runs ping directly without going through a shell, so characters like ;, &&, or backticks in host can no longer trigger additional commands. Since execSync from child_process is already imported at the top and re-required inside the function, we can either (a) keep the inner require but swap to execFileSync, or (b) rely on the top-level import. To keep the change as local and minimal as possible, we will adjust the inner require from execSync to execFileSync and update the call accordingly. No change to routing, logging, or response structure is necessary.

Concretely:

  • In backend/routes/ssh.js, in the /debug route’s "Test 1: Basic network connectivity" block, change const { execSync } = require('child_process'); to const { execFileSync } = require('child_process');.
  • Replace execSync(\ping -c 1 -W 3 ${host}`, { timeout: 5000 })withexecFileSync('ping', ['-c', '1', '-W', '3', host], { timeout: 5000 })`.
  • Keep the rest of the logic (try/catch, debug object, .toString() conversion, etc.) unchanged.

This fix uses only Node’s built-in child_process module and does not require any new dependencies.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant