harden: cap malloc arenas, warn on unit drift, and grant the portal's sudo commands

scripts/install/configure_wifi_permissions.sh

@@ -25,9 +25,44 @@
     exit 1
 fi
 
+# Resolve command paths against a fixed PATH, and check what we resolved.
+#
+# Every path found here is written into a sudoers file as a NOPASSWD grant, so
+# whoever controls the binary at that path controls root. first_time_install.sh
+# re-execs itself with `sudo -E`, which preserves the invoking user's
+# environment -- PATH included -- so without pinning it, `which nmcli` can
+# resolve to anything on that PATH: a writable directory early in it turns a
+# compromise of the low-privilege web user into permanent root.
+PATH=/usr/sbin:/usr/bin:/sbin:/bin
+export PATH
+
+# A binary named in a sudoers rule must be root-owned and writable by nobody
+# else, or the grant hands root to whoever can rewrite it.
+require_trusted_binary() {
+    local label="$1" path="$2"
+    if [ ! -x "$path" ]; then
+        echo "✗ $label: $path is not an executable file"
+        exit 1
+    fi
+    local owner perms
+    owner=$(stat -c '%u' "$path") || exit 1
+    perms=$(stat -c '%a' "$path") || exit 1
+    if [ "$owner" != "0" ]; then
+        echo "✗ $label: $path is not owned by root (uid $owner); refusing to"
+        echo "  grant it NOPASSWD sudo."
+        exit 1
+    fi
+    # Group- or world-writable means someone other than root can replace it.
+    case "$perms" in
+        *[2367]) echo "✗ $label: $path is writable by group or other ($perms);"
+                 echo "  refusing to grant it NOPASSWD sudo."
+                 exit 1 ;;
+    esac
+}
+
 # Get the full paths to commands
-NMCLI_PATH=$(which nmcli || echo "/usr/bin/nmcli")
-SYSTEMCTL_PATH=$(which systemctl)
+NMCLI_PATH=$(command -v nmcli || echo "/usr/bin/nmcli")
+SYSTEMCTL_PATH=$(command -v systemctl)
 
 echo "Command paths:"
 echo "  nmcli: $NMCLI_PATH"
@@ -37,6 +72,18 @@
 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)
+MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir)
+
+# Checked before any of them reaches the sudoers file.
+require_trusted_binary "nmcli"     "$NMCLI_PATH"
+require_trusted_binary "systemctl" "$SYSTEMCTL_PATH"
+require_trusted_binary "sysctl"    "$SYSCTL_PATH"
+require_trusted_binary "nft"       "$NFT_PATH"
+require_trusted_binary "rfkill"    "$RFKILL_PATH"
+require_trusted_binary "mkdir"     "$MKDIR_PATH"
 
 # Create a temporary sudoers file using mktemp (handles permissions better)
 TEMP_SUDOERS=$(mktemp) || {
@@ -62,6 +109,36 @@
 $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
+# NetworkManager's dnsmasq drop-in directory, exact path.
+$WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d
+#
+# iptables is deliberately NOT granted here. Its rules are built from the live
+# interface name and port, so a rule covering them needs a trailing wildcard --
+# and `iptables --modprobe=/path/to/anything` runs that path as root, so
+# `NOPASSWD: iptables *` is a root shell for the web user by another name. That
+# is a worse outcome than the gap it would close, which today is masked anyway
+# by the blanket NOPASSWD rule on stock Pi images.
+#
+# Closing it safely means a wrapper script that builds the rules itself and
+# takes only an interface and a port, granted the way safe_plugin_rm.sh already
+# is. That belongs in its own change rather than being smuggled into this one.
 
 # Allow copying hostapd and dnsmasq config files into place
 $WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf

src/startup_validator.py

@@ -62,6 +62,9 @@ def validate_all(self) -> Tuple[bool, List[str], List[str]]:
         # Validate plugins if plugin manager is available
         if self.plugin_manager:
             self._validate_plugins()
+
+        # Warn when the running systemd unit no longer matches the repo's
+        self._validate_systemd_units()
 
         is_valid = len(self.errors) == 0
 
@@ -74,6 +77,80 @@ def validate_all(self) -> Tuple[bool, List[str], List[str]]:
 
         return (is_valid, self.errors.copy(), self.warnings.copy())
 
+    #: Units this project installs, and where each is installed to.
+    _UNITS = (
+        ("systemd/ledmatrix.service", "/etc/systemd/system/ledmatrix.service"),
+        ("systemd/ledmatrix-web.service", "/etc/systemd/system/ledmatrix-web.service"),
+    )
+
+    def _validate_systemd_units(self) -> None:
+        """Warn when an installed unit has drifted from the repo's template.
+
+        Nothing re-applies these after the first install. `git pull` -- which is
+        what the web UI's update button runs -- brings a new template into the
+        checkout, but nothing copies it to /etc/systemd/system and nothing runs
+        `systemctl daemon-reload`, so the unit that actually runs is whatever
+        first_time_install.sh wrote on day one.
+
+        That makes every hardening added to a unit inert on existing installs.
+        Measured on one rig: the installed unit was thirteen days older than the
+        repo's and differed in content, so a MemoryMax the repo had specified
+        was not being enforced at all -- `systemctl show` reported
+        MemoryMax=infinity.
+
+        A warning rather than an error, and certainly not a silent rewrite:
+        editing files under /etc and restarting services is the installer's job,
+        not something a display process should do to a machine while it boots.
+        The remedy is to re-run scripts/install/install_service.sh.
+        """
+        try:
+            project_root = Path(__file__).resolve().parent.parent
+            for template_rel, installed_path in self._UNITS:
+                template = project_root / template_rel
+                installed = Path(installed_path)
+                if not template.is_file() or not installed.is_file():
+                    continue
+
+                # The template carries placeholders the installer substitutes,
+                # so compare the substituted form rather than the raw file.
+                expected = template.read_text(encoding="utf-8")
+                expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root))
+                expected = expected.replace("__USER__", "root")
+
+                try:
+                    actual = installed.read_text(encoding="utf-8")
+                except PermissionError:
+                    continue
+
+                if self._unit_body(expected) != self._unit_body(actual):
+                    self.warnings.append(
+                        f"{installed.name} differs from {template_rel}; the "
+                        "installed unit is not refreshed by an update, so "
+                        "settings added to the template are not in effect. "
+                        "Re-run scripts/install/install_service.sh to apply them."
+                    )
+        except OSError as e:
+            self.logger.debug("Could not compare systemd units: %s", e)
+
+    @staticmethod
+    def _unit_body(text: str) -> str:
+        """A unit's meaningful lines, in order: no comments, no blanks.
+
+        Order is preserved deliberately. This used to sort, which made the
+        comparison insensitive to two changes that matter in a systemd unit:
+        repeated directives such as ExecStartPre= and ExecStartPost= run in
+        the order they appear, and a directive that moves between [Unit],
+        [Service] and [Install] means something different -- or nothing --
+        where it lands. A drift check that normalises those away reports no
+        drift for a unit that has genuinely changed.
+        """
+        lines = []
+        for line in text.splitlines():
+            line = line.strip()
+            if line and not line.startswith("#"):
+                lines.append(line)
+        return "\n".join(lines)
+
     def _validate_config(self) -> None:
         """Validate configuration files."""
         try:

systemd/ledmatrix.service

@@ -8,6 +8,18 @@ Type=simple
 User=root
 WorkingDirectory=__PROJECT_ROOT_DIR__
 Environment=PYTHONDONTWRITEBYTECODE=1
+# glibc gives each allocating thread its own malloc arena, up to 8 x CPU count,
+# and an arena that has grown is never handed back to the OS. This process runs
+# 9 threads on a 3-core Pi, so the ceiling is 24 arenas -- and a rig measured at
+# 1030 MB resident held 23 large anonymous mappings on 64 MB-aligned addresses,
+# 920 MB of them, while the live data it was actually holding (widest scroll
+# strip seen: 35,746 x 64) accounts for roughly 15 MB. That gap is arena bloat,
+# not leaked objects: RSS was flat across repeated sampling, not climbing.
+#
+# Capping the arenas trades a little allocator concurrency for a large amount of
+# resident memory on a device that has neither to spare. 2 is the usual value;
+# raise it if frame times regress.
+Environment=MALLOC_ARENA_MAX=2
 ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/run.py
 # Restart=always, not on-failure: run.py exiting 0 (a clean shutdown path taken
 # for a reason that no longer applies, e.g. a config reload) would otherwise leave

test/test_sudo_allowlist_covers_calls.py

@@ -0,0 +1,147 @@
+"""The captive portal's fixed-argument sudo calls must be granted.
+
+The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and
+ledmatrix_wifi. A sudo call absent from both needs a password, which a service
+cannot supply, so it fails.
+
+Four such calls were ungranted, all of them captive-portal teardown/setup:
+
+    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
+    mkdir -p .../dnsmasq-shared.d         wifi_manager.py:922
+
+It goes unnoticed because a stock Raspberry Pi image ships
+/etc/sudoers.d/010_pi-nopasswd granting the default user
+`ALL=(ALL) NOPASSWD: ALL`, which satisfies every gap in both files. It only
+bites once that blanket rule is removed or the service runs as another user.
+
+Scope, deliberately narrow: this pins the four commands above, each of which
+can be written out literally. The portal makes further sudo calls whose
+arguments are built at runtime -- iptables and nft rules carrying an interface
+name and a port, `ip addr`, `ip link` -- and those cannot be granted safely
+here. A rule covering them needs a trailing wildcard, and
+`iptables --modprobe=/path/to/anything` runs that path as root, so
+`NOPASSWD: iptables *` is a root shell for the web user by another name.
+Closing that half needs a privileged helper that builds the rules itself and
+takes only an interface and a port, granted the way safe_plugin_rm.sh already
+is. That is a design decision, not a one-line grant, and belongs in its own
+change.
+"""
+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",
+)
+
+#: Commands this change grants, each fully literal in the source.
+REQUIRED = (
+    ("sysctl", "-w", "net.ipv4.ip_forward=0"),
+    ("sysctl", "-w", "net.ipv4.ip_forward=1"),
+    ("nft", "add", "table", "ip", "ledmatrix"),
+    ("nft", "delete", "table", "ip", "ledmatrix"),
+    ("rfkill", "unblock", "wifi"),
+    ("mkdir", "-p", "/etc/NetworkManager/dnsmasq-shared.d"),
+)
+
+#: Tools with an option that executes a program of the caller's choosing.
+#: A trailing wildcard on any of these is a privilege escalation.
+EXEC_CAPABLE = ("iptables", "ip6tables", "nft", "tcpdump", "find", "awk",
+                "sed", "perl", "python", "python3", "env")
+
+
+def _grant_lines():
+    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])
+    return lines
+
+
+def _normalise(rule):
+    """One rule with binary-path variables reduced to bare tool names.
+
+    Rules are written as `$SYSCTL_PATH -w ...` or `${NFT_PATH} ...`, so
+    matching the literal "sysctl" finds nothing and every rule looks absent --
+    which is exactly how an earlier version of this test reported six gaps
+    that did not exist. Both spellings are handled: shell expands them
+    identically, and a check that understood only one silently skipped the
+    other.
+    """
+    rule = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?",
+                  lambda m: m.group(1).lower(), rule)
+    return re.sub(r"/usr/(?:s?bin)/", "", rule)
+
+
+def _granted_commands():
+    """The command each NOPASSWD rule actually grants, normalised.
+
+    _grant_lines() already returns everything after "NOPASSWD:", so what
+    arrives here is the command, possibly preceded by the NOEXEC tag and
+    possibly still carrying the closing quote of an `echo "..."` that wrote
+    it. Both are stripped so the result is comparable to a plain command.
+    """
+    commands = []
+    for rule in _grant_lines():
+        command = _normalise(rule).strip()
+        command = re.sub(r"^NOEXEC:\s*", "", command)
+        command = command.rstrip('"').rstrip("'").strip()
+        if command:
+            commands.append(" ".join(command.split()))
+    return commands
+
+
+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}"
+
+
+@pytest.mark.parametrize("command", REQUIRED, ids=lambda c: " ".join(c))
+def test_the_command_is_granted(command):
+    """Whole command, not just the binary.
+
+    Checking only the binary made this far weaker than it looked: with
+    `sysctl` present anywhere, deleting the ip_forward=0 grant still passed,
+    and the portal would then be unable to restore forwarding on teardown.
+    """
+    wanted = " ".join(command)
+    granted = _granted_commands()
+    # Exact match, not a prefix. A substring search was satisfied by
+    # `sysctl -w net.ipv4.ip_forward=0 *`, and that trailing wildcard lets the
+    # caller append whatever they like to a command running as root -- a far
+    # wider grant than the one this test is meant to be confirming.
+    assert wanted in granted, (
+        f"no installer grants exactly `{wanted}`; closest matches: "
+        + str([g for g in granted if g.startswith(command[0])])[:200])
+
+
+def test_no_wildcard_on_a_tool_that_can_exec():
+    """`NOPASSWD: iptables *` hands the web user root.
+
+    iptables --modprobe=/path runs that path as root. This caught a grant added
+    in this very change, which is why it is here.
+    """
+    offenders = []
+    for rule in _grant_lines():
+        rule = rule.strip()
+        if not rule.endswith("*"):
+            continue
+        # Normalised the same way as everything else: `${NFT_PATH} *` left a
+        # brace before the tool name, and the word-boundary check below does
+        # not treat "{" as a boundary, so that spelling slipped through.
+        haystack = _normalise(rule).lower()
+        for tool in EXEC_CAPABLE:
+            if re.search(rf"(^|/|\s|\$){tool}(\s|$)", haystack):
+                offenders.append(rule)
+                break
+    assert not offenders, (
+        "wildcard grant on a tool that can execute another program:\n  "
+        + "\n  ".join(offenders))