Skip to content

Potential fix for code scanning alert no. 28: Server-side request forgery#16

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

Potential fix for code scanning alert no. 28: Server-side request forgery#16
TheInfamousToTo wants to merge 1 commit intomainfrom
alert-autofix-28

Conversation

@TheInfamousToTo
Copy link
Copy Markdown
Owner

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

In general, to fix SSRF in this context, we must restrict which hosts and ports the server is willing to contact based on user input. That usually means: validating host format; blocking localhost and link-local/metadata IP ranges; optionally enforcing an allow-list of domains/IP ranges; and restricting ports to a reasonable, expected set or range. We should also ensure we do not accidentally permit DNS rebinding tricks where an allowed hostname resolves to a forbidden IP.

For this specific route, the best minimally invasive fix is:

  1. Add validation helpers near the top of backend/routes/ssh.js:

    • A function to check if a string is a valid hostname or IP.
    • A function that, after resolving the hostname (via dns.promises.lookup), checks that the resulting IP is not loopback, private, link-local, or otherwise sensitive (e.g., 127.0.0.0/8, 10.0.0.0/8, 192.168.0.0/16, 172.16.0.0/12, 169.254.0.0/16, 0.0.0.0, ::1, etc.).
    • A function to validate the port: numeric, within 1–65535, and possibly only a small subset of ports if that fits the app (but to avoid changing behavior too much, we can allow full range but block a few obviously problematic low ports if desired; the key is to ensure non-numeric values are rejected).
  2. In the /debug route, before performing any network operation (ping, socket.connect, SSH), call the validation helpers:

    • If validation fails, return 400 with an error explaining invalid or disallowed host/port.
    • Use the validated/normalized host and port values (e.g., resolvedHost and validatedPort) in ping and socket.connect instead of the raw user input.
  3. Since socket.connect is the sink CodeQL is flagging, ensuring that port and host have passed strict validation and IP-range checks will address the alert. We will also switch the second argument of socket.connect to use the resolved, allowed IP address, not the raw hostname.

To implement this:

  • Add a dns import (const dns = require('dns').promises;) at the top of backend/routes/ssh.js.
  • Add helper functions (e.g., isValidHostnameOrIP, isDisallowedIp, resolveAndValidateHost, validatePort) before the route definitions.
  • In /debug, before building debug or running tests, call await resolveAndValidateHost(host) and validatePort(port). Use the returned safe values in the rest of the function, especially in socket.connect(...) and ping ....

This keeps current functionality (testing connectivity to a host/port) but prevents use with internal/loopback/metadata addresses and invalid ports, satisfying SSRF concerns.


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

…gery

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.

2 participants