From 324a4e43b134f9e3b540e16f11916754636467f6 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 20 Aug 2026 01:22:57 -0400 Subject: [PATCH] fix(install): grant the sudo commands the captive portal actually runs The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and ledmatrix_wifi. Anything the code runs under sudo that is not in one of them needs a password, which a service cannot supply, so the call fails. Five commands were being run and none of them granted: sysctl -w net.ipv4.ip_forward=0|1 wifi_manager.py:788, 883 nft add|delete table ip ledmatrix wifi_manager.py:835, 895 rfkill unblock wifi wifi_manager.py:1811 iptables ... wifi_manager.py:796, 813, 818, 871 mkdir -p .../dnsmasq-shared.d wifi_manager.py:922 Together these are the captive portal: unblock the radio, bring up the AP, add the redirect, turn on forwarding, and undo all of it afterwards. Without the grants a hardened install would associate clients to the access point and then fail to route them. Why it has gone unnoticed: a stock Raspberry Pi image ships /etc/sudoers.d/010_pi-nopasswd granting the default user ALL=(ALL) NOPASSWD: ALL which satisfies every one of these regardless of what the allow-lists say. Confirmed on a live rig -- `sudo -n -l` permits sysctl there, and the blanket rule is why. The allow-lists are effectively decorative on a default image and only start mattering once that rule is removed or the service runs as another user. test_sudo_allowlist_covers_calls.py extracts every argv-style sudo call in src/ and web_interface/ and asserts an installer grants it, so the next command added without a rule fails here rather than on someone's hardened box. Getting that test honest took three passes, each worth recording: - Matching the literal "systemctl" against rules written as `$SYSTEMCTL_PATH enable ...` reported six gaps that did not exist. Binary path variables are now normalised before comparing. - Scanning the whole installer let `NFT_PATH=$(command -v nft)` -- a variable definition, not a grant -- satisfy the check on its own, so deleting the actual nft rules still passed. Only NOPASSWD lines are considered now. - `sudo -n ` reported "-n" as the binary. sudo's own flags are skipped. Each of the five grants is individually mutation-checked: removing any one fails the suite. (cherry picked from commit a372b43cd172d9821a19873ca7ac01aaa22aff30) --- scripts/install/configure_wifi_permissions.sh | 27 ++++ test/test_sudo_allowlist_covers_calls.py | 134 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 test/test_sudo_allowlist_covers_calls.py diff --git a/scripts/install/configure_wifi_permissions.sh b/scripts/install/configure_wifi_permissions.sh index cfba4bc4..3da68439 100755 --- a/scripts/install/configure_wifi_permissions.sh +++ b/scripts/install/configure_wifi_permissions.sh @@ -37,6 +37,11 @@ echo " systemctl: $SYSTEMCTL_PATH" echo "" echo "Step 1: Configuring sudo permissions for nmcli..." SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi" +SYSCTL_PATH=$(command -v sysctl || echo /usr/sbin/sysctl) +NFT_PATH=$(command -v nft || echo /usr/sbin/nft) +RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill) +IPTABLES_PATH=$(command -v iptables || echo /usr/sbin/iptables) +MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir) # Create a temporary sudoers file using mktemp (handles permissions better) TEMP_SUDOERS=$(mktemp) || { @@ -62,6 +67,28 @@ $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH start dnsmasq $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop dnsmasq $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager +# The captive portal turns IP forwarding on while the access point is up and +# restores the previous value when it comes down (wifi_manager._setup_iptables_ +# redirect / _teardown_iptables_redirect). Without this rule that sudo call +# needs a password, so forwarding stays off and clients associate to the AP but +# cannot route. It goes unnoticed on a stock Raspberry Pi image, where +# /etc/sudoers.d/010_pi-nopasswd grants the default user blanket NOPASSWD and +# masks every gap in this file -- it only bites once that blanket rule is +# removed. +$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=0 +$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=1 +# The portal's redirect lives in its own nftables table, created when the AP +# comes up and deleted when it goes down, and the radio has to be unblocked +# before the AP can start at all. Same story as the sysctl rules above: called +# with sudo, never granted here, and invisible on a stock Pi image. +$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH add table ip ledmatrix +$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH delete table ip ledmatrix +$WEB_USER ALL=(ALL) NOPASSWD: $RFKILL_PATH unblock wifi +# The portal also inserts and removes its own iptables rules and creates +# NetworkManager's dnsmasq drop-in directory. Wildcards rather than exact +# argument lists: those rules are built from the live interface name and port. +$WEB_USER ALL=(ALL) NOPASSWD: $IPTABLES_PATH * +$WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d # Allow copying hostapd and dnsmasq config files into place $WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf diff --git a/test/test_sudo_allowlist_covers_calls.py b/test/test_sudo_allowlist_covers_calls.py new file mode 100644 index 00000000..8105f7fd --- /dev/null +++ b/test/test_sudo_allowlist_covers_calls.py @@ -0,0 +1,134 @@ +"""Every sudo the code runs must be granted by an installer allow-list. + +The installers write two files -- /etc/sudoers.d/ledmatrix_web and +/etc/sudoers.d/ledmatrix_wifi -- each an explicit allow-list. Anything the code +calls with sudo that is not in one of them needs a password, which a service +cannot supply, so the call fails. + +That failure is invisible on a stock Raspberry Pi image, because +/etc/sudoers.d/010_pi-nopasswd grants the default user + + ALL=(ALL) NOPASSWD: ALL + +which masks every gap in both files. It only surfaces on a system where that +blanket rule has been removed, or where the service runs as a different user -- +so a missing entry can sit there for a long time before anyone hits it. + +One was: wifi_manager turns IP forwarding on while the captive portal's access +point is up and restores it afterwards, calling `sudo sysctl -w +net.ipv4.ip_forward=...`. Neither allow-list granted sysctl. On such a system +clients would associate to the AP and then fail to route. +""" +import re +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +INSTALLERS = ( + ROOT / "first_time_install.sh", + ROOT / "scripts" / "install" / "configure_wifi_permissions.sh", +) +SOURCES = (ROOT / "src", ROOT / "web_interface") + +# argv-style sudo calls: ["sudo", , "arg", ...] +CALL = re.compile(r"""\[\s*["']sudo["']\s*,\s*(?P[^\]]+)\]""") + + +def _first_two_args(rest): + """The binary and its first argument, as written.""" + parts = [p.strip() for p in rest.split(",")] + # Drop sudo's own flags: `sudo -n ` is a call to , and + # treating "-n" as the binary reports a gap that does not exist. + flag = re.compile(r'[\'"]-[a-zA-Z]+[\'"]') + while parts and flag.fullmatch(parts[0]): + parts = parts[1:] + # Always two entries: `["sudo", "reboot"]` has no subcommand, and the + # caller unpacks a fixed pair. + out = [] + for part in (parts + ["", ""])[:2]: + if not part: + out.append("") + continue + literal = re.fullmatch(r"""["'](.+)["']""", part) + if literal: + out.append(literal.group(1)) + else: + # A variable such as sysctl_bin: reduce to the tool it resolves to. + out.append(part.split(".")[-1].replace("_bin", "").replace("_path", "")) + return out + + +def _sudo_calls(): + found = {} + for base in SOURCES: + for path in base.rglob("*.py"): + text = path.read_text(encoding="utf-8", errors="replace") + for match in CALL.finditer(text): + args = _first_two_args(match.group("rest")) + if not args: + continue + line = text[:match.start()].count("\n") + 1 + found.setdefault(tuple(args), f"{path.relative_to(ROOT)}:{line}") + return found + + +def _allowlisted_text(): + """The allow-list rules, with binary-path variables reduced to tool names. + + The installers write rules like `$SYSTEMCTL_PATH enable ledmatrix.service`, + so matching on the literal "systemctl" finds nothing and every systemctl + rule looks absent. Normalise $FOO_PATH and /usr/bin/foo down to foo before + comparing, or the check reports gaps that are not there -- which it did on + the first run. + """ + # NOPASSWD lines only. Taking the whole script would let a variable + # definition such as NFT_PATH=$(command -v nft) satisfy the check on its + # own, which is how an earlier version of this test passed while the grant + # itself had been deleted. + lines = [] + for installer in INSTALLERS: + if not installer.is_file(): + continue + for line in installer.read_text(encoding="utf-8", errors="replace").splitlines(): + if "NOPASSWD:" in line: + lines.append(line.split("NOPASSWD:", 1)[1]) + text = "\n".join(lines) + text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", + lambda m: m.group(1).lower(), text) + text = re.sub(r"/usr/(?:s?bin)/", "", text) + return text + + +def test_the_installers_are_present(): + missing = [str(p.relative_to(ROOT)) for p in INSTALLERS if not p.is_file()] + assert not missing, f"installer(s) missing: {missing}" + + +def test_every_sudo_call_is_granted(): + allow = _allowlisted_text() + calls = _sudo_calls() + assert calls, "no sudo calls found; the matcher has stopped working" + + ungranted = [] + for (binary, first_arg), where in sorted(calls.items()): + # A rule mentions the tool and, where it takes a subcommand, that too. + if binary not in allow: + ungranted.append(f"{binary} ({where})") + continue + if first_arg and not first_arg.startswith("-"): + pattern = rf"{re.escape(binary)}\s+{re.escape(first_arg)}" + if binary in ("systemctl",) and not re.search(pattern, allow): + ungranted.append(f"{binary} {first_arg} ({where})") + + assert not ungranted, ( + "these run under sudo but no installer grants them; on a stock Pi the " + "blanket 010_pi-nopasswd rule hides this:\n " + "\n ".join(ungranted)) + + +@pytest.mark.parametrize("needle", ["ip_forward"]) +def test_the_captive_portal_forwarding_rule_is_granted(needle): + """The specific gap this test was written for.""" + assert needle in _allowlisted_text(), ( + "no allow-list entry for sysctl ip_forward; the captive portal enables " + "forwarding while its AP is up and cannot without one")