mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-20 18:09:05 +00:00
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
<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 <tool>` 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.
This commit is contained in:
@@ -37,6 +37,11 @@ echo " systemctl: $SYSTEMCTL_PATH"
|
|||||||
echo ""
|
echo ""
|
||||||
echo "Step 1: Configuring sudo permissions for nmcli..."
|
echo "Step 1: Configuring sudo permissions for nmcli..."
|
||||||
SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi"
|
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)
|
# Create a temporary sudoers file using mktemp (handles permissions better)
|
||||||
TEMP_SUDOERS=$(mktemp) || {
|
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 stop dnsmasq
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq
|
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager
|
$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
|
# Allow copying hostapd and dnsmasq config files into place
|
||||||
$WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf
|
$WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf
|
||||||
|
|||||||
@@ -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
|
||||||
|
|
||||||
|
<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", <binary-or-var>, "arg", ...]
|
||||||
|
CALL = re.compile(r"""\[\s*["']sudo["']\s*,\s*(?P<rest>[^\]]+)\]""")
|
||||||
|
|
||||||
|
|
||||||
|
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 <tool>` is a call to <tool>, 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")
|
||||||
Reference in New Issue
Block a user