security(offbox): validate host/user/repo before the ssh exec (option-injection guard)
Background commit review flagged command/option injection: operator-provided host/user/ repo_path flow into restic's ssh -s sftp command. Reject leading '-' (ssh option injection, e.g. -oProxyCommand) + metacharacters/traversal; OffboxConfigured fails closed on an invalid target. Companion test covers the injection cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HxLA1mZurFq9kt8hneFeCs
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user