diff --git a/TODO.md b/TODO.md index e95e6a2..66483cd 100644 --- a/TODO.md +++ b/TODO.md @@ -1,39 +1,68 @@ # alpha.jayfield.org — TODO Outstanding hardening items (see `README.md` for the full service overview). -Everything below is low-risk/no-downtime **except the item marked -HIGH PRIORITY**. +Nothing here is urgent; all are low-risk, no-downtime changes. -- [ ] **HIGH PRIORITY — OS command injection in the dyndns update scripts** — found 2026-07-20 +- [x] **OS command injection in the dyndns update scripts** — done 2026-07-20 `/var/www/dyndns/nsupdate.php`, `nsupdate_ipv4.php`, and `nsupdate_ipv6.php` (used by both `index.php` and `nic/update.php`, - the router/ddclient-facing endpoint) build a shell command string by + the router/ddclient-facing endpoint) built a shell command string by directly interpolating unsanitized `$_GET` input (`hostname`/`myip`/ - `ip`, which become `$host`/`$ip`/`$ipv4`/`$ipv6`) and pass it to + `ip`, which become `$host`/`$ip`/`$ipv4`/`$ipv6`) and passed it to PHP's `exec()`, which runs it via `/bin/sh -c` — no `escapeshellarg()`/`escapeshellcmd()` anywhere. A request like - `hostname=$(id>/tmp/pwned).dyndns.jayfield.org` would execute + `hostname=$(id>/tmp/pwned).dyndns.jayfield.org` would have executed arbitrary shell commands as `www-data` (the domain-suffix check - still passes after the `.`-split). `nic/update.php` has no + still passed after the `.`-split). `nic/update.php` had no application-level auth of its own at all. Mitigating factor: the whole `dyndns.jayfield.org` vhost sits behind - Apache Basic Auth, so this needs valid (or leaked/brute-forced) + Apache Basic Auth, so this needed valid (or leaked/brute-forced) dyndns credentials to reach — not exploitable by an anonymous - visitor. Compounding factors: PHP is `7.4.33`, EOL since Nov 2022, - `disable_functions` is empty (`exec` fully available); `index.php`'s - `pass == '_NO_PASS_'` check is a hardcoded magic string, not real - per-user auth, so any valid dyndns credential can update *any* of - the zone's 5 hostnames (`jens`, `kack`, `test`, `uschi`, `vpn`), not - just its own; and the vhost's access logging turned out to be a - no-op (shared `access.log` has 0 lines), so past exploitation can't - be fully ruled out from logs alone — though file mtimes, `www-data` - processes/crontab, and current zone contents all look unremarkable, - consistent with unexploited. - Fix: switch `exec()` to pass the nsupdate command list via - `proc_open`/`popen` writing to stdin (no shell interpolation of - user input at all), or at minimum wrap every interpolated value in - `escapeshellarg()`; add strict `filter_var`/regex validation on the - hostname label and IP before use. Not yet applied. + visitor. Compounding factors found during the audit: PHP is + `7.4.33`, EOL since Nov 2022, `disable_functions` was empty (`exec` + fully available); `index.php`'s `pass == '_NO_PASS_'` check is a + hardcoded magic string, not real per-user auth, so any valid dyndns + credential can update *any* of the zone's 5 hostnames (`jens`, + `kack`, `test`, `uschi`, `vpn`), not just its own — left as-is, out + of scope for this fix; and the vhost's access logging turned out to + be a no-op (shared `access.log` has 0 lines), so past exploitation + couldn't be fully ruled out from logs alone — though file mtimes, + `www-data` processes/crontab, and zone contents all looked + unremarkable, consistent with unexploited. + Fixed: all three files backed up to + `/root/removed-configs-backup/dyndns-preinjectionfix-/`, + then rewritten to build the nsupdate command list as a plain string + written to `proc_open()`'s stdin pipe using the **array form** of + the command (`array("/usr/bin/nsupdate", "-k", ...)`) — this never + invokes a shell at all, so no value can be interpreted as shell + syntax regardless of content. Added strict `preg_match` validation + requiring `$host`/`$domain` to be shaped like a valid hostname + (letters/digits/hyphens, dot-separated labels) and + `filter_var(..., FILTER_VALIDATE_IP, FILTER_FLAG_IPV4/IPV6)` on the + IP before use, as defense in depth on top of removing the shell. + **Regression found and fixed along the way**: functional testing + showed legitimate updates were *already silently failing* + (`update failed: REFUSED`, `permission denied` reading the TSIG + key) — turned out `www-data` was never granted read access to + `/etc/bind/dyndns.jayfield.org.key` when its permissions were + tightened to `640 root:bind` in the item below, so the dyndns web + update feature had been broken for real users since 2026-07-19, + independent of this vulnerability. Fixed with a narrowly-scoped + POSIX ACL (`setfacl -m u:www-data:r + /etc/bind/dyndns.jayfield.org.key`, `acl` package installed) rather + than adding `www-data` to the `bind` group — the group also owns + `rndc.key` (full remote BIND control), which would have been a much + bigger privilege grant than dyndns needs. + Verified via `sudo -u www-data php` driving the real functions + directly (same code path as the web UI, without needing the Basic + Auth password): two injection payloads (`$(...)` and `;`-chained) + both rejected (`-1`, no `/tmp` side-effect file created), an + invalid IP rejected (`-1`), and a legitimate update succeeded + (`0`), confirmed live in the zone via `dig`, then cleaned up (test + record deleted via `nsupdate`, temp test script removed). Also + confirmed unauthenticated requests still get `401` from Apache + before ever reaching the PHP, and `apache2ctl configtest` stayed + clean throughout. - [x] **Public static site (`jayfield.org`/`www.jayfield.org`) was 401'ing every visitor** — done 2026-07-20 Found while auditing which services are reachable without credentials: