fix: validate D-Bus method inputs server-side #49

Merged

mysticalsoap merged 1 commit from mysticalsoap/fix/service-input-validation into trunk

2026-08-19 01:01:59 -04:00

AGit

mysticalsoap commented

2026-08-18 21:44:07 -04:00

Owner

Problem

The second half of #18: polkit (#47) settled who may call the root service, but the methods still trusted whatever an authorized caller sent — caller-controlled names reached setattr/getattr on the service object, path-shaped values were joined under ROOTDIR without containment (an escaping openvpn config path is a root-exec primitive via up scripts), change_ovpn_config pointed root at an arbitrary caller directory, import_thread root-wrote into a directory named in the dict, and cfgupdate/fwupdate wrote raw caller bytes to root-owned files.

Fix

New helpers utils.valid_name() (single path segment: nonempty, no /, not ./..) and utils.inside_rootdir() (realpath containment — catches .., absolute paths, and symlink escapes), applied server-side:

connect_to_server/set_hop: server dicts validated (name/provider as names, path must resolve under ROOTDIR); tunnel threads live in a tunnel_threads dict instead of setattr(self, "<name>_dict"), killing the arbitrary-attribute write
start_import_thread/cancel_import: same dict-registry treatment (import_threads); cancel_import no-ops on unknown providers instead of AttributeError
import_thread: provider name validated; the download directory is now derived from the D-Bus caller (GetConnectionUnixUser → pwd) instead of trusting a homedir value in the dict — root no longer writes to a caller-chosen path. Per-provider storage also fixes concurrent imports clobbering each other's target
change_ovpn_config: signature changed ss → sa{ss} — the GUI reads its own files with its own privileges and sends contents; the service writes only to fixed per-provider locations with validated filenames. Root never reads a caller-named path
delete_provider: provider validated before it reaches shutil.rmtree
return_tun_device: whitelisted to tun/tun_hop/tun_bypass (was getattr with any string)
log_level_change: whitelisted to real log levels (was getattr(logging, ...))
cfgupdate/fwupdate: must parse as a JSON object; the parsed object is re-serialized, so arbitrary bytes can't land in root-owned files
bypass: interface names must exist per netifaces
Rejections raise a typed org.aqomui.service.InvalidInput

Custom-provider single-server config edits did nothing: apply_edit assigned the temp filename but never wrote it, then told the service to copy the temp dir (stale or empty). Sending contents directly fixes it structurally.
Provider auto-update has been broken upstream all along: its credentials dict used the literal key "config.HOMEDIR" instead of "homedir", so import_thread raised KeyError every time the 5-day refresh fired. Deriving the directory server-side removes the key entirely; both GUI dicts dropped it.

Verification

python -W error::SyntaxWarning -m compileall aqomui/ clean; CI runs pytest/ruff
New tests/test_utils.py covers both helpers (traversal, absolute paths, symlink escape, the root itself); all cases verified locally before pushing
Not live-tested against a running service — needs a rebuilt package; the connect/import flows also need a provider account. The mechanical changes (dict registries, whitelists, JSON round-trip) are covered by compile + the tests; the change_ovpn_config GUI flow is worth one manual modify-server pass whenever a package is next installed

Closes #18

Assisted-by: claude-fable-5

## Problem The second half of [#18](https://git.mysticalsoap.com/mysticalsoap/aqomui/issues/18): polkit (#47) settled *who* may call the root service, but the methods still trusted whatever an authorized caller sent — caller-controlled names reached `setattr`/`getattr` on the service object, path-shaped values were joined under `ROOTDIR` without containment (an escaping openvpn config path is a root-exec primitive via `up` scripts), `change_ovpn_config` pointed root at an arbitrary caller directory, `import_thread` root-wrote into a directory named in the dict, and `cfgupdate`/`fwupdate` wrote raw caller bytes to root-owned files. ## Fix New helpers `utils.valid_name()` (single path segment: nonempty, no `/`, not `.`/`..`) and `utils.inside_rootdir()` (realpath containment — catches `..`, absolute paths, and symlink escapes), applied server-side: - `connect_to_server`/`set_hop`: server dicts validated (`name`/`provider` as names, `path` must resolve under `ROOTDIR`); tunnel threads live in a `tunnel_threads` dict instead of `setattr(self, "<name>_dict")`, killing the arbitrary-attribute write - `start_import_thread`/`cancel_import`: same dict-registry treatment (`import_threads`); `cancel_import` no-ops on unknown providers instead of `AttributeError` - `import_thread`: provider name validated; the download directory is now derived from the D-Bus caller (`GetConnectionUnixUser` → `pwd`) instead of trusting a `homedir` value in the dict — root no longer writes to a caller-chosen path. Per-provider storage also fixes concurrent imports clobbering each other's target - `change_ovpn_config`: signature changed `ss` → `sa{ss}` — the GUI reads its own files with its own privileges and sends contents; the service writes only to fixed per-provider locations with validated filenames. Root never reads a caller-named path - `delete_provider`: provider validated before it reaches `shutil.rmtree` - `return_tun_device`: whitelisted to `tun`/`tun_hop`/`tun_bypass` (was `getattr` with any string) - `log_level_change`: whitelisted to real log levels (was `getattr(logging, ...)`) - `cfgupdate`/`fwupdate`: must parse as a JSON object; the parsed object is re-serialized, so arbitrary bytes can't land in root-owned files - `bypass`: interface names must exist per `netifaces` - Rejections raise a typed `org.aqomui.service.InvalidInput` ## Related fixes (found along the way, technically out of scope) - Custom-provider single-server config edits did nothing: `apply_edit` assigned the temp filename but never wrote it, then told the service to copy the temp dir (stale or empty). Sending contents directly fixes it structurally. - Provider auto-update has been broken upstream all along: its credentials dict used the literal key `"config.HOMEDIR"` instead of `"homedir"`, so `import_thread` raised `KeyError` every time the 5-day refresh fired. Deriving the directory server-side removes the key entirely; both GUI dicts dropped it. ## Verification - `python -W error::SyntaxWarning -m compileall aqomui/` clean; CI runs pytest/ruff - New `tests/test_utils.py` covers both helpers (traversal, absolute paths, symlink escape, the root itself); all cases verified locally before pushing - Not live-tested against a running service — needs a rebuilt package; the connect/import flows also need a provider account. The mechanical changes (dict registries, whitelists, JSON round-trip) are covered by compile + the tests; the `change_ovpn_config` GUI flow is worth one manual modify-server pass whenever a package is next installed Closes #18 Assisted-by: claude-fable-5

mysticalsoap added 1 commit

2026-08-18 21:44:07 -04:00

fix: validate D-Bus method inputs server-side

ci / test (pull_request) Successful in 50s

Details

ci / test (push) Successful in 26s

Details

384a3bee0d

Polkit (#47) settled who may call the service; this settles what it
accepts: names must be single path segments, config paths must resolve
under ROOTDIR, config updates must parse as JSON, and setattr/getattr
on caller-controlled names is gone. change_ovpn_config takes file
contents instead of a directory for root to read, and imports land in
a directory derived from the caller, not one named in the dict.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

mysticalsoap referenced this pull request

2026-08-19 00:46:08 -04:00

fix: imported configs must stay readable by the gui #50

mysticalsoap merged commit 384a3bee0d into trunk

2026-08-19 01:01:59 -04:00