Harden pimonitor.service with additional systemd sandboxing#52
Merged
Conversation
Extend the systemd sandbox of pimonitor.service with cheap, widely supported directives to further reduce the kernel attack surface exposed to a process that parses network-facing input and shells out to apt/vcgencmd (finding S5). Add ProtectKernelModules, ProtectKernelLogs, ProtectClock, ProtectHostname, RestrictNamespaces, LockPersonality, MemoryDenyWriteExecute, RestrictRealtime, RestrictSUIDSGID, RemoveIPC, SystemCallArchitectures=native, a @System-service SystemCallFilter that denies @privileged/@resources, and UMask=0077. PrivateDevices is intentionally left unset because vcgencmd needs /dev/vchiq (or /dev/vcio) for the GPU temperature. MemoryDenyWriteExecute is safe because the binary is pure Go with CGO disabled (no W+X pages). Existing directives are unchanged; the comment block is expanded to explain the additions. Closes #22
LarsLaskowski
commented
Jul 15, 2026
Owner
Author
There was a problem hiding this comment.
Reviewed against the project's review checklist (.claude/skills/review-pr). go build ./..., go vet ./..., and go test ./... all pass on the branch (packaging-only change, as expected), and systemd-analyze verify is clean apart from the known missing-binary warning.
The direction is good and most directives are sound, but two of the new directives would break the service at runtime, and neither is catchable by systemd-analyze verify — see the inline comments:
SystemCallFilter=~@privileged @resourceskills the Go 1.22 binary with SIGSYS beforemain()(Go ≥1.19 callssetrlimit/prlimit64at init, both in@resources, and noSystemCallErrorNumber=is set). The service would enter a restart loop.ProtectClock=trueimplicitly enables device-access enforcement (via its impliedDeviceAllow=char-rtc r), blocking/dev/vchiqand silently killing the GPU temperature metric — despite the unit comment explicitly avoidingPrivateDevicesfor that reason.
Everything else checks out:
MemoryDenyWriteExecuteclaim verified: the arm/arm64 release targets in the Makefile setCGO_ENABLED=0, so no W+X pages.ProtectKernelModules/Logs,ProtectHostname,RestrictNamespaces,LockPersonality,RestrictRealtime,RestrictSUIDSGID,RemoveIPC,SystemCallArchitectures=native,UMask=0077are all appropriate for this service and don't conflict with what it does (read-only/proc///sysparsing, TCP listener, state dir writes).- On older Raspberry Pi OS releases with systemd < 245, some directives are unknown; systemd ignores them with a warning rather than failing, so that's fine.
- No API, Go code, or dependency changes; comments/docs are in English.
The on-device verification checklist in the PR description is exactly right and would have caught issue 1 on the first systemctl restart — please keep that step after fixing the two findings.
Address two review findings that systemd-analyze verify cannot catch: - Add SystemCallErrorNumber=EPERM. Since Go 1.19 the runtime raises RLIMIT_NOFILE via setrlimit/prlimit64 (both in @resources) at process init. Without a soft error number, the ~@resources deny rule would terminate the binary with SIGSYS before main() runs, causing a Restart=on-failure loop. EPERM lets Go (which ignores the setrlimit error) and the inherited apt/vcgencmd children degrade gracefully. - Drop ProtectClock=true. It implies DeviceAllow=char-rtc, and any DeviceAllow= flips DevicePolicy into enforcing mode, which would silently block /dev/vchiq and kill the GPU temperature metric - the exact device access the unit comment says it preserves. CAP_SYS_TIME is already removed via the empty CapabilityBoundingSet and @clock syscalls are already denied by ~@PRIVILEGED, so ProtectClock added almost nothing here.
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
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.
Summary
Extends the systemd sandbox of
packaging/pimonitor.servicewith a set ofcheap, widely-supported directives to further reduce the kernel attack
surface exposed to a process that parses network-facing input and shells
out to
apt/vcgencmd(review finding S5, defense in depth).Added to
[Service]:Notes:
PrivateDevicesis intentionally not set —vcgencmdneeds/dev/vchiq(or/dev/vcio) to read the GPU temperature. Enabling itwould require paired
DeviceAllow=rules and is out of scope.MemoryDenyWriteExecuteis safe — the binary is pure Go with CGOdisabled in builds (no JIT, no W+X pages).
the unit is expanded to document the additions and the
PrivateDevicescaveat.
Related Issue
Closes #22
Verification
systemd-analyze verify packaging/pimonitor.servicepasses with nosyntax errors on any of the new directives (only the expected warning
that the
ExecStartbinary is absent in the CI/dev container).On-device verification still required (this container has no booted
systemd, so
systemd-analyze securitycannot run here). On a Raspberry Pior Debian-family host, please confirm the acceptance criteria from the
issue:
systemctl daemon-reload && systemctl restart pimonitorstarts cleanly;dashboard reachable; CPU/memory/disk/network/temperature metrics
populate; the updates count appears after the next slow tick (i.e.
apt list --upgradablestill works under the syscall filter); GPUtemperature still shows where
vcgencmdexists.journalctl -u pimonitorshows no seccomp/permission errors.systemd-analyze security pimonitor.servicescore improves versus theprevious unit (please record before/after here).
Checklist
go test ./...passes locally) — n/a, packaging-only change (no Go code touched)go vet ./...andgolangci-lint runare clean — n/a, no Go code changeddocs/API.md),configuration (
README.md,packaging/pimonitor.example.yaml), orinstallation/packaging (
packaging/install.sh, systemd units) —the systemd unit's inline comment block is updated
/api/v1/...response shapes, or a new APIversion was introduced instead — n/a, no API change