Fix command injection in dyndns update scripts

nsupdate.php/nsupdate_ipv4.php/nsupdate_ipv6.php rewritten to build the
nsupdate command as a string written to proc_open()'s stdin, using the
array form of the command so no shell is ever invoked -- no value can be
interpreted as shell syntax regardless of content. Added hostname/IP
validation as defense in depth. Originals backed up.

Also fixes a regression discovered while testing: www-data never got
read access to the TSIG key when it was tightened to 640 root:bind
(2026-07-19), so legitimate dyndns updates were silently failing
(REFUSED) independent of the vulnerability. Fixed with a POSIX ACL
scoped to just that key file rather than group membership, since the
bind group also owns rndc.key (full remote BIND control).

Verified end-to-end via the real PHP functions: injection payloads
rejected with no side effects, invalid IP rejected, legitimate update
succeeds and is visible in the zone, unauthenticated requests still 401.

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
+Nothing here is urgent; all are low-risk, no-downtime changes.
-HIGH PRIORITY**.
 
-- [ ] **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,
+      visitor. Compounding factors found during the audit: PHP is
-      `disable_functions` is empty (`exec` fully available); `index.php`'s
+      `7.4.33`, EOL since Nov 2022, `disable_functions` was empty (`exec`
-      `pass == '_NO_PASS_'` check is a hardcoded magic string, not real
+      fully available); `index.php`'s `pass == '_NO_PASS_'` check is a
-      per-user auth, so any valid dyndns credential can update *any* of
+      hardcoded magic string, not real per-user auth, so any valid dyndns
-      the zone's 5 hostnames (`jens`, `kack`, `test`, `uschi`, `vpn`), not
+      credential can update *any* of the zone's 5 hostnames (`jens`,
-      just its own; and the vhost's access logging turned out to be a
+      `kack`, `test`, `uschi`, `vpn`), not just its own — left as-is, out
-      no-op (shared `access.log` has 0 lines), so past exploitation can't
+      of scope for this fix; and the vhost's access logging turned out to
-      be fully ruled out from logs alone — though file mtimes, `www-data`
+      be a no-op (shared `access.log` has 0 lines), so past exploitation
-      processes/crontab, and current zone contents all look unremarkable,
+      couldn't be fully ruled out from logs alone — though file mtimes,
-      consistent with unexploited.
+      `www-data` processes/crontab, and zone contents all looked
-      Fix: switch `exec()` to pass the nsupdate command list via
+      unremarkable, consistent with unexploited.
-      `proc_open`/`popen` writing to stdin (no shell interpolation of
+      Fixed: all three files backed up to
-      user input at all), or at minimum wrap every interpolated value in
+      `/root/removed-configs-backup/dyndns-preinjectionfix-<timestamp>/`,
-      `escapeshellarg()`; add strict `filter_var`/regex validation on the
+      then rewritten to build the nsupdate command list as a plain string
-      hostname label and IP before use. Not yet applied.
+      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: