fix(CTRL-001): reject path traversal in .fab import manifest

manifest.AppName / HDDSubdirs / VolumeNames are attacker-controlled JSON inside an imported .fab and reach filepath.Join+MkdirAll/extractTar with a trusted base (restore.go:339/606/678). UnmarshalManifest did zero validation, so '../..' in any of them escaped the stacks / HDD destination dir. - New appexport.ValidateSegment + validateManifestPaths; UnmarshalManifest now fails the parse on a traversal segment (the chokepoint). - Defence-in-depth ValidateSegment guards at the HDD-subdir and volume-name join loops in restore.go. - ConfigFiles deliberately NOT validated (holds dotfiles like .felhom.yml; never used in a restore join). - Permanent regression test (was the deep-sweep failing audit test) now asserts rejection of traversal + acceptance of legit names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-13 19:09:47 +02:00
parent eea235bd69
commit c20ff56e4a
4 changed files with 164 additions and 0 deletions
@@ -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
 }
@@ -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 {
@@ -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)
+	}
+}
@@ -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
+}