mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-20 09:59:09 +00:00
Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 ChuckBuilds
|
cf521bdfd8 |
@@ -37,10 +37,6 @@ 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)
|
||||
MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir)
|
||||
|
||||
# Create a temporary sudoers file using mktemp (handles permissions better)
|
||||
TEMP_SUDOERS=$(mktemp) || {
|
||||
@@ -66,36 +62,6 @@ $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
|
||||
# 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
|
||||
|
||||
@@ -8,18 +8,6 @@ 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
|
||||
|
||||
@@ -1,120 +0,0 @@
|
||||
"""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 _normalised_grants():
|
||||
"""Grants with binary-path variables reduced to tool names.
|
||||
|
||||
Rules are written as `$SYSCTL_PATH -w ...`, 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.
|
||||
Only NOPASSWD lines are considered, because taking the whole script let a
|
||||
variable definition such as NFT_PATH=$(command -v nft) satisfy the check on
|
||||
its own while the grant itself had been deleted.
|
||||
"""
|
||||
text = "\n".join(_grant_lines())
|
||||
text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", lambda m: m.group(1).lower(), text)
|
||||
return re.sub(r"/usr/(?:s?bin)/", "", 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}"
|
||||
|
||||
|
||||
@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.
|
||||
"""
|
||||
pattern = r"\s+".join(re.escape(word) for word in command)
|
||||
assert re.search(pattern, _normalised_grants()), (
|
||||
f"no installer grants `{' '.join(command)}`")
|
||||
|
||||
|
||||
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
|
||||
haystack = rule.replace("_PATH", "").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))
|
||||
@@ -1,95 +0,0 @@
|
||||
"""The display unit must cap glibc's malloc arenas.
|
||||
|
||||
glibc hands each allocating thread its own malloc arena, up to 8 x CPU count,
|
||||
and an arena that has grown is never returned to the OS. This process runs
|
||||
threads for the render loop, the update workers and the background fetchers, so
|
||||
on a 3-core Pi the ceiling is 24 arenas.
|
||||
|
||||
Measured on a live rig, 2.5 hours in:
|
||||
|
||||
RSS 1030 MB
|
||||
Private_Dirty 988 MB
|
||||
anonymous mappings > 10 MB 23 (ceiling is 8 x 3 = 24)
|
||||
largest few 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses
|
||||
|
||||
against live data that accounts for perhaps 15 MB -- the widest scroll strip
|
||||
observed was 35,746 x 64, about 7 MB as RGB and the same again for its numpy
|
||||
mirror. Repeated sampling showed RSS flat between 990 and 1030 MB rather than
|
||||
climbing, so this is arena bloat rather than a leak: memory Python has freed
|
||||
but glibc is holding per-arena.
|
||||
|
||||
The device had 59 MB free at the time.
|
||||
|
||||
Capping the arena count trades a little allocator concurrency for that resident
|
||||
memory. The render loop is latency-sensitive, so if p99 frame time regresses the
|
||||
right response is to raise this rather than remove it.
|
||||
"""
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
UNIT = (Path(__file__).resolve().parent.parent / "systemd" / "ledmatrix.service")
|
||||
|
||||
#: The value the unit is expected to carry. 2 is the usual choice for a
|
||||
#: threaded Python process; 1-4 all keep some of the saving, but only one of
|
||||
#: them is what this project ships.
|
||||
EXPECTED_ARENA_MAX = 2
|
||||
|
||||
|
||||
def _environment(unit_text):
|
||||
return dict(
|
||||
line.split("=", 2)[1:3] if line.count("=") >= 2 else (line.split("=", 1)[1], "")
|
||||
for line in unit_text.splitlines()
|
||||
if line.startswith("Environment=")
|
||||
)
|
||||
|
||||
|
||||
def test_the_unit_exists():
|
||||
assert UNIT.is_file(), f"{UNIT} is missing"
|
||||
|
||||
|
||||
def test_malloc_arena_max_is_capped():
|
||||
env = _environment(UNIT.read_text(encoding="utf-8"))
|
||||
assert "MALLOC_ARENA_MAX" in env, (
|
||||
"the display unit does not cap glibc arenas; on a 3-core Pi the default "
|
||||
"ceiling is 24 and a measured rig held 23 of them, 920 MB"
|
||||
)
|
||||
value = int(env["MALLOC_ARENA_MAX"])
|
||||
# Pinned, not a range. A range let a change to 4 -- which hands most of the
|
||||
# saving back -- pass unnoticed, which was the point of the finding that
|
||||
# prompted this. Raising it is a legitimate response to a frame-time
|
||||
# regression, but it should be a visible edit here rather than a silent
|
||||
# drift, so the number lives in one place and changing it shows up in
|
||||
# review.
|
||||
assert value == EXPECTED_ARENA_MAX, (
|
||||
f"MALLOC_ARENA_MAX={value}, expected {EXPECTED_ARENA_MAX}. If this was "
|
||||
"raised deliberately because frame times regressed, update "
|
||||
"EXPECTED_ARENA_MAX here and say so in the commit."
|
||||
)
|
||||
|
||||
|
||||
def test_the_reason_is_recorded_next_to_it():
|
||||
"""A bare tuning knob invites removal by whoever meets it next."""
|
||||
text = UNIT.read_text(encoding="utf-8")
|
||||
index = text.index("Environment=MALLOC_ARENA_MAX")
|
||||
preamble = text[:index].splitlines()[-12:]
|
||||
comment = "\n".join(line for line in preamble if line.startswith("#"))
|
||||
assert "arena" in comment.lower(), "no explanation precedes the setting"
|
||||
assert re.search(r"\d", comment), (
|
||||
"the explanation cites no measurement, so a reader cannot tell whether "
|
||||
"it still applies to their hardware"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("unit", ["ledmatrix.service"])
|
||||
def test_the_unit_still_parses_as_ini(unit):
|
||||
"""systemd will refuse a malformed unit, and the panel stays dark."""
|
||||
import configparser
|
||||
|
||||
path = UNIT.parent / unit
|
||||
parser = configparser.ConfigParser(strict=False)
|
||||
# systemd allows repeated keys; ConfigParser needs them merged, not rejected.
|
||||
parser.read_string(path.read_text(encoding="utf-8"))
|
||||
assert parser.has_section("Service")
|
||||
assert parser.has_option("Service", "ExecStart")
|
||||
Reference in New Issue
Block a user