fix(redactors): assert embedded temp paths stay in their temp directory

internal/embedded/embedded_test.go

@@ -13,10 +13,11 @@ import (
 //
 // The value SafeExt returns is concatenated into a filesystem path when an embedded
 // part is materialized as a temp file, and the input is a zip entry name, which is
-// entirely producer-controlled. So the output must be one of a fixed set of ".xyz"
-// literals no matter what the entry is called — not "sanitized", but incapable of
-// carrying a separator, a parent reference, a NUL or an absolute path in the first
-// place (BSC1: validate untrusted input against an allowlist at the sink).
+// entirely producer-controlled. So whatever the entry is called, the output must be
+// a dot followed by 1-10 characters drawn from [a-z0-9], or the ".bin" fallback, and
+// nothing else — not "sanitized", but incapable of carrying a separator, a parent
+// reference, a NUL or an absolute path in the first place (BSC1: validate untrusted
+// input against an allowlist at the sink).
 func TestSafeExtNeverYieldsAPathComponent(t *testing.T) {
 	hostile := []string{
 		"../../../../etc/passwd",

internal/redactors/embedded.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	"github.com/awslabs/ferret-scan/v2/internal/detector"
@@ -169,9 +170,9 @@ func (rm *RedactionManager) RedactEmbedded(req EmbeddedRedactionRequest) (*Embed
 	// The extension comes from the admission allowlist, never from the entry name.
 	// req.PartName is producer-controlled and the value below is concatenated into
 	// a filesystem path, so per BSC1 it is validated against an allowlist at the
-	// sink. embedded.SafeExt returns one of a fixed set of ".xyz" literals, which
-	// makes "..", separators and NUL unrepresentable in the result rather than
-	// something to strip.
+	// sink. embedded.SafeExt returns either ".bin" or a dot followed by 1-10
+	// characters drawn from [a-z0-9] and nothing else, which makes "..", separators
+	// and NUL unrepresentable in the result rather than something to strip.
 	safeExt, ok := embedded.SafeExt(req.PartName)
 	if !ok {
 		return nil, fmt.Errorf("%w: %s", ErrNoEmbeddedRedactor, filepath.Ext(req.PartName))
@@ -195,6 +196,24 @@ func (rm *RedactionManager) RedactEmbedded(req EmbeddedRedactionRequest) (*Embed
 
 	inPath := filepath.Join(dir, "embedded"+safeExt)
 	outPath := filepath.Join(dir, "redacted"+safeExt)
+
+	// Containment assertion at the sink. SafeExt already makes an escaping path
+	// unrepresentable, so on today's code this cannot fire — it is defence in depth,
+	// for the refactor that starts deriving the basename from the entry name and
+	// only notices later. A traversal here would write a document's UNREDACTED bytes
+	// to an attacker-chosen location, so the cost of the check is not worth saving.
+	//
+	// It is also what a scanner can see: CodeQL's go/zipslip models a cleaned-prefix
+	// containment test as a barrier but not a per-character allowlist, so without
+	// this the flow from the zip entry to these writes reads as unsanitized (alert
+	// 3820, 19 sinks, all of them downstream of this one construction).
+	for _, p := range []string{inPath, outPath} {
+		if !withinDir(dir, p) {
+			return nil, fmt.Errorf("%w: embedded part %s resolved outside its temp directory",
+				ErrNoEmbeddedRedactor, filepath.Base(req.PartName))
+		}
+	}
+
 	if err := os.WriteFile(inPath, req.Content, 0o600); err != nil {
 		return nil, fmt.Errorf("writing embedded part to temp file: %w", err)
 	}
@@ -241,3 +260,18 @@ func (rm *RedactionManager) RedactEmbedded(req EmbeddedRedactionRequest) (*Embed
 		PartName:     req.PartName,
 	}, nil
 }
+
+// withinDir reports whether path is dir itself or a descendant of it.
+//
+// Both sides are cleaned before comparison, so a "." or ".." element is resolved
+// rather than compared literally, and the separator is appended to dir so a
+// sibling with dir as a name prefix ("/tmp/ferret-embedded-1-evil" against
+// "/tmp/ferret-embedded-1") is not mistaken for a child.
+func withinDir(dir, path string) bool {
+	cleanDir := filepath.Clean(dir)
+	cleanPath := filepath.Clean(path)
+	if cleanPath == cleanDir {
+		return true
+	}
+	return strings.HasPrefix(cleanPath, cleanDir+string(os.PathSeparator))
+}

internal/redactors/embedded_containment_test.go

@@ -0,0 +1,129 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package redactors
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/awslabs/ferret-scan/v2/internal/embedded"
+)
+
+// TestWithinDirRejectsEscapes covers the containment predicate directly, including
+// the case a naive prefix comparison gets wrong.
+func TestWithinDirRejectsEscapes(t *testing.T) {
+	dir := filepath.Clean("/tmp/ferret-embedded-1")
+
+	inside := []string{
+		dir,
+		filepath.Join(dir, "embedded.docx"),
+		filepath.Join(dir, "nested", "deeper.jpg"),
+		// A traversal that resolves back inside is inside.
+		filepath.Join(dir, "a", "..", "embedded.bin"),
+	}
+	for _, p := range inside {
+		if !withinDir(dir, p) {
+			t.Errorf("withinDir(%q, %q) = false, want true", dir, p)
+		}
+	}
+
+	outside := []string{
+		filepath.Clean("/tmp/ferret-embedded-1-evil/x.docx"), // sibling sharing the prefix
+		filepath.Clean("/tmp/other/x.docx"),
+		filepath.Join(dir, "..", "escape.docx"),
+		filepath.Join(dir, "..", "..", "etc", "passwd"),
+		filepath.Clean("/etc/passwd"),
+	}
+	for _, p := range outside {
+		if withinDir(dir, p) {
+			t.Errorf("withinDir(%q, %q) = true, want false", dir, p)
+		}
+	}
+}
+
+// TestHostilePartNamesStayInTheTempDir is the end-to-end statement of the property
+// CodeQL's go/zipslip could not see: a producer-controlled archive entry name cannot
+// steer a write out of the per-part temp directory.
+//
+// It asserts through RedactEmbedded rather than through SafeExt alone, so it covers
+// the composition — allowlisted extension, hardcoded basename, containment check —
+// rather than one link of it. Any escape would write a document's UNREDACTED bytes to
+// an attacker-chosen path, which is strictly worse than the leak the redactor exists
+// to prevent.
+func TestHostilePartNamesStayInTheTempDir(t *testing.T) {
+	rm := newTestManager(t)
+
+	// A canary in every directory a traversal from os.TempDir() could plausibly
+	// reach. Its content must be unchanged afterwards.
+	tmp := os.TempDir()
+	canaries := map[string][]byte{}
+	for _, rel := range []string{"ferret-canary.txt", filepath.Join("..", "ferret-canary.txt")} {
+		p := filepath.Clean(filepath.Join(tmp, rel))
+		want := []byte("canary must not be overwritten\n")
+		if err := os.WriteFile(p, want, 0o600); err != nil {
+			continue // not writable here; skip this location rather than fail
+		}
+		canaries[p] = want
+		defer func(target string) { _ = os.Remove(target) }(p)
+	}
+
+	hostile := []string{
+		"../../../../tmp/ferret-canary.txt",
+		"../ferret-canary.txt",
+		"word/media/../../../../tmp/ferret-canary.txt",
+		"/tmp/ferret-canary.txt",
+		"C:\\Windows\\System32\\ferret-canary.txt",
+		"..\\..\\ferret-canary.txt",
+		"word/media/x.txt\x00../../ferret-canary.txt",
+		strings.Repeat("../", 256) + "tmp/ferret-canary.txt",
+		"....//....//ferret-canary.txt",
+	}
+
+	for _, name := range hostile {
+		t.Run(name, func(t *testing.T) {
+			// The call is expected to succeed or fail; either is fine. What must not
+			// happen is a write outside the temp directory.
+			_, _ = rm.RedactEmbedded(EmbeddedRedactionRequest{
+				ParentPath: filepath.Join(t.TempDir(), "outer.docx"),
+				PartName:   name,
+				Content:    []byte("ssn 796-58-4123\n"),
+				Strategy:   RedactionFormatPreserving,
+			})
+
+			for p, want := range canaries {
+				got, err := os.ReadFile(p) // #nosec G304 -- test-owned path
+				if err != nil {
+					t.Errorf("canary %s disappeared after part %q: %v", p, name, err)
+					continue
+				}
+				if string(got) != string(want) {
+					t.Errorf("canary %s was MODIFIED by part %q: a hostile archive entry "+
+						"escaped its temp directory", p, name)
+				}
+			}
+		})
+	}
+}
+
+// TestSafeExtStillGuardsTheExtension pins that the containment check did not become
+// the only defence. The allowlist is the primary control — it makes an escaping path
+// unrepresentable — and the check exists for a future refactor, not instead of it.
+func TestSafeExtStillGuardsTheExtension(t *testing.T) {
+	for _, name := range []string{
+		"../../../../etc/passwd",
+		"word/media/../../../evil.jpg",
+		"/etc/shadow.jpg",
+		"x.jpg\x00.exe",
+	} {
+		ext, ok := embedded.SafeExt(name)
+		if !ok {
+			continue
+		}
+		if strings.ContainsAny(ext, `/\`+"\x00") || strings.Contains(ext, "..") {
+			t.Errorf("SafeExt(%q) = %q, which can carry a path component", name, ext)
+		}
+	}
+}