mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-20 09:59:09 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
f0efdafbdb |
@@ -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
|
||||
|
||||
@@ -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))
|
||||
@@ -715,10 +715,12 @@ def save_main_config():
|
||||
if not data:
|
||||
return jsonify({'status': 'error', 'message': 'No data provided'}), 400
|
||||
|
||||
import logging
|
||||
logging.error(f"DEBUG: save_main_config received data: {data}")
|
||||
logging.error(f"DEBUG: Content-Type header: {request.content_type}")
|
||||
logging.error(f"DEBUG: Headers: {dict(request.headers)}")
|
||||
# What arrives here is the config itself, and the headers carry the
|
||||
# session cookie -- neither belongs in the journal, least of all at
|
||||
# ERROR on every save. The shape of the request is the part with
|
||||
# diagnostic value, so log that, at the level it deserves.
|
||||
logger.debug("save_main_config: %s, %d top-level key(s)",
|
||||
request.content_type or 'no content-type', len(data))
|
||||
|
||||
# Merge with existing config (similar to original implementation)
|
||||
current_config = api_v3.config_manager.load_config()
|
||||
|
||||
Reference in New Issue
Block a user