fix(P02): input validation + injection hardening — 11 vectors (REQ-150)
Critical fixes:
- logs --job: validate ^[A-Za-z0-9_-]+$ + shellQuote (was %q backtick RCE)
- pprof: isLoopback treats empty host as bind-all (was :6060 bypass)
- backup restore: filepath.Rel containment check (was tar-slip via a/../..)
- WebAuthn reg auth deferred to P04 (requires session infra)
High fixes:
- txn rollback/show/apply: validate ^T-[0-9a-f]{16}$ + shellQuote
- nft diff --against: validate txn ID before filepath.Join
- drain stopAlloc: validate allocID ^[A-Za-z0-9_-]+$
- cluster_compat: shellQuote peer dir name
- podman image: shellQuote (was %q backtick injection)
- nft TrustedProbes: net.ParseIP/CIDR validation + split v4/v6 sets
- sudoers: validate --proxmox-user/--proxmox-role ^[a-zA-Z_][a-zA-Z0-9_-]{0,31}$
fixed path /etc/sudoers.d/orca; shellQuote pveum/useradd; validateSudoers
checks actual file
- nft country block: validate ^[A-Z]{2}$ (was len==2 only)
New file: internal/cli/validate.go (shared validators + shellQuote)
All 38 Go test packages pass. go vet + gofmt clean.
---ci---
project: orca
phase: 2
milestone: v0.13
status: complete
requirements:
covered: [150]
---/ci---
This commit is contained in:
@@ -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, "'", "'\\''") + "'"
|
||||
}
|
||||
|
||||
|
||||
+12
-4
@@ -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-<id>.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 <name>-migrated-<timestamp>.`,
|
||||
}
|
||||
|
||||
result := map[string]any{
|
||||
"job": jobName,
|
||||
"target": target.Name,
|
||||
"job": jobName,
|
||||
"target": target.Name,
|
||||
"already_on_target": len(onTarget) > 0,
|
||||
}
|
||||
|
||||
|
||||
+10
-1
@@ -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)
|
||||
|
||||
+14
-6
@@ -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 <txn-id> 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
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
+15
-1
@@ -84,6 +84,11 @@ Cluster-wide txns (no --namespace) require --force +
|
||||
scoped txns (--namespace <ns>) 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 <dir>/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))
|
||||
|
||||
@@ -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, "'", "'\\''") + "'"
|
||||
}
|
||||
Reference in New Issue
Block a user