mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-20 18:09:05 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
f0efdafbdb |
@@ -37,10 +37,6 @@ 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)
|
|
||||||
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) || {
|
||||||
@@ -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 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
|
|
||||||
# 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
|
# 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
|
||||||
|
|||||||
@@ -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:
|
if not data:
|
||||||
return jsonify({'status': 'error', 'message': 'No data provided'}), 400
|
return jsonify({'status': 'error', 'message': 'No data provided'}), 400
|
||||||
|
|
||||||
import logging
|
# What arrives here is the config itself, and the headers carry the
|
||||||
logging.error(f"DEBUG: save_main_config received data: {data}")
|
# session cookie -- neither belongs in the journal, least of all at
|
||||||
logging.error(f"DEBUG: Content-Type header: {request.content_type}")
|
# ERROR on every save. The shape of the request is the part with
|
||||||
logging.error(f"DEBUG: Headers: {dict(request.headers)}")
|
# 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)
|
# Merge with existing config (similar to original implementation)
|
||||||
current_config = api_v3.config_manager.load_config()
|
current_config = api_v3.config_manager.load_config()
|
||||||
|
|||||||
Reference in New Issue
Block a user