diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 1f9e834..b011978 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -299,10 +299,16 @@ func Restore(opts RestoreOptions) error { return fmt.Errorf("restore: read tar entry: %w", err) } name := filepath.FromSlash(hdr.Name) - if strings.HasPrefix(name, "/") || strings.HasPrefix(name, "..") { - return fmt.Errorf("restore: unsafe path %q", hdr.Name) - } + // F3: tar-slip containment check. The prior prefix check + // (HasPrefix "/" || "..") missed patterns like "a/../../etc". + // Resolve the destination and verify it stays within target + // via filepath.Rel; reject if the relative path escapes (starts + // with ".." or is absolute). dest := filepath.Join(target, name) + rel, err := filepath.Rel(target, dest) + if err != nil || strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) { + return fmt.Errorf("restore: unsafe path %q escapes target (F3: tar-slip)", hdr.Name) + } switch hdr.Typeflag { case tar.TypeDir: if err := os.MkdirAll(dest, os.FileMode(hdr.Mode)); err != nil { diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 92e0a19..4afb71c 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -391,6 +391,71 @@ func TestRestoreRejectsTraversalSymlink(t *testing.T) { } } +// createCraftedTarballWithFile creates a tar.gz containing a single +// regular file entry with the given (possibly malicious) name. Used to +// test the tar-slip path-traversal guard (F3). +func createCraftedTarballWithFile(path, name, body string) error { + f, err := os.Create(path) + if err != nil { + return err + } + defer f.Close() + gz := gzip.NewWriter(f) + defer gz.Close() + tw := tar.NewWriter(gz) + defer tw.Close() + hdr := &tar.Header{ + Name: name, + Typeflag: tar.TypeReg, + Mode: 0o644, + Size: int64(len(body)), + } + if err := tw.WriteHeader(hdr); err != nil { + return err + } + if _, err := tw.Write([]byte(body)); err != nil { + return err + } + return nil +} + +// TestRestoreRejectsTarSlipRegularFile verifies a tarball with a regular +// file entry whose name contains an embedded ".." traversal (e.g. +// "a/../../etc/passwd") is rejected. The old prefix-only check missed +// this pattern; the F3 filepath.Rel containment check catches it. +func TestRestoreRejectsTarSlipRegularFile(t *testing.T) { + dir := t.TempDir() + tarPath := filepath.Join(dir, "slip.tar.gz") + sigPath := tarPath + ".sig" + if err := createCraftedTarballWithFile(tarPath, "a/../../etc/passwd", "pwned"); err != nil { + t.Fatalf("create tarball: %v", err) + } + key := make([]byte, 32) + for i := range key { + key[i] = byte(i + 9) + } + mac := hmac.New(sha256.New, key) + data, _ := os.ReadFile(tarPath) + mac.Write(data) + if err := os.WriteFile(sigPath, []byte(hex.EncodeToString(mac.Sum(nil))), 0o600); err != nil { + t.Fatalf("write sig: %v", err) + } + target := filepath.Join(dir, "restore") + os.MkdirAll(target, 0o755) + err := Restore(RestoreOptions{ + InputPath: tarPath, + TargetDir: target, + MasterKey: key, + Force: true, + }) + if err == nil { + t.Fatal("Restore should reject tar-slip regular file (F3)") + } + if !strings.Contains(err.Error(), "unsafe path") { + t.Errorf("error should mention unsafe path: %v", err) + } +} + // createCraftedTarball creates a tar.gz containing a single symlink // entry with the given linkname. Used to test symlink validation. func createCraftedTarball(path, name, linkname string) error { diff --git a/internal/cli/cluster_compat.go b/internal/cli/cluster_compat.go index 84e6ba0..b46a9c9 100644 --- a/internal/cli/cluster_compat.go +++ b/internal/cli/cluster_compat.go @@ -36,9 +36,9 @@ if any peer fails.`, } type noOrcaPeerResult struct { - Node string `json:"node"` - Peer string `json:"peer"` - Pass bool `json:"pass"` + Node string `json:"node"` + Peer string `json:"peer"` + Pass bool `json:"pass"` Violations []string `json:"violations,omitempty"` } @@ -192,12 +192,12 @@ Reports: which peers are on which version, any compatibility issues.`, } type compatPeerResult struct { - Node string `json:"node"` - Peer string `json:"peer"` - Version string `json:"version"` - LeadVersion string `json:"lead_version,omitempty"` - Compatible bool `json:"compatible"` - Issue string `json:"issue,omitempty"` + Node string `json:"node"` + Peer string `json:"peer"` + Version string `json:"version"` + LeadVersion string `json:"lead_version,omitempty"` + Compatible bool `json:"compatible"` + Issue string `json:"issue,omitempty"` } func runCompatCheck(cmd *cobra.Command) error { @@ -261,13 +261,13 @@ func runCompatCheck(cmd *cobra.Command) error { } summary := map[string]any{ - "lead_version": leadVersion, - "schema_version": emit.SchemaVersion, - "results": results, - "versions_seen": versionSet, - "issues": issues, - "schema_ok": schemaOK, - "manifest_ok": manifestOK, + "lead_version": leadVersion, + "schema_version": emit.SchemaVersion, + "results": results, + "versions_seen": versionSet, + "issues": issues, + "schema_ok": schemaOK, + "manifest_ok": manifestOK, } if jsonOutput { @@ -396,7 +396,10 @@ func verifyTxnManifestCompat(ctx context.Context, ex drainExecer, nodes []*model if first == "" { continue } - man, err := ex.Exec(ctx, peer, fmt.Sprintf("cat /etc/orca/cluster/txns/%s/manifest.json 2>/dev/null || true", first)) + // F7: first is a directory name parsed from remote `ls` output + // and is therefore attacker-controlled (stored injection from a + // malicious peer). Shell-quote it before interpolation. + man, err := ex.Exec(ctx, peer, fmt.Sprintf("cat /etc/orca/cluster/txns/%s/manifest.json 2>/dev/null || true", sshQuote(first))) if err != nil { continue } @@ -414,4 +417,3 @@ func verifyTxnManifestCompat(ctx context.Context, ex drainExecer, nodes []*model func sshQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" } - diff --git a/internal/cli/drain.go b/internal/cli/drain.go index 5570c28..a380dcf 100644 --- a/internal/cli/drain.go +++ b/internal/cli/drain.go @@ -74,8 +74,8 @@ func splitHostPort(addr string) (string, string, bool) { } var ( - drainTimeout time.Duration - migrateTarget string + drainTimeout time.Duration + migrateTarget string ) // allocUnit is the systemd unit name pattern for orca allocations. @@ -128,7 +128,15 @@ func listRunningAllocs(ctx context.Context, ex drainExecer, peer string) ([]stri // stopAlloc sends `systemctl stop orca-alloc-.service` to a node. // A unit that is already stopped (or never existed) is treated as // success: drain is idempotent. +// +// F6: allocID is parsed from remote `systemctl list-units` output and is +// therefore attacker-controlled (a malicious peer could emit a crafted +// unit name). Validate against ^[A-Za-z0-9_-]+$ before interpolation into +// the shell command to prevent stored command injection. func stopAlloc(ctx context.Context, ex drainExecer, peer, allocID string) error { + if !validSafeName(allocID) { + return fmt.Errorf("stopAlloc: invalid alloc id %q (allowed: A-Z a-z 0-9 _ -)", allocID) + } cmd := fmt.Sprintf("systemctl stop %s", allocUnit(allocID)) _, err := ex.Exec(ctx, peer, cmd) if err != nil { @@ -554,8 +562,8 @@ is named -migrated-.`, } result := map[string]any{ - "job": jobName, - "target": target.Name, + "job": jobName, + "target": target.Name, "already_on_target": len(onTarget) > 0, } diff --git a/internal/cli/logs.go b/internal/cli/logs.go index 6d7a987..611bdae 100644 --- a/internal/cli/logs.go +++ b/internal/cli/logs.go @@ -128,6 +128,13 @@ Ctrl-C cancels the fan-out via signal.NotifyContext.`, if logsAllNodes && logsNode != "" { return fmt.Errorf("--all-nodes and --node are mutually exclusive") } + // F1: validate --job before interpolation into the journalctl + // unit pattern. Go's %q does not escape backticks and bash + // executes command substitution inside double quotes, so an + // unvalidated job name is a remote RCE vector. + if logsJob != "" && !validSafeName(logsJob) { + return fmt.Errorf("logs: --job %q contains disallowed characters (allowed: A-Z a-z 0-9 _ -)", logsJob) + } since, err := parseSince(logsSince) if err != nil { return err @@ -271,7 +278,9 @@ func streamNodeLines(ctx context.Context, ex logsExecer, n *model.Node, since ti unitPattern = "orca-alloc-" + job + "-*" } sinceStr := since.Format("2006-01-02 15:04:05") - cmd := fmt.Sprintf("journalctl -u %q --since %q --output json --no-pager", unitPattern, sinceStr) + // F1: shellQuote (single-quote wrap) instead of %q — %q does not + // escape backticks, enabling command substitution in double quotes. + cmd := fmt.Sprintf("journalctl -u %s --since %s --output json --no-pager", shellQuote(unitPattern), shellQuote(sinceStr)) raw, err := ex.Exec(ctx, peer, cmd) if err != nil { slog.Default().Warn("logs: exec failed", "node", n.Name, "peer", peer, "error", err) diff --git a/internal/cli/nft.go b/internal/cli/nft.go index dabb90c..30cf8de 100644 --- a/internal/cli/nft.go +++ b/internal/cli/nft.go @@ -22,9 +22,9 @@ import ( ) var ( - nftShowPeer string - nftDiffAgainst string - nftRateLimitRate int + nftShowPeer string + nftDiffAgainst string + nftRateLimitRate int nftCountryBlockCC string ) @@ -70,6 +70,11 @@ recorded at apply time). Reports per-rule diffs.`, if nftDiffAgainst == "" { return errors.New("nft diff: --against is required") } + // F5: validate --against txn ID before interpolation into a + // filesystem path (filepath.Join(paths.TxnDir(), txnID, ...)). + if !validTxnID(nftDiffAgainst) { + return fmt.Errorf("nft diff: --against %q is not a valid txn id (expected T-[0-9a-f]{16})", nftDiffAgainst) + } t, err := nftTransportFromCtx() if err != nil { return fmt.Errorf("nft transport: %w", err) @@ -132,8 +137,12 @@ orca nft country block add RU,CN`, } codes := strings.Split(ccList, ",") for _, c := range codes { - if len(c) != 2 { - return fmt.Errorf("nft country block add: %q is not a 2-letter country code", c) + // F11: validate against ^[A-Z]{2}$ (two uppercase ASCII letters), + // not just len==2. The old check accepted arbitrary 2-byte + // strings (e.g. "RU" but also "; " or "$(") which could inject + // nft syntax or shell metacharacters. + if !validCountryCode(c) { + return fmt.Errorf("nft country block add: %q is not a valid ISO-3166 alpha-2 country code (expected two uppercase letters)", c) } } t, err := nftTransportFromCtx() @@ -263,4 +272,3 @@ func quoteAll(in []string) []string { } return out } - diff --git a/internal/cli/nft_test.go b/internal/cli/nft_test.go index b14e753..b5816d7 100644 --- a/internal/cli/nft_test.go +++ b/internal/cli/nft_test.go @@ -96,7 +96,7 @@ func TestNftDiffCmd_NoDrift(t *testing.T) { var buf bytes.Buffer rootCmd.SetOut(&buf) rootCmd.SetErr(&buf) - rootCmd.SetArgs([]string{"nft", "diff", "--against", "txn-123"}) + rootCmd.SetArgs([]string{"nft", "diff", "--against", "T-abcdef0123456789"}) if err := rootCmd.Execute(); err != nil { t.Fatalf("nft diff: %v", err) } diff --git a/internal/cli/txn.go b/internal/cli/txn.go index e285874..2dd2ee8 100644 --- a/internal/cli/txn.go +++ b/internal/cli/txn.go @@ -84,6 +84,11 @@ Cluster-wide txns (no --namespace) require --force + scoped txns (--namespace ) only touch that namespace.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // F4: validate txn ID before interpolation into a remote shell + // command and filesystem path. + if !validTxnID(args[0]) { + return fmt.Errorf("txn apply: invalid txn id %q (expected T-[0-9a-f]{16})", args[0]) + } id := txn.TxnID(args[0]) transport, err := txnTransportFromCtx() if err != nil { @@ -175,6 +180,10 @@ var txnShowCmd = &cobra.Command{ Short: "Show txn details (desired state, manifest, status)", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // F4: validate txn ID before interpolation into a filesystem path. + if !validTxnID(args[0]) { + return fmt.Errorf("txn show: invalid txn id %q (expected T-[0-9a-f]{16})", args[0]) + } id := args[0] dir := filepath.Join(paths.TxnDir(), id) manifestPath := filepath.Join(dir, "manifest.json") @@ -230,6 +239,11 @@ the manual rollback path; orca-pull.sh runs rollback automatically on verify failure.`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // F4: validate txn ID before interpolation into a remote shell + // command (bash /rollback.sh) and filesystem path. + if !validTxnID(args[0]) { + return fmt.Errorf("txn rollback: invalid txn id %q (expected T-[0-9a-f]{16})", args[0]) + } id := txn.TxnID(args[0]) transport, err := txnTransportFromCtx() if err != nil { @@ -237,7 +251,7 @@ verify failure.`, } ctx := cmd.Context() dir := "/run/orca/txns/" + string(id) - cmdStr := fmt.Sprintf("bash %s/rollback.sh", dir) + cmdStr := fmt.Sprintf("bash %s/rollback.sh", shellQuote(dir)) out, err := transport.Exec(ctx, txnRollbackLead, cmdStr) if err != nil { return fmt.Errorf("rollback %s on %s: %w (output: %s)", id, txnRollbackLead, err, string(out)) diff --git a/internal/cli/validate.go b/internal/cli/validate.go new file mode 100644 index 0000000..c920f77 --- /dev/null +++ b/internal/cli/validate.go @@ -0,0 +1,59 @@ +// Package cli: validate.go provides shared input-validation helpers for +// CLI command arguments that are interpolated into remote shell commands +// or filesystem paths (Phase 02 injection hardening, v0.13). +// +// These helpers enforce strict allowlists so that attacker-controlled +// values (job names, txn IDs, alloc IDs, country codes) cannot reach +// shell interpolation or path joins without matching a known-safe shape. +package cli + +import ( + "regexp" + "strings" +) + +// safeNameRe matches the allowlist for shell-interpolated identifiers +// (job names, alloc IDs): ASCII letters, digits, underscore, hyphen. +// Used to prevent backtick/command-substitution and metacharacter +// injection into remote shell commands. +var safeNameRe = regexp.MustCompile(`^[A-Za-z0-9_-]+$`) + +// txnIDRe matches the canonical orca transaction ID format: "T-" prefix +// followed by exactly 16 lowercase hex digits. Used to validate txn IDs +// before they are interpolated into filesystem paths or remote shell +// commands (`orca txn rollback`, `orca nft diff --against`). +var txnIDRe = regexp.MustCompile(`^T-[0-9a-f]{16}$`) + +// countryCodeRe matches ISO-3166 alpha-2 country codes: exactly two +// uppercase ASCII letters. Used by `orca nft country block add` before +// codes are interpolated into the nft ruleset. +var countryCodeRe = regexp.MustCompile(`^[A-Z]{2}$`) + +// validSafeName reports whether s is a safe shell-interpolation +// identifier (ASCII alphanumeric, underscore, hyphen only, non-empty). +func validSafeName(s string) bool { + return safeNameRe.MatchString(s) +} + +// validTxnID reports whether s matches the canonical orca txn ID format +// (^T-[0-9a-f]{16}$). +func validTxnID(s string) bool { + return txnIDRe.MatchString(s) +} + +// validCountryCode reports whether s is a valid ISO-3166 alpha-2 code +// (two uppercase letters). +func validCountryCode(s string) bool { + return countryCodeRe.MatchString(s) +} + +// shellQuote single-quotes a string for safe shell interpolation over +// SSH exec. It escapes embedded single-quotes via the standard '\” idiom +// (POSIX shell). This is the cli-package copy of the helper duplicated +// across runtime/identity/stepca/sshpush to avoid import cycles; it +// hardens command interpolation against backtick/command-substitution +// injection (Go's %q does NOT escape backticks, and bash executes +// command substitution inside double quotes). +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" +} diff --git a/internal/daemon/pprof.go b/internal/daemon/pprof.go index ab88484..7343c9f 100644 --- a/internal/daemon/pprof.go +++ b/internal/daemon/pprof.go @@ -13,13 +13,21 @@ import ( // isLoopback reports whether the address binds to a loopback interface // (127.0.0.1, ::1, localhost). REQ-123: pprof must be loopback-only. +// +// An empty host (e.g. ":6060") binds ALL interfaces and is therefore +// treated as NON-loopback (F2: loopback-bypass fix). Only an explicit +// loopback IP or the "localhost" name is accepted. func isLoopback(addr string) bool { host, _, err := net.SplitHostPort(addr) if err != nil { host = addr } host = strings.TrimSpace(host) - if host == "" || host == "localhost" { + // F2: empty host (":6060") binds all interfaces — reject. + if host == "" { + return false + } + if host == "localhost" { return true } ip := net.ParseIP(host) @@ -29,18 +37,22 @@ func isLoopback(addr string) bool { return false } +// StartPprof starts the pprof HTTP server on addr. REQ-123: pprof is +// unauthenticated and MUST bind to a loopback interface only; this is a +// hard invariant (F2: the --pprof-allow-public override was a phantom flag +// that was never implemented and has been removed — non-loopback binds are +// always refused). func StartPprof(addr string, log *slog.Logger) (*http.Server, error) { if addr == "" { return nil, nil } - // REQ-123: pprof must bind to loopback only. Non-loopback addresses - // require explicit --pprof-allow-public confirmation (which the CLI - // passes after a warning). We refuse non-loopback here by default. + // REQ-123: pprof must bind to loopback only. This is a hard + // invariant; there is no public-bind override. if !isLoopback(addr) { log.Error("pprof refuses non-loopback bind", slog.String("addr", addr), - slog.String("reason", "REQ-123: pprof is unauthenticated; use --pprof-allow-public to override (operator-only)")) - return nil, fmt.Errorf("pprof: refusing non-loopback bind %s (REQ-123; unauthenticated; use --pprof-allow-public)", addr) + slog.String("reason", "REQ-123: pprof is unauthenticated; loopback-only is a hard invariant")) + return nil, fmt.Errorf("pprof: refusing non-loopback bind %s (REQ-123; unauthenticated; loopback-only is a hard invariant)", addr) } mux := http.NewServeMux() mux.HandleFunc("/debug/pprof/", pprof.Index) diff --git a/internal/daemon/pprof_test.go b/internal/daemon/pprof_test.go index 23332d0..e557207 100644 --- a/internal/daemon/pprof_test.go +++ b/internal/daemon/pprof_test.go @@ -7,6 +7,7 @@ import ( "net" "net/http" "path/filepath" + "strings" "testing" "time" @@ -286,3 +287,16 @@ func TestStartPprof_LoopbackAccepted(t *testing.T) { srv.Close() } } + +// TestStartPprof_EmptyHostRefused verifies that an address with an empty +// host (e.g. ":6060"), which binds ALL interfaces, is refused as +// non-loopback (F2: loopback-bypass fix). +func TestStartPprof_EmptyHostRefused(t *testing.T) { + _, err := StartPprof(":6060", slog.Default()) + if err == nil { + t.Error("StartPprof on \":6060\" should be refused (F2: empty host binds all interfaces)") + } + if err != nil && !strings.Contains(err.Error(), "non-loopback") { + t.Errorf("error should mention non-loopback, got: %v", err) + } +} diff --git a/internal/emitter/nft.go b/internal/emitter/nft.go index 872e876..90916be 100644 --- a/internal/emitter/nft.go +++ b/internal/emitter/nft.go @@ -13,7 +13,8 @@ // The ruleset defines: // // - table inet orca-ingress -// - set orca_trusted_probes (ipv4_addr interval, default 127.0.0.1) +// - set orca_trusted_probes_v4 (ipv4_addr interval, default 127.0.0.1) +// - set orca_trusted_probes_v6 (ipv6_addr interval, default ::1) // - input chain (SYN-flood filter on :443) // - prerouting chain (DNAT :443->127.0.0.1:8443, :80->127.0.0.1:8080) // - forward chain (rate-limit meter on :443) @@ -25,6 +26,7 @@ package emitter import ( "errors" "fmt" + "net" "strings" ) @@ -39,9 +41,11 @@ const nftConfigPath = "/etc/nftables.d/orca.nft" // All fields have safe defaults so a zero-value config renders a // working ruleset. type NftClusterConfig struct { - // TrustedProbes is the list of source IPs exempt from the + // TrustedProbes is the list of source IPs/CIDRs exempt from the // SYN-flood filter and rate-limit (monitoring probes, the orca - // lead itself). Defaults to [127.0.0.1, ::1]. + // lead itself). Defaults to [127.0.0.1, ::1]. Each entry must + // parse as a valid IP or CIDR via net.ParseIP / net.ParseCIDR or + // RenderNftConfig returns an error (F9: ruleset injection guard). TrustedProbes []string // RateLimit is the per-source rate limit (packets/second) for the // forward-chain meter on :443. Defaults to 100. @@ -73,31 +77,79 @@ func (c NftClusterConfig) withDefaults() NftClusterConfig { // `#!/usr/sbin/nft -f` shebang (so `nft -f` applies it and so a // drift-check `nft -c -f` validates the syntax). // -// Returns an error only when the config is internally inconsistent -// (e.g. a negative rate, which the defaults already prevent). +// Returns an error when the config is internally inconsistent (e.g. a +// negative rate, which the defaults already prevent) or when a +// TrustedProbes entry fails to parse as an IP or CIDR (F9: ruleset +// injection hardening — unvalidated entries are written directly into +// the nft ruleset and could inject arbitrary nft syntax). func (NftEmitter) RenderNftConfig(clusterConfig NftClusterConfig) ([]File, error) { if clusterConfig.RateLimit < 0 || clusterConfig.RateBurst < 0 { return nil, errors.New("emitter/nft: rate/burst must be non-negative") } cfg := clusterConfig.withDefaults() - content := renderNftRuleset(cfg) + // F9: validate every TrustedProbes entry before rendering. An + // invalid entry is rejected with an error rather than written raw + // into the ruleset (which would allow nft-syntax injection). + v4, v6, err := partitionTrustedProbes(cfg.TrustedProbes) + if err != nil { + return nil, err + } + content := renderNftRuleset(cfg, v4, v6) return []File{{Path: nftConfigPath, Content: content, Mode: "0644"}}, nil } +// partitionTrustedProbes validates each entry as an IP or CIDR and +// partitions the list into IPv4 and IPv6 slices. Returns an error if +// any entry is neither a valid IP nor a valid CIDR (F9). +func partitionTrustedProbes(probes []string) (v4, v6 []string, err error) { + for _, p := range probes { + if p == "" { + return nil, nil, fmt.Errorf("emitter/nft: empty trusted probe entry (F9: ruleset injection guard)") + } + if ip := net.ParseIP(p); ip != nil { + if ip.To4() != nil { + v4 = append(v4, p) + } else { + v6 = append(v6, p) + } + continue + } + if _, _, cidrErr := net.ParseCIDR(p); cidrErr == nil { + // Determine address family from the CIDR prefix. + ip := net.ParseIP(strings.Split(p, "/")[0]) + if ip != nil && ip.To4() != nil { + v4 = append(v4, p) + } else { + v6 = append(v6, p) + } + continue + } + return nil, nil, fmt.Errorf("emitter/nft: trusted probe %q is not a valid IP or CIDR (F9: ruleset injection guard)", p) + } + return v4, v6, nil +} + // renderNftRuleset builds the nft ruleset string. The shape is // documented in the package comment; the exact lines are load-bearing // for `orca doctor nft` (which greps the live table for them) and for // `nft -c -f` (which parses the syntax). -func renderNftRuleset(cfg NftClusterConfig) string { +// +// F9: TrustedProbes are split into separate ipv4_addr and ipv6_addr +// sets (orca_trusted_probes_v4 / orca_trusted_probes_v6) because the +// prior single ipv4_addr set included ::1 (an IPv6 address), which is +// a type mismatch nft rejects. +func renderNftRuleset(cfg NftClusterConfig, v4, v6 []string) string { var b strings.Builder b.WriteString("#!/usr/sbin/nft -f\n\n") b.WriteString("flush table inet orca-ingress\n\n") b.WriteString("table inet orca-ingress {\n") - b.WriteString("\tset orca_trusted_probes {\n") + + // F9: split IPv4 and IPv6 trusted probes into separate typed sets. + b.WriteString("\tset orca_trusted_probes_v4 {\n") b.WriteString("\t\ttype ipv4_addr\n") b.WriteString("\t\tflags interval\n") b.WriteString("\t\telements = { ") - for i, p := range cfg.TrustedProbes { + for i, p := range v4 { if i > 0 { b.WriteString(", ") } @@ -105,6 +157,20 @@ func renderNftRuleset(cfg NftClusterConfig) string { } b.WriteString(" }\n") b.WriteString("\t}\n\n") + + b.WriteString("\tset orca_trusted_probes_v6 {\n") + b.WriteString("\t\ttype ipv6_addr\n") + b.WriteString("\t\tflags interval\n") + b.WriteString("\t\telements = { ") + for i, p := range v6 { + if i > 0 { + b.WriteString(", ") + } + b.WriteString(p) + } + b.WriteString(" }\n") + b.WriteString("\t}\n\n") + b.WriteString("\tchain input {\n") b.WriteString("\t\ttype filter hook input priority filter; policy accept;\n") b.WriteString("\t\tct state invalid drop\n") diff --git a/internal/emitter/nft_test.go b/internal/emitter/nft_test.go index eb58c9d..5c03a5c 100644 --- a/internal/emitter/nft_test.go +++ b/internal/emitter/nft_test.go @@ -90,3 +90,48 @@ func TestNftEmitter_ShebangFirst(t *testing.T) { t.Errorf("shebang not first:\n%s", files[0].Content[:40]) } } + +// TestNftEmitter_RejectsInvalidTrustedProbe verifies that a TrustedProbes +// entry that is not a valid IP or CIDR is rejected (F9: ruleset injection +// guard). An unvalidated entry written raw into the ruleset could inject +// arbitrary nft syntax. +func TestNftEmitter_RejectsInvalidTrustedProbe(t *testing.T) { + bad := []string{ + "not-an-ip", + "127.0.0.1; flush ruleset", + "$(whoami)", + "10.0.0.0/33", // invalid CIDR prefix + } + for _, b := range bad { + _, err := (NftEmitter{}).RenderNftConfig(NftClusterConfig{TrustedProbes: []string{"127.0.0.1", b}}) + if err == nil { + t.Errorf("expected error for invalid trusted probe %q, got nil", b) + } + } +} + +// TestNftEmitter_TrustedProbesSplitV4V6 verifies that IPv4 and IPv6 +// probes are rendered into separate typed sets (F9: the prior single +// ipv4_addr set included ::1, an IPv6 address — a type mismatch). +func TestNftEmitter_TrustedProbesSplitV4V6(t *testing.T) { + files, err := (NftEmitter{}).RenderNftConfig(NftClusterConfig{TrustedProbes: []string{"10.0.0.5", "::1"}}) + if err != nil { + t.Fatalf("Render: %v", err) + } + c := files[0].Content + if !strings.Contains(c, "set orca_trusted_probes_v4") { + t.Errorf("missing v4 set:\n%s", c) + } + if !strings.Contains(c, "set orca_trusted_probes_v6") { + t.Errorf("missing v6 set:\n%s", c) + } + if !strings.Contains(c, "type ipv6_addr") { + t.Errorf("missing ipv6_addr type:\n%s", c) + } + if !strings.Contains(c, "10.0.0.5") { + t.Errorf("missing 10.0.0.5 in v4 set:\n%s", c) + } + if !strings.Contains(c, "::1") { + t.Errorf("missing ::1 in v6 set:\n%s", c) + } +} diff --git a/internal/proxmox/bootstrap.go b/internal/proxmox/bootstrap.go index ca75b10..cc1578e 100644 --- a/internal/proxmox/bootstrap.go +++ b/internal/proxmox/bootstrap.go @@ -29,6 +29,7 @@ import ( "log/slog" "net" "os" + "regexp" "strings" "time" @@ -118,6 +119,18 @@ func BootstrapProxmox(ctx context.Context, opts Options) (*Result, error) { if opts.SSHPort == 0 { opts.SSHPort = DefaultSSHPort } + // F10: validate ProxmoxUser and ProxmoxRole before they are + // interpolated into sudoers content, file paths, and shell commands + // (useradd, pveum). An attacker-controlled value could inject shell + // metacharacters or path traversal. Allowlist: lowercase letter or + // underscore start, followed by lowercase alphanumerics, underscore, + // or hyphen; max 32 chars. + if !validProxmoxName(opts.ProxmoxUser) { + return nil, fmt.Errorf("proxmox bootstrap: invalid ProxmoxUser %q (allowed: ^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$)", opts.ProxmoxUser) + } + if !validProxmoxName(opts.ProxmoxRole) { + return nil, fmt.Errorf("proxmox bootstrap: invalid ProxmoxRole %q (allowed: ^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$)", opts.ProxmoxRole) + } log := opts.Logger if log == nil { log = slog.Default() @@ -392,7 +405,8 @@ func deployPubKey(user, pubLine string) error { // createLinuxUser creates the orca system user if it doesn't already // exist. Idempotent: `id -u` check before `useradd`. func createLinuxUser(user string) error { - cmd := fmt.Sprintf("id -u %s 2>/dev/null || useradd -r -s /usr/sbin/nologin %s", user, user) + // F10c: shellQuote the user (validated upstream, but defense-in-depth). + cmd := fmt.Sprintf("id -u %s 2>/dev/null || useradd -r -s /usr/sbin/nologin %s", shellQuote(user), shellQuote(user)) if _, err := runRemote(cmd); err != nil { return err } @@ -402,9 +416,10 @@ func createLinuxUser(user string) error { // createPVERole creates the OrcaOperator PVE role if it doesn't exist. // Idempotent: probes `pveum role list` before `pveum role add`. func createPVERole(role string) error { + // F10c: shellQuote the role (validated upstream, but defense-in-depth). cmd := fmt.Sprintf( "pveum role list 2>/dev/null | grep -q '^%s' || pveum role add %s --privs '%s'", - role, role, OrcaOperatorPrivileges, + shellQuote(role), shellQuote(role), OrcaOperatorPrivileges, ) if _, err := runRemote(cmd); err != nil { return err @@ -417,9 +432,11 @@ func createPVERole(role string) error { // Uses @pam realm (AD-019) since orca creates a Linux system user. func createPVEUser(user string) error { pveUserID := user + "@pam" + // F10c: shellQuote the PVE user id (validated upstream, but + // defense-in-depth). cmd := fmt.Sprintf( - "pveum user list 2>/dev/null | grep -q '%s' || pveum user add %s -comment 'Orca automation user'", - pveUserID, pveUserID, + "pveum user list 2>/dev/null | grep -q %s || pveum user add %s -comment 'Orca automation user'", + shellQuote(pveUserID), shellQuote(pveUserID), ) if _, err := runRemote(cmd); err != nil { return err @@ -431,7 +448,9 @@ func createPVEUser(user string) error { // (cluster-wide). `pveum acl modify` is idempotent (creates or updates). func assignPVEACL(user, role string) error { pveUserID := user + "@pam" - cmd := fmt.Sprintf("pveum acl modify / -user %s -role %s", pveUserID, role) + // F10c: shellQuote the PVE user id and role (validated upstream, + // but defense-in-depth). + cmd := fmt.Sprintf("pveum acl modify / -user %s -role %s", shellQuote(pveUserID), shellQuote(role)) if _, err := runRemote(cmd); err != nil { return err } @@ -454,13 +473,20 @@ func sudoersContent(user string) string { `, user, user) } +// sudoersPath is the fixed on-peer path for the orca sudoers drop-in. +// F10b: the file is always written here regardless of the configured +// ProxmoxUser name, so a crafted username cannot redirect the sudoers +// drop-in to an arbitrary path. +const sudoersPath = "/etc/sudoers.d/orca" + // writeSudoers writes the /etc/sudoers.d/orca file on the remote host -// with mode 0440. Uses a heredoc via cat to avoid quoting issues. +// with mode 0440. Uses a heredoc via cat to avoid quoting issues. F10b: +// the path is fixed (sudoersPath) regardless of the configured username. func writeSudoers(user string) error { content := sudoersContent(user) - // Write via cat heredoc, then chmod 0440. - cmd := fmt.Sprintf("cat > /etc/sudoers.d/%s <<'ORCA_SUDOERS_EOF'\n%s\nORCA_SUDOERS_EOF\nchmod 0440 /etc/sudoers.d/%s", - user, content, user) + // Write via cat heredoc to the fixed path, then chmod 0440. + cmd := fmt.Sprintf("cat > %s <<'ORCA_SUDOERS_EOF'\n%s\nORCA_SUDOERS_EOF\nchmod 0440 %s", + sudoersPath, content, sudoersPath) if _, err := runRemote(cmd); err != nil { return err } @@ -470,8 +496,14 @@ func writeSudoers(user string) error { // validateSudoers runs `visudo -cf` on the sudoers file. Aborts the // bootstrap if validation fails (prevents a broken sudoers from // locking the orca user out of sudo). +// validateSudoers runs `visudo -cf` on the sudoers file. F10d: it +// validates the actual file that writeSudoers wrote (sudoersPath, +// /etc/sudoers.d/orca), which is now a fixed path — the prior version +// hardcoded /etc/sudoers.d/orca while writeSudoers wrote to +// /etc/sudoers.d/, so a custom username would validate the +// wrong file. func validateSudoers() error { - cmd := "visudo -cf /etc/sudoers.d/orca" + cmd := fmt.Sprintf("visudo -cf %s", sudoersPath) out, err := runRemote(cmd) if err != nil { return fmt.Errorf("visudo validation failed: %w (output: %s)", err, strings.TrimSpace(string(out))) @@ -482,6 +514,29 @@ func validateSudoers() error { return nil } +// proxmoxNameRe is the allowlist for ProxmoxUser and ProxmoxRole values +// that are interpolated into sudoers content, file paths, and shell +// commands (F10a). Letter or underscore start, followed by +// alphanumerics, underscore, or hyphen; max 32 chars. Uppercase is +// permitted (DefaultProxmoxRole is "OrcaOperator"); shell +// metacharacters (spaces, ;, $, backticks, etc.) are blocked. +var proxmoxNameRe = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$`) + +// validProxmoxName reports whether s is a safe ProxmoxUser or ProxmoxRole +// value (F10a injection guard). +func validProxmoxName(s string) bool { + return proxmoxNameRe.MatchString(s) +} + +// shellQuote single-quotes a string for safe shell interpolation over +// the SSH exec session. It escapes embedded single-quotes via the +// standard ”' idiom (POSIX shell). F10c: hardens pveum/useradd commands +// against metacharacter injection (the validated allowlist is +// defense-in-depth on top of this). +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" +} + // ResetHostKey removes all known_hosts entries for the given host from // certpaths.KnownHostsPath() (REQ-059, D-046, AD-029). It rewrites the // file atomically via security.WriteAtomic. LOCAL ONLY — it does NOT diff --git a/internal/proxmox/bootstrap_test.go b/internal/proxmox/bootstrap_test.go index 8499ae1..0fcc826 100644 --- a/internal/proxmox/bootstrap_test.go +++ b/internal/proxmox/bootstrap_test.go @@ -96,6 +96,42 @@ func TestBootstrapProxmox_Validation(t *testing.T) { } } +// TestBootstrapProxmox_RejectsInvalidProxmoxUser verifies that a +// ProxmoxUser containing shell metacharacters is rejected before any +// SSH dial (F10a: sudoers/shell injection guard). +func TestBootstrapProxmox_RejectsInvalidProxmoxUser(t *testing.T) { + ctx := context.Background() + bad := []string{"orca; rm -rf /", "orca$(whoami)", "orca`id`", "orca user", "1orca"} + for _, b := range bad { + _, err := BootstrapProxmox(ctx, Options{Host: "10.0.0.1", SSHKeyPath: certpaths.SSHKeyPath(), ProxmoxUser: b}) + if err == nil { + t.Errorf("expected error for invalid ProxmoxUser %q, got nil", b) + continue + } + if !strings.Contains(err.Error(), "invalid ProxmoxUser") { + t.Errorf("error should mention invalid ProxmoxUser for %q, got: %v", b, err) + } + } +} + +// TestBootstrapProxmox_RejectsInvalidProxmoxRole verifies that a +// ProxmoxRole containing shell metacharacters is rejected before any +// SSH dial (F10a: sudoers/shell injection guard). +func TestBootstrapProxmox_RejectsInvalidProxmoxRole(t *testing.T) { + ctx := context.Background() + bad := []string{"role; flush", "role$(id)", "role`whoami`", "role name", "1role"} + for _, b := range bad { + _, err := BootstrapProxmox(ctx, Options{Host: "10.0.0.1", SSHKeyPath: certpaths.SSHKeyPath(), ProxmoxRole: b}) + if err == nil { + t.Errorf("expected error for invalid ProxmoxRole %q, got nil", b) + continue + } + if !strings.Contains(err.Error(), "invalid ProxmoxRole") { + t.Errorf("error should mention invalid ProxmoxRole for %q, got: %v", b, err) + } + } +} + func TestDefaultOptions(t *testing.T) { if DefaultProxmoxUser != "orca" { t.Errorf("DefaultProxmoxUser = %q, want orca", DefaultProxmoxUser) diff --git a/internal/runtime/podman.go b/internal/runtime/podman.go index f5a964b..80794c8 100644 --- a/internal/runtime/podman.go +++ b/internal/runtime/podman.go @@ -64,7 +64,7 @@ func (p *PodmanRuntime) Start(ctx context.Context, alloc *Alloc) (int, error) { } cmdStr, _ := commandFor(alloc) name := containerName(alloc) - cmd := fmt.Sprintf("podman run -d --name %s %q %s", shellQuote(name), image, shellQuote(cmdStr)) + cmd := fmt.Sprintf("podman run -d --name %s %s %s", shellQuote(name), shellQuote(image), shellQuote(cmdStr)) out, err := p.transport.Exec(ctx, alloc.Node, cmd) if err != nil { return 0, fmt.Errorf("podman: run: %w", err)