fix(security): P1-1 + P1-3 — reject flag-shaped names in systemctl + fail2ban calls (#41)

* fix(security): reject systemctl unit names that look like flags

The HTTP handler for POST /api/v1/services/{name}/{action} validated the
service name with a per-character whitelist that allowed "-" anywhere,
including position 0. A name like "--all", "--no-block", or "-h" passed
validation and reached exec.Command("systemctl", action, name), where
systemctl treats it as a flag instead of a unit. Discrete-arg exec.Command
prevents shell injection but not flag injection — the kernel doesn't know
which argv slot is "the flags" and which is "the operand."

Concrete impact: a caller authenticated to the agent could pass action=start
and service=--all, producing `systemctl start --all`, which systemd
interprets as "start every available unit." Lower-privilege flags are
similar — `--no-block` (don't wait), `--state=...` (filter), etc.

Fix:
- New validUnitName regex (`^[a-zA-Z0-9][a-zA-Z0-9._@-]{0,255}$`) anchors
  the first character to alphanumeric so flag-shaped names are rejected
  at the HTTP boundary. Subsequent characters allow the systemd unit
  charset; total length unchanged (256 cap).
- ControlService passes "--" before the unit name in exec.Command as
  defense-in-depth.

Tests pin down both the legitimate-name pass list (sshd.service, haproxy,
getty@tty1.service, etc.) and the attacker-controlled reject list (--all,
-h, --no-block, .leading-dot, names with whitespace/semicolons/$()).

P1-1 from the 2026-05 security audit.

* fix(security): validate fail2ban jail names before exec; add `--` separator

getJailStats called `fail2ban-client status <jail>` with the jail name
flowing in from getJails(), which parses `fail2ban-client status` output.
The only flow today is internal: agent → fail2ban-client → parse →
fail2ban-client. There is no HTTP path that lets a caller specify a jail
name directly, so this is defense-in-depth rather than a closed exploit.

Two scenarios are still worth defending against:
1. A future code path that exposes a jail-name parameter to HTTP input
   (e.g. "force fail2ban-client to refresh jail X"), where the same
   flag-injection class as P1-1 (systemctl) would re-emerge.
2. A malicious or malformed fail2ban configuration that produces a
   weird jail name. fail2ban itself accepts jail names that include
   characters our parser passes through unchecked, so a jail named
   "--help" on a misconfigured host would crash the agent's collection
   loop or worse.

Fix:
- New validJailName regex (`^[a-zA-Z0-9][a-zA-Z0-9_-]{0,63}$`) anchors
  the first character to alphanumeric. getJailStats rejects mismatches
  with an explicit error rather than passing them through.
- exec.Command passes "--" before the jail name.

Tests cover both legitimate jail names (sshd, haproxy-http, my_jail)
and the flag-injection class (--help, -h) plus the usual injection
characters (newlines, semicolons, $()).

P1-3 from the 2026-05 security audit.

Author: sarg3nt

gearbox-agent/internal/gears/metrics/collector.go

@@ -231,9 +231,13 @@ func formatDuration(d time.Duration) string {
 
 // ControlService starts, stops, or restarts a systemd service.
 // Returns the command output and any error.
+//
+// The "--" separator before the unit name prevents an attacker-chosen unit
+// name (e.g. "--all" or "-h") from being interpreted as a systemctl flag.
+// This is defense-in-depth on top of validUnitName regex validation at the
+// HTTP boundary (plugin.go). See 2026-05 audit P1-1.
 func (c *Collector) ControlService(service, action string) (string, error) {
-	// Use systemctl to control the service
-	cmd := exec.Command("systemctl", action, service)
+	cmd := exec.Command("systemctl", action, "--", service)
 	output, err := cmd.CombinedOutput()
 	return strings.TrimSpace(string(output)), err
 }

gearbox-agent/internal/gears/metrics/collector_test.go

@@ -1,6 +1,7 @@
 package metrics
 
 import (
+	"strings"
 	"testing"
 	"time"
 )
@@ -347,3 +348,48 @@ func TestCollectServiceStatuses(t *testing.T) {
 		t.Errorf("expected non-existent service to be inactive")
 	}
 }
+
+// 2026-05 audit P1-1: service names that could be interpreted as systemctl
+// flags must be rejected before they reach exec.Command. The HTTP handler
+// in plugin.go matches against validUnitName, and ControlService also passes
+// "--" before the unit name as defense-in-depth — but the regex is the
+// primary gate.
+func TestValidUnitName(t *testing.T) {
+	tests := []struct {
+		name string
+		want bool
+	}{
+		// Real-world systemd unit names that must continue to work.
+		{"sshd.service", true},
+		{"haproxy", true},
+		{"docker.socket", true},
+		{"getty@tty1.service", true},
+		{"systemd-journald.service", true},
+		{"my_service", true},
+		{"a", true},
+
+		// Attacker-controlled values that must be rejected. The most
+		// important class: names whose first character is "-" so that
+		// exec.Command("systemctl", action, name) would treat the name
+		// as a flag.
+		{"", false},
+		{"--all", false},
+		{"-h", false},
+		{"--no-block", false},
+		{"--help", false},
+		{".leading-dot", false},
+		{"@leading-at", false},
+		{"name with space", false},
+		{"name\nwith-newline", false},
+		{"name;rm -rf /", false},
+		{"name$(whoami)", false},
+		{strings.Repeat("a", 257), false}, // length cap (256 chars allowed)
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := validUnitName.MatchString(tt.name); got != tt.want {
+				t.Errorf("validUnitName.MatchString(%q) = %v, want %v", tt.name, got, tt.want)
+			}
+		})
+	}
+}

