refactor(security): harden path classification and failure handling in systemd_service_auditor by krishsharma-code · Pull Request #5 · krishsharma-code/kali-linux-automation

krishsharma-code · 2026-10-31T06:40:42Z

Summary

Audit of python_scripts/systemd_service_auditor.py, the script that flags running systemd services whose unit files live outside the standard systemd directories. The audit found three genuine correctness defects that make the tool report false results in both directions, plus missing failure handling and no way to consume the output programmatically.

Linked Issue

Closes #

Type of Change

What Changed

Path comparison is now component-based. str.startswith('/lib/systemd/system') accepted /lib/systemd/system-evil/backdoor.service as a standard location. _is_within() compares normalised components (path == dir or path.startswith(dir + os.sep)), so a sibling directory with a shared prefix can no longer masquerade as a trusted one.
High-risk directory matching is component-based too. The old '/home' in path test flagged /opt/homegrown/app.service as high risk and missed nothing it should have caught. Same fix, applied to /tmp, /var/tmp, /dev/shm, /home.
Symlinks are resolved before classification. /etc/systemd/system/foo.service is usually a symlink into /lib/systemd/system/; the old code reported every such unit as non-standard. --no-resolve-symlinks restores the raw behaviour when needed.
systemctl failures are no longer silent. The old except Exception: continue hid a missing binary, a dead bus and a per-unit failure alike, and a non-zero exit produced an empty unit list that was reported as "all services standard". Failures now raise AuditError (exit 2) or land in AuditReport.errors and are printed.
Every systemctl call has a timeout (--timeout, default 15s) so a hung bus cannot wedge the audit forever.
Unit names are validated against SERVICE_NAME_RE before being passed to a second subprocess. argv is used throughout, so there was no shell injection, but unvalidated names are no longer forwarded.
Output is now consumable: --json, --standard-path (repeatable), --no-resolve-symlinks, and an exit-code contract (0 clean / 1 findings / 2 could not run) for CI.
Docs: module docstring covering the trust boundary and exit codes, docstrings on every public function, Finding / AuditReport dataclasses, type hints.
Tests: new tests/test_systemd_service_auditor.py — 36 tests covering output parsing, name validation, component boundaries, symlink resolution, failure handling and the CLI contract.

audit_systemd_services() is kept as the historical entry point, so existing imports and the README description still hold.

New or Modified Scripts

Script	Language	Change
`python_scripts/systemd_service_auditor.py`	Python	Refactor + defect fixes
`tests/test_systemd_service_auditor.py`	Python	New test module

Safety Review

This script only performs actions the user explicitly requests
Any destructive operation requires an explicit flag and supports --dry-run (n/a — the script is read-only)
Any state-changing action is printed before it is carried out (n/a — no state changes)
No credentials, API keys or tokens are committed
No hardcoded personal or machine-specific paths

Testing Checklist

