fix(install): tag the secondary installer's journalctl grants too (#491)

Review was right on all three counts, and the first is the one that matters:
scripts/install/configure_web_sudo.sh writes the same three wildcard
journalctl rules as first_time_install.sh and none of them carried NOEXEC. So
this PR closed the pager escape on one installer path and left it open on the
other, which is close to no fix at all -- a rig configured through that script
still hands out a root shell via less's "!command".

The test could not have caught it, for two independent reasons. INSTALLERS
did not list the file. And even listed, _grant_lines() kept the raw source
line: that installer echoes its rules, so each one ends in a quote rather
than the wildcard, and the trailing-* check skipped every one of them. Either
alone would have hidden it.

Both fixed: the file is covered, and an echoed rule is unwrapped to the
sudoers line it actually emits.

The selector test now covers -t ledmatrix as well. It asserted only the two
-u forms, so deleting the -t rule would have passed.

Verified by removing NOEXEC again from the secondary installer: four of the
six tests fail, where before the suite passed with the vulnerability present.


Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-08-21 14:28:23 -04:00
committed by GitHub
co-authored by Claude Opus 5
parent fe5a3aa99d
commit cc258aaffd
2 changed files with 26 additions and 9 deletions
+8 -3
View File
@@ -100,10 +100,15 @@ TEMP_SUDOERS="/tmp/ledmatrix_web_sudoers_$$"
echo "$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart ledmatrix-web.service" echo "$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart ledmatrix-web.service"
# Optional: journalctl (non-critical — skip if not found) # Optional: journalctl (non-critical — skip if not found)
#
# NOEXEC, matching first_time_install.sh. These rules end in a wildcard and
# journalctl starts a pager, so without it the caller can reach a shell:
# less runs "!command" as the user the pager belongs to, which here is
# root. NOEXEC stops the granted command executing anything of its own.
if [ -n "$JOURNALCTL_PATH" ]; then if [ -n "$JOURNALCTL_PATH" ]; then
echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -u ledmatrix.service *" echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -u ledmatrix.service *"
echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -u ledmatrix *" echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -u ledmatrix *"
echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -t ledmatrix *" echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -t ledmatrix *"
fi fi
# Required: python3, bash # Required: python3, bash
+18 -6
View File
@@ -30,6 +30,10 @@ ROOT = Path(__file__).resolve().parent.parent
INSTALLERS = ( INSTALLERS = (
ROOT / "first_time_install.sh", ROOT / "first_time_install.sh",
ROOT / "scripts" / "install" / "configure_wifi_permissions.sh", ROOT / "scripts" / "install" / "configure_wifi_permissions.sh",
# Writes the same journalctl grants as first_time_install.sh. It was
# missing here, and because of that this suite passed while three
# ungranted wildcard rules sat in it.
ROOT / "scripts" / "install" / "configure_web_sudo.sh",
) )
#: Commands that will start another program of their own accord -- a pager, an #: Commands that will start another program of their own accord -- a pager, an
@@ -44,8 +48,14 @@ def _grant_lines():
continue continue
for line in installer.read_text(encoding="utf-8", errors="replace").splitlines(): for line in installer.read_text(encoding="utf-8", errors="replace").splitlines():
stripped = line.strip() stripped = line.strip()
if "NOPASSWD" in stripped and not stripped.startswith("#"): if "NOPASSWD" not in stripped or stripped.startswith("#"):
lines.append(stripped) continue
# Installers emit rules two ways: written literally into a heredoc,
# or echoed into a file. An echoed rule ends in a quote, so the
# trailing-wildcard check below would skip it and the rule would
# never be examined at all.
echoed = re.fullmatch(r"""echo\s+(['"])(.*)\1""", stripped)
lines.append(echoed.group(2) if echoed else stripped)
return lines return lines
@@ -78,10 +88,12 @@ def test_journalctl_is_granted_at_all():
"no journalctl grant remains; the web interface reads logs through it") "no journalctl grant remains; the web interface reads logs through it")
@pytest.mark.parametrize("unit", ["ledmatrix.service", "ledmatrix"]) @pytest.mark.parametrize("selector", ["-u ledmatrix.service", "-u ledmatrix",
def test_each_journalctl_rule_is_tagged(unit): "-t ledmatrix"])
def test_each_journalctl_rule_is_tagged(selector):
"""Every selector, so removing one cannot pass by the others' presence."""
matching = [r for r in _grant_lines() matching = [r for r in _grant_lines()
if "JOURNALCTL_PATH" in r and f"-u {unit} " in r] if "JOURNALCTL_PATH" in r and f"{selector} " in r]
assert matching, f"no journalctl rule for -u {unit}" assert matching, f"no journalctl rule for {selector}"
untagged = [r for r in matching if "NOEXEC" not in r] untagged = [r for r in matching if "NOEXEC" not in r]
assert not untagged, f"untagged journalctl rule(s): {untagged}" assert not untagged, f"untagged journalctl rule(s): {untagged}"