diff --git a/internal/embedded/embedded_test.go b/internal/embedded/embedded_test.go index 2f9e26f..905a2a5 100644 --- a/internal/embedded/embedded_test.go +++ b/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", diff --git a/internal/redactors/embedded.go b/internal/redactors/embedded.go index f035ebd..a214b6a 100644 --- a/internal/redactors/embedded.go +++ b/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)) +} diff --git a/internal/redactors/embedded_containment_test.go b/internal/redactors/embedded_containment_test.go new file mode 100644 index 0000000..d89617d --- /dev/null +++ b/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) + } + } +}