python3 -m py_compile python_scripts/*.py passes
flake8 python_scripts --select=E9,F63,F7,F82,F821,F823 --max-line-length=120 passes
for f in bash_scripts/*.sh; do bash -n "$f"; done passes (untouched)
shellcheck bash_scripts/*.sh reports no errors (advisory, untouched)
python3 -m pytest tests -q — 127 passed
I tested the --help output
I tested the failure path: no systemd in the sandbox gives exit 2 with [!] Error running systemctl: systemctl not found

Documentation

README.md already indexes systemd_service_auditor.py; description unchanged
The script has a module docstring explaining what it does
Non-obvious logic (component matching, symlink resolution) is commented

Code Quality Checklist

My code follows the standards in CONTRIBUTING.md
Imports are at the top of the file
I have performed a self-review of my own code
I have removed debugging code and unused imports

Additional Notes

This is a self-review; GitHub blocks self-approval, so the formal review is submitted with event: COMMENT and carries the line-level findings.

Follow-up not in this PR: the script still only reads FragmentPath. A later pass could also compare unit-file ownership/permissions, which is the other half of the same persistence story.

…n systemd_service_auditor Audit of python_scripts/systemd_service_auditor.py: - Compare FragmentPath by path component instead of a raw prefix, so /lib/systemd/system-evil/x.service is no longer treated as standard - Match high-risk directories by component, so /opt/homegrown no longer matches /home - Resolve symlinks before classifying, avoiding false positives on the very common /etc/systemd/system -> /lib/systemd/system links - Bound every systemctl call with a timeout and fail loudly on a non-zero exit instead of reporting a false all-clear - Validate unit names before they are handed back to a subprocess and stop swallowing per-unit exceptions - Add argparse (--json, --timeout, --standard-path, --no-resolve-symlinks) and an exit-code contract for CI use - Add tests/test_systemd_service_auditor.py

krishsharma-code

Self-review of python_scripts/systemd_service_auditor.py (submitted as COMMENT because GitHub blocks self-approval on your own PR).

Scope reviewed: the whole script as it stood on main, against four axes — security boundaries, memory efficiency, edge-case/failure handling, and docstring/comment clarity. Line references below are to the new version.

What was found (3 genuine defects, 4 robustness gaps):

path.startswith('/lib/systemd/system') treated /lib/systemd/system-evil/backdoor.service as a standard location — a false negative that hides exactly the persistence mechanism the tool exists to catch.
p in path for /tmp, /home etc. matched substrings, so /opt/homegrown/app.service was reported HIGH RISK — a false positive that trains the operator to ignore the alert.
Symlinked units (/etc/systemd/system/foo.service -> /lib/systemd/system/foo.service) were reported as non-standard. On a stock Debian/Ubuntu box that is most of the enabled units.
except Exception: continue swallowed every per-unit failure with no record.
A non-zero systemctl exit produced empty stdout, which fell through to "All active systemd services are running from standard locations" — a false all-clear on the failure path.
No timeout on any systemctl call.
Unit names taken from stdout were passed straight back into a second subprocess without validation.

What changed and why: component-based _is_within() comparison (1, 2), os.path.realpath before classification with --no-resolve-symlinks to opt out (3), AuditError plus AuditReport.errors so failures are reported rather than hidden (4, 5), timeout= on every call with TimeoutExpired translated (6), SERVICE_NAME_RE validation before re-invoking systemctl (7). Plus argparse, --json, and a 0/1/2 exit-code contract so the script can be a CI gate, a full module docstring documenting the trust boundary, and 36 new tests.

Not changed on purpose: output text is preserved line-for-line for existing findings, and audit_systemd_services() still prints and returns a report, so anyone importing or grepping the old output keeps working. Still read-only — no state changes anywhere.

Verification: py_compile clean, flake8 strict set clean, pytest tests -q -> 127 passed, --help exercised, and the no-systemd path confirmed to exit 2 with a clear message instead of a false clean run.

Follow-up (not in this PR): unit-file ownership and permission checks (--property=FragmentPath gives the path but not who owns it), and a --units-file mode for auditing a mounted image without systemd running.

krishsharma-code · 2026-10-31T06:41:14Z

+#: Unit names systemd accepts: alphanumerics plus ``: _ . @ -`` and the
+#: ``\x2d``-style escapes some units use.  Anything else is not passed on to a
+#: subprocess.
+SERVICE_NAME_RE = re.compile(r"^[A-Za-z0-9_.@:\\-]+$")


This regex is the trust boundary for anything that comes back from systemctl output and then gets handed to a second subprocess. Worth keeping deliberately narrow: it allows A-Za-z0-9_.@:\- and nothing else, so a name containing a space, ;, /, $ or a newline never reaches argv. argv usage means there was no shell-injection path even before this change, but validating here also protects the systemctl show call from being handed a name that systemd would interpret as a different unit (e.g. --foo style arguments).

krishsharma-code · 2026-10-31T06:41:14Z

+# systemctl helpers
+# ---------------------------------------------------------------------------
+
+def _run_systemctl(args: list[str], timeout: float = DEFAULT_TIMEOUT) -> str:


All three failure modes now collapse into one exception type with an actionable message, and timeout= is set on every call. Previously capture_output=True with no timeout meant a wedged dbus/systemd --user would hang the audit indefinitely, and check=False (the default) meant a non-zero exit returned empty stdout that downstream code read as 'no services'. Translating OSError as well covers the permission-denied-on-the-binary case, which is a plausible sudo misconfiguration.

krishsharma-code · 2026-10-31T06:41:14Z

+    return completed.stdout
+
+
+def list_running_services(timeout: float = DEFAULT_TIMEOUT) -> list[str]:


--plain is the fix for the parsing hazard: without it, systemd decorates failed units with a leading ● and may colourise, so line.split()[0] could yield ● instead of the unit name. lstrip handles the bullet for older systemd that ignores --plain, and blank lines are skipped before indexing — the old code raised IndexError on an empty line.

krishsharma-code · 2026-10-31T06:41:14Z

+# Classification
+# ---------------------------------------------------------------------------
+
+def _is_within(path: str, directory: str) -> bool:


This is the core fix. Comparing normalised components means /lib/systemd/system-evil/x.service is not inside /lib/systemd/system, while /lib/systemd/system/x.service and /lib/systemd/system itself both are. The path == directory arm matters for the (unusual) case of FragmentPath pointing at a directory.

krishsharma-code · 2026-10-31T06:41:14Z

+    if not fragment_path:
+        return Finding(service, "", "missing-fragment-path", SEVERITY_INFO)
+
+    path = os.path.realpath(fragment_path) if resolve_symlinks else fragment_path


Resolving before classifying removes the largest source of false positives: /etc/systemd/system/*.service is normally a symlink into /lib/systemd/system/. Note this deliberately resolves the reported path, not the trust decision — if an attacker can symlink from /etc/systemd/system into /tmp, the resolved target still lands in the high-risk branch, which is the behaviour we want.

krishsharma-code · 2026-10-31T06:41:14Z

-
-        except Exception:
+            fragment_path = get_fragment_path(service, timeout=timeout)
+        except (AuditError, ValueError) as exc:


Per-unit failures are now recorded rather than discarded. One unreadable unit no longer aborts the scan, and it also no longer disappears — the error is printed and included in --json output, so an operator can tell 'nothing found' apart from 'nothing could be read'.

krishsharma-code · 2026-10-31T06:41:14Z

+    else:
+        print_report(report)
+
+    return 1 if report.has_problems else 0


The exit-code contract (0 clean / 1 findings / 2 could not run) is what makes this usable in CI or a cron wrapper. 2 for 'the audit itself failed' is deliberately distinct from 1 for 'findings present', so a wrapper can alert differently on a broken audit than on a real hit.

krishsharma-code · 2026-10-31T06:41:14Z

+    assert auditor.classify_path("a.service", "/usr/lib/systemd/system/a.service") is None
+
+
+def test_classify_prefix_boundary_is_not_standard():


This test is the regression guard for defect #1 — it fails against the old startswith implementation. Same for test_classify_substring_is_not_high_risk and test_classify_sibling_of_home_is_not_high_risk, which pin the /opt/homegrown and /home2 false positives.

krishsharma-code · 2026-10-31T06:41:15Z

+    assert report.has_problems is False
+
+
+def test_audit_services_records_error_and_continues(fake):


The fake systemctl injects at the _run_systemctl seam, so these tests exercise real control flow without needing systemd. Failure translation (missing binary, timeout, non-zero exit, OSError) is covered separately at the subprocess.run seam in test_run_systemctl_translates_*.

krishsharma-code commented Oct 31, 2026

View reviewed changes

krishsharma-code merged commit 37bdbc8 into main Oct 31, 2026
2 of 3 checks passed

krishsharma-code deleted the audit/review-systemd-service-auditor branch October 31, 2026 06:41

Navigation Menu

Repository navigation

refactor(security): harden path classification and failure handling in systemd_service_auditor - #5

refactor(security): harden path classification and failure handling in systemd_service_auditor#5
krishsharma-code merged 1 commit into
mainfrom
audit/review-systemd-service-auditor

krishsharma-code commented Oct 31, 2026

Uh oh!

krishsharma-code left a comment

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

krishsharma-code Oct 31, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		return completed.stdout


		def list_running_services(timeout: float = DEFAULT_TIMEOUT) -> list[str]:

		assert auditor.classify_path("a.service", "/usr/lib/systemd/system/a.service") is None


		def test_classify_prefix_boundary_is_not_standard():

		assert report.has_problems is False


		def test_audit_services_records_error_and_continues(fake):

Conversation

krishsharma-code commented Oct 31, 2026

Summary

Linked Issue

Type of Change

What Changed

New or Modified Scripts

Safety Review

Testing Checklist

Documentation

Code Quality Checklist

Additional Notes

Uh oh!

krishsharma-code left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant