diff --git a/controller/internal/backup/offbox.go b/controller/internal/backup/offbox.go index 0236833..1ab8eaa 100644 --- a/controller/internal/backup/offbox.go +++ b/controller/internal/backup/offbox.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "time" @@ -108,13 +109,49 @@ func generateOffboxPassword() (string, error) { return hex.EncodeToString(b), nil } -// OffboxConfigured reports whether the target is set, enabled, and the key + password files exist (so the -// UI/scheduler can gate a run without leaking why). +// Off-box target field validation — the security boundary for the values that flow into the `ssh … -s +// sftp` command restic runs. Host/user are charset-restricted AND must not start with '-' (an ssh +// OPTION-INJECTION vector: a host like "-oProxyCommand=evil" would make ssh execute an arbitrary command). +// RepoPath is an absolute, traversal-free, metacharacter-free path. This mirrors the agent's validate.go +// discipline: validate before any value reaches an exec. +var ( + reOffboxHost = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) + reOffboxUser = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) + reOffboxPath = regexp.MustCompile(`^/[A-Za-z0-9._/-]+$`) +) + +// ValidateOffboxTarget rejects values that could inject into the ssh command line (option injection via a +// leading '-', shell/space metacharacters, path traversal). Returns nil for a safe target. +func ValidateOffboxTarget(t *settings.OffboxTarget) error { + if t == nil { + return fmt.Errorf("no off-box target") + } + if t.Host == "" || len(t.Host) > 255 || !reOffboxHost.MatchString(t.Host) || strings.HasPrefix(t.Host, "-") || strings.HasPrefix(t.Host, ".") { + return fmt.Errorf("invalid NAS host (letters, digits, '.', '-', '_'; must not start with '-' or '.')") + } + if t.User == "" || len(t.User) > 64 || !reOffboxUser.MatchString(t.User) || strings.HasPrefix(t.User, "-") { + return fmt.Errorf("invalid user (letters, digits, '.', '-', '_'; must not start with '-')") + } + if len(t.RepoPath) > 512 || !reOffboxPath.MatchString(t.RepoPath) || strings.Contains(t.RepoPath, "..") { + return fmt.Errorf("invalid repo path (absolute, no spaces/metacharacters, no '..')") + } + if p := t.Port; p != 0 && (p < 1 || p > 65535) { + return fmt.Errorf("invalid port") + } + return nil +} + +// OffboxConfigured reports whether the target is set, enabled, VALID, and the key + password files exist +// (so the UI/scheduler can gate a run without leaking why). A target that fails validation is treated as +// not-configured — fail-closed, so a bad/hostile persisted target can never reach the ssh exec. func (m *Manager) OffboxConfigured() bool { t := m.settings.GetOffboxTarget() if t == nil || !t.Enabled || t.Host == "" || t.User == "" || t.RepoPath == "" { return false } + if err := ValidateOffboxTarget(t); err != nil { + return false + } if _, err := os.Stat(m.offboxKeyPath()); err != nil { return false } diff --git a/controller/internal/backup/offbox_test.go b/controller/internal/backup/offbox_test.go index 30533d2..49bc5c1 100644 --- a/controller/internal/backup/offbox_test.go +++ b/controller/internal/backup/offbox_test.go @@ -265,6 +265,31 @@ func TestOffbox_SecretsAre0600(t *testing.T) { // --- tiny test helpers --- +// TestOffbox_ValidateRejectsInjection is the security companion: host/user/repo values that could inject +// an ssh option (leading '-' → e.g. -oProxyCommand) or a shell metacharacter must be REFUSED; a clean +// target is accepted. A build without this guard would let a hostile target reach the ssh exec → FAIL. +func TestOffbox_ValidateRejectsInjection(t *testing.T) { + ok := &settings.OffboxTarget{Host: "nas.local", Port: 22, User: "felhom", RepoPath: "/srv/repo"} + if err := ValidateOffboxTarget(ok); err != nil { + t.Fatalf("clean target rejected: %v", err) + } + bad := []settings.OffboxTarget{ + {Host: "-oProxyCommand=touch /tmp/pwn", User: "felhom", RepoPath: "/srv/repo"}, // ssh option injection + {Host: "nas;rm -rf /", User: "felhom", RepoPath: "/srv/repo"}, // metacharacters + {Host: "nas.local", User: "-oProxyCommand=x", RepoPath: "/srv/repo"}, // user option injection + {Host: "nas.local", User: "felhom", RepoPath: "/srv/repo; evil"}, // path metacharacters + {Host: "nas.local", User: "felhom", RepoPath: "/srv/../etc"}, // traversal + {Host: "nas local", User: "felhom", RepoPath: "/srv/repo"}, // space + {Host: "nas.local", User: "felhom", RepoPath: "relative/path"}, // non-absolute + } + for i, b := range bad { + bb := b + if err := ValidateOffboxTarget(&bb); err == nil { + t.Errorf("case %d (%+v) must be rejected", i, bb) + } + } +} + func runtimeIsUnix() bool { return os.PathSeparator == '/' } func contains(ss []string, want string) bool { diff --git a/controller/internal/web/offbox_handlers.go b/controller/internal/web/offbox_handlers.go index 105f684..a132414 100644 --- a/controller/internal/web/offbox_handlers.go +++ b/controller/internal/web/offbox_handlers.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "gitea.dooplex.hu/admin/felhom-controller/internal/backup" "gitea.dooplex.hu/admin/felhom-controller/internal/settings" ) @@ -50,6 +51,12 @@ func (s *Server) offboxConfigHandler(w http.ResponseWriter, r *http.Request) { offboxRedirect(w, r, "A tárhely útvonalának abszolútnak kell lennie (/-rel kezdődjön).", true) return } + // Validate BEFORE persisting — host/user/repo flow into the ssh command restic runs; reject anything + // that could inject an ssh option (leading '-') or a metacharacter (the security boundary). + if err := backup.ValidateOffboxTarget(&settings.OffboxTarget{Host: host, User: user, RepoPath: repoPath, Port: port}); err != nil { + offboxRedirect(w, r, "Érvénytelen beállítás: "+err.Error(), true) + return + } // First-time config requires the SSH key + a pinned known-host line (no blind TOFU). existing := s.backupMgr.OffboxConfigured() if !existing && (strings.TrimSpace(sshKey) == "" || strings.TrimSpace(knownHosts) == "") {