Security Hardening — Pre-Auth, Escape, SSRF, Path-Traversal, CSRF#144
Open
wowa1990 wants to merge 18 commits into
Open
Security Hardening — Pre-Auth, Escape, SSRF, Path-Traversal, CSRF#144wowa1990 wants to merge 18 commits into
wowa1990 wants to merge 18 commits into
Conversation
…jection (HIGH-1)
deleteLocal() spliced a frontend-supplied `deleteFile` into a shell
`rm -r "…"` command via two decodeURI passes, then ran it through
exec(). Any %-encoded shell metachar in the path round-tripped back
to its literal form before the shell saw it, so a WebSocket call
with `deleteFile="foo%22%3B+touch+%2Ftmp%2FPWN%3B+%22"` would decode
to `foo"; touch /tmp/PWN; "` — the closing `"` ended the quoted arg,
then the trailing command ran as the dietpi user. Auth-protected
(frontend-only) but worth fixing on defence-in-depth grounds; the
same surface picks up RSS-fed values via the resume-list path,
which Phase 4's MED-15 escape mitigated but didn't eliminate.
Three layers:
1. Single decodeURIComponent in a try/catch (rejects malformed
%FF-style sequences). The previous decodeURI-then-decode chain
could re-introduce escapes round-trip-style.
2. path.resolve under a fixed MEDIA_ROOT, then a startsWith check
to refuse any input that resolved outside the root. `..`
segments and absolute paths fold to a path that fails the
prefix test and gets rejected.
3. fs.existsSync gate so we don't shell out to a non-existent
target — refuses silently instead of running rm with surprising
args.
4. execFile instead of exec — `rm` and the path are passed as
separate argv entries, no shell involved, no quoting needed,
no metachar to escape.
…t.sh
\$1 (BT-MAC from the admin bluetooth.php POST) was piped into bluetoothctl
without validation; a value like "AA:BB:CC:DD:EE:FF\nremove all\n" injects
arbitrary bluetoothctl commands. The scripts run via sudo from the backend,
so this was effectively a privilege-escalation vector. Add a strict regex
check at the top of both scripts.
Drive-by typo fixes while in here:
- "defaut-agent" -> "default-agent" (bluetoothctl was silently ignoring
the agent registration command, so pairing could hang on
"no agent registered").
- "ouput=" -> "output=" (the captured bluetoothctl output was being
written to a different variable than the subsequent echo read,
so the frontend was always getting an empty response).
…nd auth
These six endpoints did not include any authentication, so the entire
login gate was bypassed for any caller in the LAN. Backup and fullbackup
expose the bcrypt password hash from interfacelogin plus every
Spotify/Telegram credential in mupiboxconfig.json, debug.php streams
chrome_debug.log (OAuth redirect URLs with auth codes, Authorization
headers), pm2logs ships stack traces with embedded tokens,
support_data.zip bundles all of the above, and backend.php streams pm2
log contents directly via fetch.
Add a new includes/auth_check.php — a header-only auth gate that
mirrors header.php's session check verbatim but emits zero output
(http_response_code(401) + plain text body and exit() on unauth).
header.php itself is the wrong include for these endpoints because it
renders the entire admin page chrome (<!DOCTYPE html>, head, body,
top-nav) before the calling page gets a chance to call
header('Content-Type: application/octet-stream') — PHP refuses headers
after output, so the browser would see text/html with the admin page
concatenated to the binary zip.
Add `require __DIR__ . '/includes/auth_check.php';` as the first line
of each endpoint, and downgrade the chmod 777 on the generated zip
artifacts to chmod 600 — the chown www-data already covers the access
path PHP needs, the world-readable mode was strictly extra exposure.
…auth Four privileged GET handlers (hshutdown, hreboot, hchromerestart, hrefreshdatabase) lived in header.php above the login gate, so any unauthenticated LAN visitor could curl `?hshutdown=1` to take the box offline, `?hreboot=1` to reboot it, or `?hrefreshdatabase=1` to grind the SD card by re-running m3u_generator.sh. The HTML buttons that fire these are themselves rendered after the gate, but the server-side handlers ran unconditionally on every page load — a curl was enough. Wrap the four `if ($_GET[...])` blocks in `$authGatePassed = !$loginEnabled || (logged_in === true)`, mirroring the same logic the auth gate uses further down the file. While here, switch from truthiness `$_GET[x]` to `isset($_GET[x])` so PHP 8 doesn't emit warnings about undefined keys.
…l injection
The submitfile handler in admin.php sat above the auth gate (header.php
is included on line 78, the handler is on lines 16-74), so every step
of the restore flow ran for any unauthenticated LAN caller:
- move_uploaded_file into /tmp/<user-controlled-name>
- sudo unzip -o -a '/tmp/<name>' -d / >> /tmp/restore.log
- sudo /boot/dietpi/func/change_hostname <host-from-zip>
- sudo rm '/tmp/<name>'
That is full pre-auth root file write via ZIP-slip (zip can stash
entries under ../../etc/cron.d/, /home/dietpi/.ssh/, anything), plus
shell injection via the uploaded filename single-quoted into the unzip
/ rm commands.
Three layers of defence:
1. Narrow auth gate at the top of admin.php scoped specifically to
submitfile POSTs (`!empty($_POST['submitfile'])`). Crucial: do NOT
block all POSTs — header.php's login POST (password=...) must
reach its own auth handler, which lives BELOW the include.
2. Filename whitelist regex `/^[A-Za-z0-9._-]+\.zip$/` plus basename();
all real backups pass, anything with quotes / semicolons / spaces
/ path separators is rejected before move_uploaded_file ever runs.
3. Pre-extraction ZIP-slip check via ZipArchive: iterate every entry,
reject the upload unless every path falls under one of three known
prefixes (home/dietpi/MuPiBox/media/, etc/mupibox/mupiboxconfig.json,
home/dietpi/.mupibox/...data.json). Anything containing `..` is
rejected.
Also escapeshellarg() the target_file in the unzip + rm commands and
the host value in the subsequent change_hostname call — the host
string comes from the just-restored config, so a malicious backup that
survived the ZIP whitelist could still inject there.
…ge write
Three issues with the JSON editor's POST handler:
1. No CSRF protection. Any other tab the admin has open could POST a
crafted mupiboxconfig.json to /jsoneditor.php?file=mupiboxconfig and
either disable interfacelogin (`state: false`) or replace the bcrypt
hash with one the attacker knows. Header.php already starts the
session, so add a `csrf_token` minted via random_bytes(32), embed it
as a hidden input, and validate with hash_equals() on POST.
2. interfacelogin not pinned. Even with auth, the editor lets a
logged-in user edit the auth block — a single typo while editing
other settings can lock the admin out, or silently re-open the box.
Authentication is the dedicated login.php's job. When editing
mupiboxconfig.json, read the on-disk `interfacelogin` and overwrite
whatever was submitted with it.
3. Non-atomic write. `file_put_contents($file, ...)` overwrites
in-place; if the box loses power mid-write the JSON is truncated and
on next boot mupibox cannot start. Use the same two-stage approach
admin.php's write_json() already uses:
- file_put_contents to /tmp/.jsoneditor.<rand>.json (always
writable for www-data because /tmp is world-writable)
- sudo install -m <mode> -o <orig-owner> -g <orig-group> /tmp/<f>
<target>, preserving owner and perms
www-data cannot write into the actual config dirs (e.g.
/home/dietpi/.mupibox/.../config/ is dietpi:dietpi 755), so the
simpler same-dir tempfile + rename() approach fails outright.
install does a copy + close — not strictly atomic across filesystems,
but matches the existing admin.php pattern and is the only thing
that works given the permission model.
The receiver previously responded to commands from any chat that messaged the bot. Anyone who discovered the bot username (e.g. via Telegram search, by being in a shared group, or by guessing) could send /shutdown, /reboot, /vol, /sleep, /screen — with the bot token as the *only* barrier. That is too thin a defense for a parent's network-exposed device. Adds an is_authorized() check that compares the inbound chat_id / user_id against the chatId configured in mupiboxconfig.json. An empty chatId is treated as "deny all" — the user has to configure one anyway for outbound notifications, so requiring it here costs nothing. Applies to both on_chat_message and on_callback_query (the inline keyboard) so neither path is privileged. This fix can be backported to any existing MuPiBox installation that runs telegram_receiver.py — no other changes required, no schema changes, no service config changes.
…ED-16)
The three admin-page POST handlers most worth CSRF-protecting are
the system-mutating ones: service.php (apt-remove packages, stop
systemd units), tweaks.php (writes /boot/config.txt + /boot/dietpi.txt
+ swap), and mupihat.php (toggles HAT triggering a reboot, sets
battery shutdown thresholds). All three were unprotected — an evil
page the admin happened to visit could submit a hidden form there
from the admin's authenticated browser and silently flip switches.
CSRF helpers in a new includes/csrf.php (kept separate from header.php
because header.php emits HTML chrome — by the time a handler at the
top of e.g. service.php tried to call csrf_check() against helpers
defined in header.php, the response had already streamed several kB
of HTML and http_response_code(403) silently became HTTP 200 with
"headers already sent" warnings). Three helpers:
csrf_token() — lazy-mints a per-session 32-byte hex token, stored
in $_SESSION['csrf_token'] (same key jsoneditor.php's
CRIT-6 fix already uses)
csrf_field() — renders the hidden <input name="csrf_token">
csrf_check() — on POST, validates via hash_equals and 403 + plain-
text body + exit() on mismatch
Calling sites pull csrf.php in BEFORE include('header.php'), call
csrf_check() at the very top, then continue with the normal page
flow. csrf_field() goes after each <form> opening tag.
Also pulled into the same commit:
- service.php's `$rc = $output[count($output)-1]` was a pre-existing
PHP 7-era line that became a TypeError in PHP 8 (count(null) no
longer returns 0). On any GET the whole page fataled before the
form rendered, which blocked verification of the new CSRF gate.
Guard with `!empty($output) ? … : ''`.
- header.php require_once's csrf.php so csrf_field() also works in
template positions further down the body.
Verified: POST without csrf_token returns HTTP 403 + "CSRF token
mismatch" body; POST with a valid token from the same session
proceeds normally. service.php now renders its full form on GET.
…hattable (MED-17)
Four polling endpoints that the admin UI's status icons hit every
few seconds were entirely unauth. From the LAN anyone could:
- read the box's current Wi-Fi SSID and signal strength
(update_wifiicon.php — useful for fingerprinting which AP)
- read battery SOC + bus voltage + charge state
(update_batteryicon.php — infer when the box is in use)
- read fan speed + CPU/IC temperatures
(update_fanicon.php — same)
- dump the full /tmp/mupihat.json
(update_mupihattable.php)
update_wifiicon.php additionally fired `sudo iwgetid -r` and
`sudo iwconfig wlan0` per request — every browser tab on the
LAN polling the same endpoint produced a steady stream of sudo
forks under the dietpi user, effectively a DoS amplifier.
Add `require __DIR__ . '/includes/auth_check.php';` (the header-only
gate established in CRIT-2/3/5) at the top of all four files. After
this, only authenticated admin sessions can poll, and the sudo-fork
rate is bounded by however many admin tabs are open rather than
how many devices are on the LAN.
…cover, network, mupi Five separate PHP endpoints each spliced caller-controlled values into shell exec() commands without escaping. All five sit BEHIND the auth gate (header.php is included), so an unauthenticated LAN visitor can't reach them — but a logged-in admin (or anyone who slipped a CSRF past the editor) gets RCE / file-deletion / config-tampering on top of whatever the original handler does. Defense in depth. spotify.php:21 (HIGH-2): the OAuth callback handler interpolates $_GET['code'] (echoed back from Spotify's redirect) plus four other values into a curl command line. escapeshellarg() each. bluetooth.php:29,42 (HIGH-3): pair_bt.sh / remove_bt.sh receive a MAC from $_POST. Even though the scripts now validate MAC format themselves (CRIT-7), the shell command line is built here and executes BEFORE pair_bt.sh runs — backticks would expand at the shell layer. Strict MAC regex check + escapeshellarg(). Reject path sets $change=1 so the footer renders the error toast (otherwise the user clicks "remove pairing" with a broken MAC and sees nothing even though server-side the reject is working). cover.php:6-7 (HIGH-5): the deleteimage handler ran `sudo rm /var/www/cover/$image` as root with $image straight from POST — `image=../../etc/mupibox/mupiboxconfig.json` would happily wipe the box's main config. basename() to strip path traversal, filename whitelist, escapeshellarg(). Reject path also sets $change=1 for visibility. network.php:178 (HIGH-6): wpa_cli wifinr is always a small integer. intval() collapses anything non-numeric to 0; the previous code let `wifinr=0; rm -rf /` pass through. -1 sentinel for invalid. mupi.php:31,37,43,167 (HIGH-4): three display-rotation handlers and the sleep-timer handler all spliced numeric POST values into shells. intval() each, plus a whitelist of valid rotation values (0/1/2/3/90/180/270) and a 24h cap on the sleep timer to make obviously-wrong inputs fail loud.
…S (MED-15 / LOW-2) Three PHP pages echoed admin-controlled fields directly into HTML without escaping. With XSS-laundering paths now closing in the audit (CSRF being addressed in a follow-up), the remaining vector was "admin writes a hostile value once → it persists in data.json / mupiboxconfig.json → every admin session re-renders it". RSS feed ingestion is the most realistic injection path: a podcast feed's title is fully attacker-controlled and lands in data.json via /api/add when the user adds an RSS subscription. MED-15 (media.php). The "Media database contents" page rendered every field of every entry — index, type, category, artist, title, id, query, cover, artistcover, sorting, etc. — straight into table cells. Wrap each `print` in a local `$h()` helper that calls htmlspecialchars(ENT_QUOTES). Anchors get a `$safeHref()` helper that also enforces an http(s) scheme: a `cover` of `javascript:alert(1)` would otherwise still produce a clickable script-URL even after text-escaping. Non-http URLs collapse to '#'. LOW-2 (vnc.php / content.php). Same pattern — `$data["mupibox"] ["host"]` was concatenated into <embed src='...' host='...'> and <a href='...' />. Could land an XSS via mupiboxconfig.json edits. htmlspecialchars on the host before splicing.
The endpoint took a user-supplied URL via ?url= and ky-fetched it
server-side, then echoed the body back. With no schema check and no
host filtering this was textbook SSRF — a caller (the frontend, or
anyone slipping past the auth gate) could pivot the box into reaching
arbitrary services routable from the box's network: the home router's
admin UI, the user's NAS, other boxes' admin pages, cloud-metadata
endpoints if the box were ever in a cloud, etc.
The endpoint sits behind auth, but treating "authenticated frontend"
as fully trusted couples the LAN-pivot risk to any XSS / admin-CSRF
that ever lands. Defense in depth:
1. Schema allowlist (http / https only). Drops file:, ftp:, gopher:,
data:, etc. that ky would otherwise dutifully fetch.
2. Host allowlist via blocklist of private ranges: 127/8, 10/8,
192.168/16, 172.16/12, 169.254/16, 0/8, IPv6 ::1, fe80::/10,
fc00::/7, plus the literal "localhost" / "0.0.0.0" / "::".
Doesn't do DNS resolution (would be slow + DNS-rebinding-able)
but DOES catch raw IP literals that bypass DNS.
3. 10-second timeout via ky's timeout option.
4. 5 MB response cap — RSS feeds are text and small, anything
bigger is either misconfigured or hostile.
Type-checks pass.
…(MED-5)
The cache layer constructed file paths as
`path.join(cacheDir, ${cacheKey}.json)` where cacheKey was a string
template literal containing user-controlled fields — search queries,
playlist IDs, album IDs, etc. A search for `../../etc/passwd_x` would
produce a cacheKey of `search_albums_../../etc/passwd_x_10_0`, and
path.join would resolve the `../` segments and let fs.writeFile escape
the cache directory. Even within the dietpi user's reach that's a
straight write-anywhere primitive — `/home/dietpi/.ssh/authorized_keys
.json`, `/home/dietpi/.bashrc.json`, `/tmp/$RANDOM/spotify-api/...`,
etc. The data written is just the Spotify API JSON, but the FILENAME
is the attack surface (overwrite a file with controlled content via
controlled name).
Hash the cacheKey with SHA-256 before joining. The on-disk filename is
now always 64 hex chars + ".json" — filesystem-safe, no `.`, no `/`,
nothing path.join can fold into ../. The expiry classifier
getCacheExpiryForKey() still operates on the unhashed key (it only
inspects the prefix), so cache lifetimes are unchanged.
Trade-off: existing cache files (with their old plaintext names)
become orphans after this change — they'll just sit in
cache/spotify-api/ until manually cleaned. Acceptable; cache invalidates
naturally over hours-to-days anyway and doesn't affect correctness.
Type-checks pass.
src/backend-player/package.json had two phantom dependencies:
- "http": "0.0.1-security" — the npm "security holding" placeholder
package, content is literally "this name has been temporarily
reserved by npm". Zero functional value.
- "path": "^0.12.7" — a Joyent-era userland polyfill of the node:path
module. Brings its own transitive deps (process, util) for behaviour
Node has supported natively since 0.10.
Both source files use the explicit `node:`-prefixed builtins
(`require('node:http')`, `require('node:path')`), so Node was already
ignoring the userland packages. Removing them deletes ~6 transitive
deps from the install tree without any source change.
No package-lock.json in this workspace, so no lockfile churn.
Four small post-auth hardening patches that share the same scope —
admin-only PHP endpoints with thin or missing input handling.
M10 cover.php — upload filename whitelist + output escape:
basename() on the upload branch strips path components but happily
passes "<", quotes, "&", spaces and unicode lookalikes. Those names
land in the file listing below and (without htmlspecialchars there)
inject into the <img> tag for every admin who reopens the page.
Same /^[A-Za-z0-9._-]+\.(jpe?g|png|gif|webp)$/i regex the
deleteimage branch already enforces. Plus defence-in-depth:
htmlspecialchars every basename + the host into HTML attributes,
rawurlencode into URL components, so legacy files from before this
patch can't break out of the markup either.
M11 mupi.php — fopen resource check at end of file:
fgets(fopen("/tmp/.time2sleep", 'r')) raised an uncaught TypeError
on PHP 8+ whenever the sleep timer file didn't exist (i.e. every
page load without an active sleep timer). Guard with
if ($fh = @fopen(...)) { fgets / fclose } and default to empty
string; the JS side already handles that.
AR5-15 bluetooth.php — CSRF guard + escapeshellarg for paired-device shell:
bluetooth.php was missed by the Phase-5 CSRF sweep. Every POST
handler runs `sudo systemctl` or `sudo /usr/local/bin/mupibox/
*_bt.sh` — a cross-site request from another tab could toggle BT
state, pair an attacker MAC, or remove a paired device. Gate all
writes behind csrf_check() at the top of the file before any other
code runs. Plus: the device-listing loop builds a
`sudo bluetoothctl info <mac>` command from bluetoothctl's own
output without escapeshellarg — normally safe AA:BB:CC:DD:EE:FF
but defence in depth doesn't hurt. Also escape device name and MAC
into the HTML form value attributes.
AR5-17 jsoneditor.php — path-mapping correction:
The `monitor` and `offline_monitor` keys both pointed at the
resume.json paths. Admin clicking those tabs in the editor opened
the wrong file content. Corrected to monitor.json and
offline_monitor.json respectively. (Schema validation for
jsoneditor.php — also AR5-17 — is intentionally out of scope for
this small bundle; the persistent-XSS exposure via data.json name
fields requires a frontend-side fix that's a separate phase.)
mupihat.php's save_custom branch wrote $_POST values straight into mupiboxconfig.json with no intval, no range check, no ordering check. A malformed POST could persist non-numeric strings or out-of-range millivolts. The high-impact field is th_shutdown — if it's set above the pack's normal range the box will shutdown-loop; if it's 0 or negative the protection safeguard is silently disabled, and the pack can be deep-discharged past BMS cutoff. Validation now enforced before writing: 1. Each field must be a positive integer string (ctype_digit on the raw POST value, then intval to bind). 2. Each value must lie in 4000-12600 mV (covers 1S, 2S and 3S Li-Ion packs the BQ25792 can charge). 3. Discharge curve strictly descending: v_100 > v_75 > v_50 > v_25 > v_0. 4. Threshold ordering: v_0 >= th_warning >= th_shutdown. On any failure, the partial state is discarded and a descriptive (htmlspecialchars-escaped) message is added to $CHANGE_TXT so the user sees why the save was rejected. No write to /etc/mupibox/ mupiboxconfig.json in that case.
mupihat.py's Flask app was bound to 0.0.0.0:5000, exposing the BQ25792 register inspector at "/" and "/api/registers" to every client on the LAN — without authentication. Acceptable on a trusted home network, problematic on guest/school WiFi: charger registers disclose pack voltage, current, temperature, and JEITA state. No consumer on the box hits port 5000 — frontend-box, admin-ui, backend-api server.ts, telegram_*.py, mqtt.py, .bashrc and fan_control.py all read /tmp/mupihat.json directly. So switching to host="127.0.0.1" loses no functionality. For remote debugging the port is still reachable via `ssh -L 5000:127.0.0.1:5000 mupibox`.
POST['cpugovernor'] floss ungeprüft in einen verschachtelten sudo su -c "...G_CONFIG_INJECT 'CONFIG_CPU_GOVERNOR=<wert>'..." und war post-auth command-injectable. Whitelist gegen die vom Kernel angebotene Liste aus scaling_available_governors — das ist genau die Liste, aus der der HTML-<select> generiert wird, also keine funktionale Einschränkung. Plus htmlspecialchars im Echo gegen XSS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Was es macht
backup.php,debug.php,pm2logs.phpund den jsoneditor (CSRF + interfacelogin-Pin)/api/deleteLocal+ Spotify-Cover-Cacheescapeshellarg/htmlspecialcharsin allen PHP-Pages, die Shell-Calls oder echo machen (spotify, bluetooth, cover, network, mupi, data.json, wlan, jsoneditor)/api/rssfeed(Host-Whitelist, kein file:// oder localhost)Architektur (kurz)
Kein neues Auth-Layer — wir hängen uns überall an das bestehende
is_logged_in()ausheader.phpund ancsrf_check.php. Validierung in PHP-Routinen liegt jeweils direkt am Eintrittspunkt, nicht in einer zentralen Middleware (matcht den restlichen Codebase-Stil).Test in Codespaces / lokal
npm installim Rootcp config/templates/www.json src/backend-api/config/config.json+monitor.jsonkopierennpm run serve:backend-api(Terminal 1)cd AdminInterface/www && php -S 127.0.0.1:8000(Terminal 2)curl http://127.0.0.1:8000/backup.php→ erwartet 302 →login.phpcurl http://127.0.0.1:8000/jsoneditor.phpohne Session → 302file:///etc/passwdsetzen → erwartet leere/abgelehnte Response../../../tmp/xals Pfad hat → erwartet Ablehnung; echo pwnankoppeln → erwartet escaping, kein Command-InjectHinweis: Telegram-chatId-Auth und mupihat-Flask-Loopback sind nur auf realer Box prüfbar (Bot-Hookup + Hardware).
Offene Punkte
interfacelogin-Pin Default-leer bleiben (= Feature opt-in) oder beim ersten Setup zwingend abgefragt werden? Ich hab Default-leer gelassen, um Bestandsboxen nicht zu blockieren.