gearbox-agent/internal/gears/metrics/plugin.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"encoding/json"
 	"net/http"
+	"regexp"
 	"strings"
 	"time"
 
@@ -13,6 +14,14 @@ import (
 	"github.com/sarg3nt/gearbox-agent/internal/framework/gear"
 )
 
+// validUnitName matches a systemd unit name that is safe to pass to systemctl
+// without risk of flag injection. Must start with an alphanumeric so a value
+// like "--all" or "-h" can't be interpreted as a systemctl flag; subsequent
+// characters match the systemd unit charset (alphanumerics plus `.`, `-`,
+// `_`, `@`). Length capped at 256 to match the prior length check.
+// See 2026-05 security audit, P1-1.
+var validUnitName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._@-]{0,255}$`)
+
 func init() {
 	gear.Register(&Gear{})
 }
@@ -267,18 +276,17 @@ func (p *Gear) handleServiceControl(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Validate service name (basic sanity check - prevent command injection)
-	if req.Service == "" || len(req.Service) > 256 {
+	// Validate service name. The first character MUST be alphanumeric so a
+	// crafted name like "--all" or "--no-block" cannot be passed to systemctl
+	// as a flag (discrete-arg exec prevents shell injection but not flag
+	// injection). Subsequent characters allow the usual systemd unit charset
+	// (alphanumeric plus . - _ @). collector.ControlService also passes "--"
+	// to systemctl before the unit name as defense-in-depth.
+	// See 2026-05 security audit, P1-1.
+	if !validUnitName.MatchString(req.Service) {
 		http.Error(w, "Invalid service name", http.StatusBadRequest)
 		return
 	}
-	// Only allow alphanumeric, dash, underscore, at-sign, and dot in service names
-	for _, c := range req.Service {
-		if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || c == '_' || c == '@' || c == '.') {
-			http.Error(w, "Invalid character in service name", http.StatusBadRequest)
-			return
-		}
-	}
 
 	// Execute systemctl command
 	output, err := p.collector.ControlService(req.Service, req.Action)

gearbox-agent/internal/gears/security/fail2ban.go

@@ -19,6 +19,20 @@ var ErrServiceNotInstalled = errors.New("service not installed")
 // Pre-compiled regex for fail2ban log parsing.
 var banRegex = regexp.MustCompile(`(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}).*\[(\w+)\] (Ban|Unban) (\S+)`)
 
+// validJailName matches a fail2ban jail name that is safe to pass to
+// fail2ban-client. The first character MUST be alphanumeric so a value
+// like "--help" or "-h" cannot be interpreted as a fail2ban-client flag
+// (discrete-arg exec prevents shell injection but not flag injection).
+// Subsequent characters allow alphanumerics, underscore, and hyphen.
+//
+// Jail names today flow from the agent's own `fail2ban-client status`
+// parse (not from HTTP input), so this is defense-in-depth — if a future
+// code path ever accepts a jail name from API input, or if a malicious
+// fail2ban config produces a weird jail name, this guard prevents the
+// value from being interpreted as a fail2ban-client flag. See 2026-05
+// audit P1-3.
+var validJailName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]{0,63}$`)
+
 // Fail2BanStats represents overall fail2ban statistics.
 type Fail2BanStats struct {
 	Available   bool          `json:"available"`   // Whether fail2ban is installed
@@ -142,10 +156,21 @@ func (c *Fail2BanCollector) getJails() ([]string, error) {
 }
 
 // getJailStats returns statistics for a specific jail.
+//
+// Defense-in-depth: the jail name is verified to match validJailName before
+// being passed to fail2ban-client, and "--" separates flags from the unit
+// argument. Today jailName always comes from getJails() (which parses
+// fail2ban-client's own output), but a malformed jail configuration on the
+// host could otherwise produce a value that fail2ban-client itself would
+// interpret as a flag. See 2026-05 audit P1-3.
 func (c *Fail2BanCollector) getJailStats(jailName string, includeIPs bool) (JailStats, error) {
 	stats := JailStats{Name: jailName}
 
-	cmd := exec.Command("fail2ban-client", "status", jailName)
+	if !validJailName.MatchString(jailName) {
+		return stats, fmt.Errorf("invalid jail name: %q", jailName)
+	}
+
+	cmd := exec.Command("fail2ban-client", "status", "--", jailName)
 	output, err := cmd.Output()
 	if err != nil {
 		return stats, err

gearbox-agent/internal/gears/security/fail2ban_test.go

@@ -1,6 +1,7 @@
 package security
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -104,3 +105,37 @@ func TestJailStatsFields(t *testing.T) {
 		t.Errorf("BannedIPs len = %d, want 2", len(stats.BannedIPs))
 	}
 }
+
+// 2026-05 audit P1-3: jail names that could be interpreted as fail2ban-client
+// flags must be rejected before they reach exec.Command. Today the only flow
+// is from getJails() (which parses fail2ban-client's own output), but this
+// pins down the validation in case a future code path exposes jail names to
+// API input.
+func TestValidJailName(t *testing.T) {
+	tests := []struct {
+		name string
+		want bool
+	}{
+		{"sshd", true},
+		{"haproxy-http", true},
+		{"my_jail", true},
+		{"jail-1", true},
+		{"a", true},
+
+		{"", false},
+		{"--help", false},
+		{"-h", false},
+		{"jail with space", false},
+		{"jail\nwith-newline", false},
+		{"jail;rm -rf /", false},
+		{"jail$(whoami)", false},
+		{strings.Repeat("a", 65), false}, // length cap
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := validJailName.MatchString(tt.name); got != tt.want {
+				t.Errorf("validJailName.MatchString(%q) = %v, want %v", tt.name, got, tt.want)
+			}
+		})
+	}
+}