From 0a70a2437e4b6489b1c0430b83bfea0583229247 Mon Sep 17 00:00:00 2001 From: Lee Myers Date: Fri, 14 Aug 2026 09:13:16 -0400 Subject: [PATCH] fix(redactors): assert embedded temp paths stay in their temp directory CodeQL go/zipslip (alert 3820, high, 19 sinks) flags the flow from a zip entry name to the writes that materialize an embedded part. The flow is real; the conclusion is not. Two things already made an escape impossible: - the entry name never becomes a FILENAME. inPath and outPath use the literals "embedded" and "redacted"; only the extension comes from the archive. - embedded.SafeExt validates that extension against a CHARACTER allowlist -- a dot plus 1-10 bytes from [a-z0-9], else the ".bin" fallback -- so "..", separators, NUL and drive letters are unrepresentable in the result rather than stripped from it. TestSafeExtNeverYieldsAPathComponent already asserts that against 21 hostile entry names. So this is not a fix for an exploitable bug. It adds the containment assertion at the sink anyway, for two reasons. The first is the refactor. The safety of those two writes currently rests on a basename that happens to be a literal three lines above them. Someone deriving the basename from the entry name -- to keep part names recognizable in a debug dump, say -- reintroduces the traversal, and nothing local to the write says otherwise. A traversal here writes a document's UNREDACTED bytes to an attacker-chosen path, which is worse than the leak this package exists to prevent, so the check is cheap relative to the failure it forecloses. The second is that a scanner can see it. go/zipslip models a cleaned-prefix containment test as a barrier and does not model a per-character allowlist as one, which is why all 19 sinks -- every one of them downstream of this single construction, not 19 distinct defects -- read as unsanitized. Clearing the class matters beyond the noise: the next genuine path-traversal finding would land in a pile of 19 that a reviewer has already learned to skip. Whether CodeQL actually retires the alert is its call, not something this commit can promise; the alert is dismissed with this reasoning either way. withinDir cleans both sides before comparing, so a "." or ".." element resolves rather than comparing literally, and appends the separator to the directory so a sibling sharing its name prefix is not mistaken for a child -- /tmp/ferret-embedded-1-evil against /tmp/ferret-embedded-1 is the case a naive HasPrefix gets wrong, and the test fails if the separator is dropped. Also corrects a comment in both places that said SafeExt "returns one of a fixed set of '.xyz' literals". It does not: it returns any charset-validated [a-z0-9]{1,10} extension, or ".bin". The security property is identical and the wording overstated the mechanism, which matters in a comment whose whole job is to tell the next reader why the sink is safe. TESTS TestHostilePartNamesStayInTheTempDir drives RedactEmbedded with nine hostile entry names -- relative and absolute traversal, Windows separators, NUL injection, 256 stacked "../" -- against canary files planted in os.TempDir() and its parent, and asserts the canaries are byte-identical afterwards. It asserts through the public entry point rather than through SafeExt alone, so it covers the composition of allowlist, hardcoded basename and containment check rather than one link. TestWithinDirRejectsEscapes covers the predicate directly, including the sibling-prefix case and a traversal that resolves back inside. TestSafeExtStillGuardsTheExtension pins that the allowlist remains the primary control and the new check has not quietly become the only one. go test ./... (66 packages), go vet ./... and make build clean. Refs alert 3820 --- internal/embedded/embedded_test.go | 9 +- internal/redactors/embedded.go | 40 +++++- .../redactors/embedded_containment_test.go | 129 ++++++++++++++++++ 3 files changed, 171 insertions(+), 7 deletions(-) create mode 100644 internal/redactors/embedded_containment_test.go 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) + } + } +}