diff --git a/controller/internal/appexport/manifest.go b/controller/internal/appexport/manifest.go index 937af37..69bef81 100644 --- a/controller/internal/appexport/manifest.go +++ b/controller/internal/appexport/manifest.go @@ -38,5 +38,11 @@ func UnmarshalManifest(data []byte) (*Manifest, error) { if err := json.Unmarshal(data, &m); err != nil { return nil, err } + // [CTRL-001] Reject path-traversal in any segment used to build a filesystem + // path on import (app_name, hdd_subdirs, volume_names). A hostile .fab must + // fail to parse rather than escape the stacks / HDD destination dir. + if err := validateManifestPaths(&m); err != nil { + return nil, err + } return &m, nil } diff --git a/controller/internal/appexport/restore.go b/controller/internal/appexport/restore.go index 4a5847d..9866274 100644 --- a/controller/internal/appexport/restore.go +++ b/controller/internal/appexport/restore.go @@ -589,6 +589,11 @@ func (e *Exporter) restoreHDDData(tmpDir string, manifest *Manifest, composePath } for _, subdir := range manifest.HDDSubdirs { + // [CTRL-001] defence-in-depth: refuse any subdir that is not a single + // safe segment before it reaches MkdirAll/extractTar on a user drive. + if err := ValidateSegment("hdd_subdir", subdir); err != nil { + return err + } tarPath := filepath.Join(hddDir, subdir+".tar") tarInfo, err := os.Stat(tarPath) if err != nil { @@ -670,6 +675,11 @@ func (e *Exporter) restoreVolumeData(tmpDir string, manifest *Manifest) error { volDir := filepath.Join(tmpDir, "data", "volumes") for _, volName := range manifest.VolumeNames { + // [CTRL-001] defence-in-depth: refuse any volume name that is not a + // single safe segment before it reaches a tar path / docker volume op. + if err := ValidateSegment("volume_name", volName); err != nil { + return err + } tarPath := filepath.Join(volDir, volName+".tar") tarInfo, err := os.Stat(tarPath) if err != nil { diff --git a/controller/internal/appexport/traversal_regression_test.go b/controller/internal/appexport/traversal_regression_test.go new file mode 100644 index 0000000..6ff7dbe --- /dev/null +++ b/controller/internal/appexport/traversal_regression_test.go @@ -0,0 +1,82 @@ +package appexport + +import ( + "encoding/json" + "strings" + "testing" +) + +// Regression test for [CTRL-001] (path traversal on .fab import). Originated as +// a failing audit test (audit/2026-06-13-deep-sweep); now a permanent guard. +// UnmarshalManifest must REJECT any manifest whose AppName / HDDSubdirs / +// VolumeNames contain a path-traversal or separator, and ACCEPT legitimate +// single-segment names. Do NOT weaken these assertions. + +func mustManifestJSON(t *testing.T, m Manifest) []byte { + t.Helper() + b, err := json.Marshal(m) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return b +} + +func TestUnmarshalManifestRejectsTraversal(t *testing.T) { + cases := []struct { + name string + m Manifest + }{ + {"appname-parent", Manifest{Version: 1, AppName: "../evil"}}, + {"appname-deep", Manifest{Version: 1, AppName: "../../etc/cron.d/x"}}, + {"appname-absolute", Manifest{Version: 1, AppName: "/etc/cron.d/x"}}, + {"appname-dotdot", Manifest{Version: 1, AppName: ".."}}, + {"appname-empty", Manifest{Version: 1, AppName: ""}}, + {"appname-backslash", Manifest{Version: 1, AppName: `..\evil`}}, + {"hdd-subdir-escape", Manifest{Version: 1, AppName: "romm", HDDSubdirs: []string{"../../mnt"}}}, + {"volume-escape", Manifest{Version: 1, AppName: "romm", VolumeNames: []string{"../../var/lib"}}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := UnmarshalManifest(mustManifestJSON(t, tc.m)) + if err == nil { + t.Fatalf("CTRL-001 regression: UnmarshalManifest accepted a traversal manifest %+v; expected rejection", tc.m) + } + }) + } +} + +func TestUnmarshalManifestAcceptsLegitNames(t *testing.T) { + m := Manifest{ + Version: 1, + AppName: "paperless-ngx", + HDDSubdirs: []string{"felhom-usb", "romm"}, + VolumeNames: []string{"adventurelog_postgres_data", "romm_redis-data"}, + ConfigFiles: []string{".felhom.yml", "docker-compose.yml", "app.yaml"}, // dotfiles must NOT be rejected + } + got, err := UnmarshalManifest(mustManifestJSON(t, m)) + if err != nil { + t.Fatalf("CTRL-001 regression: UnmarshalManifest rejected a legitimate manifest: %v", err) + } + if got.AppName != "paperless-ngx" { + t.Fatalf("AppName round-trip mismatch: %q", got.AppName) + } +} + +func TestValidateSegment(t *testing.T) { + good := []string{"romm", "paperless-ngx", "adventurelog_postgres_data", "felhom-usb", "a", "App1.2_3-4"} + for _, s := range good { + if err := ValidateSegment("x", s); err != nil { + t.Errorf("ValidateSegment(%q) = %v; want nil", s, err) + } + } + bad := []string{"", ".", "..", "../x", "a/b", `a\b`, "/abs", ".hidden", "-leadingdash", "a/../b"} + for _, s := range bad { + if err := ValidateSegment("x", s); err == nil { + t.Errorf("ValidateSegment(%q) = nil; want rejection", s) + } + } + // Sanity: a rejected value's message names the kind, for operator clarity. + if err := ValidateSegment("app_name", "../x"); err == nil || !strings.Contains(err.Error(), "app_name") { + t.Errorf("expected error mentioning app_name, got %v", err) + } +} diff --git a/controller/internal/appexport/validate.go b/controller/internal/appexport/validate.go new file mode 100644 index 0000000..4834359 --- /dev/null +++ b/controller/internal/appexport/validate.go @@ -0,0 +1,66 @@ +package appexport + +import ( + "fmt" + "path/filepath" + "regexp" + "strings" +) + +// safeSegment matches a single safe path component: starts with an +// alphanumeric, then alphanumerics / dot / dash / underscore. It cannot be +// "." or ".." (must start alnum), cannot contain a path separator, and cannot +// be an absolute path. This covers the legitimate values these fields hold — +// app slugs (e.g. "paperless-ngx"), HDD mount basenames (e.g. "felhom-usb"), +// and docker volume names (e.g. "adventurelog_postgres_data"). +var safeSegment = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`) + +// ValidateSegment rejects any value that is not a single safe path component. +// It is the guard for [CTRL-001]: manifest fields that reach filepath.Join with +// a trusted base (AppName, HDDSubdirs, VolumeNames) are fully attacker-controlled +// JSON inside an imported .fab, so a value like "../../etc/cron.d/x" must be +// refused before it can escape the stacks / HDD destination directory. +// +// NOTE: this is deliberately NOT applied to manifest.ConfigFiles — those are +// dotfile-bearing names (e.g. ".felhom.yml") that are never used in a restore +// join (restoreConfig enumerates the extracted dir via os.ReadDir, whose names +// are already single components). +func ValidateSegment(kind, s string) error { + if s == "" { + return fmt.Errorf("appexport: empty %s", kind) + } + if s == "." || s == ".." { + return fmt.Errorf("appexport: %s %q is a path-traversal segment", kind, s) + } + if strings.ContainsAny(s, `/\`) || strings.ContainsRune(s, filepath.Separator) { + return fmt.Errorf("appexport: %s %q must not contain a path separator", kind, s) + } + if filepath.IsAbs(s) { + return fmt.Errorf("appexport: %s %q must not be an absolute path", kind, s) + } + if !safeSegment.MatchString(s) { + return fmt.Errorf("appexport: %s %q is not a safe single-segment name", kind, s) + } + return nil +} + +// validateManifestPaths checks every manifest field that is later used as a +// path segment in a filepath.Join against a trusted base. Called from +// UnmarshalManifest so a hostile bundle fails the parse, before executeImport +// can MkdirAll/extract into a traversed location. +func validateManifestPaths(m *Manifest) error { + if err := ValidateSegment("app_name", m.AppName); err != nil { + return err + } + for _, s := range m.HDDSubdirs { + if err := ValidateSegment("hdd_subdir", s); err != nil { + return err + } + } + for _, v := range m.VolumeNames { + if err := ValidateSegment("volume_name", v); err != nil { + return err + } + } + return nil +}