From bc2f2b527f7686c528ef028b4e7484be05fc3420 Mon Sep 17 00:00:00 2001
From: boban
Date: Sun, 5 Jul 2026 16:05:16 +0200
Subject: [PATCH] fix(security): close file-read RBAC gap + fail2ban
arg-injection + update/HMAC hardening
Confirmed-security quick-wins from a full re-audit (6-agent subsystem fan-out,
Codex-reviewed). Each item ships with a regression test proven to fail pre-fix.
- files: gate file CONTENT reads (download / edit / FileEditor::load) behind the
`operate` ability so a read-only viewer keeps LISTING/browse access but can no
longer pull file bytes over the server credential (root -> /etc/shadow, keys,
.env). Hide the download/edit/delete controls from viewers in the blade, and
basename() Files::open() for parity with the other path builders.
- fail2ban: reject a leading dash/dot in validJail(). A jail name like "-h" or
"--help" was parsed by fail2ban-client as an OPTION (argument-injection) rather
than a positional jail; internal dashes ("nginx-http-auth") still pass.
- bruteforce: release the atomically-claimed ban-audit dedup slot when the
AuditEvent write throws, so a transient DB failure no longer suppresses the
auth.ip_banned audit for the whole 60s window (the ban itself is unaffected).
- update path: verify_update_request now FAILS CLOSED when UPDATE_HMAC_KEY is
unset (was fail-open -> any ./run marker could drive a root update). Pin the
self-update `git pull` to the recorded CLUSEV_BUILD_BRANCH and refuse a diverged
checkout, so a `git checkout` in the tree cannot redirect the next root update.
- wireguard: normalise the DNS input to a single comma list and QUOTE the WG_DNS
assignment in wg.env; an unquoted space previously split the assignment and
broke the next `. "$WG_ENV"`.
13 new regression tests. 614 tests, Pint, shellcheck, Codex review all green.
Co-Authored-By: Claude Fable 5
---
app/Livewire/Files/Index.php | 15 ++-
app/Livewire/Modals/FileEditor.php | 5 +
app/Services/BruteforceGuard.php | 27 +++--
app/Services/Fail2banService.php | 7 +-
docker/restart-sentinel/watch.sh | 6 +-
docker/wg/clusev-wg.sh | 11 ++-
.../views/livewire/files/index.blade.php | 37 ++++---
tests/Feature/BruteforceGuardBanTest.php | 46 +++++++++
tests/Feature/Fail2banJailValidationTest.php | 99 +++++++++++++++++++
tests/Feature/RbacOperateGateTest.php | 89 +++++++++++++++++
tests/Feature/RbacUiHidingTest.php | 42 ++++++++
update.sh | 18 +++-
12 files changed, 370 insertions(+), 32 deletions(-)
create mode 100644 tests/Feature/Fail2banJailValidationTest.php
diff --git a/app/Livewire/Files/Index.php b/app/Livewire/Files/Index.php
index fda2e35..8980319 100644
--- a/app/Livewire/Files/Index.php
+++ b/app/Livewire/Files/Index.php
@@ -60,7 +60,11 @@ class Index extends Component
return;
}
- $this->path = rtrim($this->path, '/').'/'.$name;
+ // basename() for parity with download()/edit()/confirmDelete(): a listing entry
+ // name is a single path segment, never a traversal fragment. $entries is a public,
+ // non-#[Locked] property, so a crafted /livewire/update could otherwise set name to
+ // ../.. and walk outside the browsed tree.
+ $this->path = rtrim($this->path, '/').'/'.basename($name);
$this->load();
}
@@ -102,6 +106,11 @@ class Index extends Component
/** Stream a remote file to the browser as a download (SFTP get). */
public function download(int $index, FleetService $fleet)
{
+ // Reading a file's BYTES over the server credential (often root) discloses secrets a
+ // viewer must not reach (/etc/shadow, keys, .env). Browsing the LISTING stays open;
+ // pulling content requires operate, like every other file operation.
+ abort_unless(auth()->user()?->can('operate'), 403);
+
$name = $this->entries[$index]['name'] ?? null;
$active = $this->activeServer();
if ($name === null || ! $active || ! $active->credential_exists) {
@@ -122,6 +131,10 @@ class Index extends Component
/** Open the view/edit modal for a file. */
public function edit(int $index): void
{
+ // The editor reads (and can write) file content — an operate action. Gate here so a
+ // viewer never even opens the modal; FileEditor::load() re-checks on the read itself.
+ abort_unless(auth()->user()?->can('operate'), 403);
+
$name = $this->entries[$index]['name'] ?? null;
$active = $this->activeServer();
if ($name === null || ! $active) {
diff --git a/app/Livewire/Modals/FileEditor.php b/app/Livewire/Modals/FileEditor.php
index c22cee3..8118a4e 100644
--- a/app/Livewire/Modals/FileEditor.php
+++ b/app/Livewire/Modals/FileEditor.php
@@ -78,6 +78,11 @@ class FileEditor extends ModalComponent
public function load(FleetService $fleet, Sftp $sftp): void
{
+ // The actual content read (SFTP get / readFile). Gate it directly — not just the
+ // Files::edit entry point — so file bytes the server credential can reach (root →
+ // /etc/shadow, keys, .env) are never disclosed to a read-only viewer.
+ abort_unless(auth()->user()?->can('operate'), 403);
+
$server = Server::find($this->serverId);
if (! $server) {
$this->error = __('common.server_not_found');
diff --git a/app/Services/BruteforceGuard.php b/app/Services/BruteforceGuard.php
index de2a539..67c8ff7 100644
--- a/app/Services/BruteforceGuard.php
+++ b/app/Services/BruteforceGuard.php
@@ -158,15 +158,24 @@ class BruteforceGuard
// honeypot ban) is still audited. A short 60s TTL (matching record()'s ban dedup) collapses the
// rapid scanner flood WITHOUT masking a legitimate later re-ban — e.g. after an operator unban,
// which is minutes of human time later, well past this window (so it is not tied to unban()).
- if (Cache::add('bruteforce:banaudit:'.$canonical.':'.$reason, 1, 60)) {
- AuditEvent::create([
- 'user_id' => null,
- 'actor' => 'system',
- 'action' => 'auth.ip_banned',
- 'target' => $canonical,
- 'ip' => $canonical,
- 'meta' => ['reason' => $reason],
- ]);
+ $auditKey = 'bruteforce:banaudit:'.$canonical.':'.$reason;
+ if (Cache::add($auditKey, 1, 60)) {
+ try {
+ AuditEvent::create([
+ 'user_id' => null,
+ 'actor' => 'system',
+ 'action' => 'auth.ip_banned',
+ 'target' => $canonical,
+ 'ip' => $canonical,
+ 'meta' => ['reason' => $reason],
+ ]);
+ } catch (\Throwable $e) {
+ // The Cache::add above atomically CLAIMED the dedup slot before the write. If the
+ // write throws (a transient DB blip), release the slot so the 60s window is not
+ // burned with no audit row — the next attempt re-claims and re-audits this ban.
+ Cache::forget($auditKey);
+ throw $e;
+ }
}
return true;
diff --git a/app/Services/Fail2banService.php b/app/Services/Fail2banService.php
index 21fca9f..363fa13 100644
--- a/app/Services/Fail2banService.php
+++ b/app/Services/Fail2banService.php
@@ -398,7 +398,12 @@ class Fail2banService
private function validJail(string $jail): bool
{
- return (bool) preg_match('/^[A-Za-z0-9._-]+$/', $jail);
+ // Must START with an alphanumeric/underscore: a leading '-' would be parsed by
+ // fail2ban-client as an OPTION (e.g. "-h", "--help") rather than a jail argument,
+ // and a leading '.' has no legitimate jail use. Internal '.-_' stay allowed so real
+ // names like "nginx-http-auth" pass. (Shell metacharacters are already inert via the
+ // base64 transport in runPrivileged; this closes the argument-injection confusion.)
+ return (bool) preg_match('/^[A-Za-z0-9_][A-Za-z0-9._-]*$/', $jail);
}
private function validIpOrCidr(string $v): bool
diff --git a/docker/restart-sentinel/watch.sh b/docker/restart-sentinel/watch.sh
index 4de6faf..d04f74f 100755
--- a/docker/restart-sentinel/watch.sh
+++ b/docker/restart-sentinel/watch.sh
@@ -66,7 +66,11 @@ write_update_error() {
verify_update_request() {
local f="$1" key ts mac expect
key="$(grep -m1 '^UPDATE_HMAC_KEY=' "${CLUSEV_DIR}/.env" 2>/dev/null | cut -d= -f2-)"
- [ -n "$key" ] || { log "UPDATE_HMAC_KEY unset — skipping request authentication"; return 0; }
+ # Fail CLOSED: with no key the marker cannot be authenticated, so refuse rather than run a
+ # ROOT update on an unauthenticated request. install.sh always backfills UPDATE_HMAC_KEY
+ # (set_kv), so a real install has one; a missing key means a broken/hand-edited .env — the
+ # operator re-runs install.sh (or sets the key) to restore dashboard-triggered updates.
+ [ -n "$key" ] || { log "UPDATE_HMAC_KEY unset — REFUSING update request (re-run install.sh to backfill the key)"; return 1; }
ts="$(sed -n '1p' "$f" 2>/dev/null)"
mac="$(sed -n '2p' "$f" 2>/dev/null)"
[ -n "$ts" ] && [ -n "$mac" ] || { log "update request unsigned/malformed — refusing"; return 1; }
diff --git a/docker/wg/clusev-wg.sh b/docker/wg/clusev-wg.sh
index 3b62d57..61097f6 100755
--- a/docker/wg/clusev-wg.sh
+++ b/docker/wg/clusev-wg.sh
@@ -509,14 +509,19 @@ _set_endpoint() {
_set_dns() {
local dns="$1"; require_setup
- # One or more IPv4 addresses, comma/space separated (e.g. "10.0.0.1" or "1.1.1.1, 1.0.0.1").
+ # One or more IPv4 addresses, comma/space separated on INPUT (e.g. "10.0.0.1" or "1.1.1.1, 1.0.0.1").
# Only affects NEW peer configs (the DNS line is baked into each client config at creation).
case "$dns" in *[!0-9.,\ ]*|'') echo "bad dns" >&2; return 1 ;; esac
printf '%s' "$dns" | grep -qE '^[0-9]{1,3}(\.[0-9]{1,3}){3}([,[:space:]]+[0-9]{1,3}(\.[0-9]{1,3}){3})*$' || { echo "bad dns" >&2; return 1; }
+ # Normalise every comma/space run to a single comma before STORING: WireGuard's peer `DNS =`
+ # line wants a comma list, and — critically — an unquoted space in wg.env would split the
+ # WG_DNS assignment so the next `. "$WG_ENV"` runs the second address as a command. Store as a
+ # single comma-separated token and QUOTE the assignment (defence-in-depth) so sourcing is safe.
+ dns="$(printf '%s' "$dns" | tr -s ',[:space:]' ',' | sed -e 's/^,//' -e 's/,$//')"
if grep -q '^WG_DNS=' "$WG_ENV" 2>/dev/null; then
- sed -i "s#^WG_DNS=.*#WG_DNS=${dns}#" "$WG_ENV"
+ sed -i "s#^WG_DNS=.*#WG_DNS=\"${dns}\"#" "$WG_ENV"
else
- printf 'WG_DNS=%s\n' "$dns" >> "$WG_ENV" # older wg.env (pre-DNS) has no line yet
+ printf 'WG_DNS="%s"\n' "$dns" >> "$WG_ENV" # older wg.env (pre-DNS) has no line yet
fi
}
diff --git a/resources/views/livewire/files/index.blade.php b/resources/views/livewire/files/index.blade.php
index 8459934..a5f5a7c 100644
--- a/resources/views/livewire/files/index.blade.php
+++ b/resources/views/livewire/files/index.blade.php
@@ -101,16 +101,22 @@
{{ $e['name'] }}/
@else
- {{-- Filename opens the editor modal too — inline modalTrigger keeps the
- link layout but adds the pending state + timeout error toast. --}}
-
+ @can('operate')
+ {{-- Filename opens the editor modal too — inline modalTrigger keeps the
+ link layout but adds the pending state + timeout error toast.
+ operate-gated: reading content is not a viewer capability. --}}
+
+ @else
+ {{-- Viewer: read-only listing — the filename is not a content-read trigger. --}}
+ {{ $e['name'] }}
+ @endcan
@endif
@@ -128,13 +134,14 @@
{{ $e['modified'] }}
- {{-- Row actions --}}
+ {{-- Row actions — content read (download/edit) and delete all require operate;
+ a viewer only browses the listing, so it sees no row actions. --}}