diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index cf3b7e07..ea49e22a 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -243,3 +243,9 @@ parallel-agent merge contention bites. per-repo override cannot set dormant and that the kill switch cannot silence — this adds a new protected-domain concept to the rules contract, a maintainer decision, not an autonomous change. +- 2026-07-12 — iss-30 (memory ingest boundary) partially resolved: the fetch/read + subset — C12 (HTTP status), P11 (SSRF NAT64/6to4), C13 (local size cap), the + ~user tilde mangle — landed in PR #38. iss-30 stays OPEN for its remaining + instances (the larger "ingest test-suite" effort): the --keep-original + partial-failure reporting, CRLF parser-parity (parseFrontmatter vs + splitFileFrontmatter), and broader URL-ingest/content-type/PDF path coverage. diff --git a/CHANGELOG.md b/CHANGELOG.md index e5e1a337..2a75bea8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,12 @@ called out in a **Breaking** section. ### Security +- **Memory-ingest fetch/read hardening** (iss-30). `abcd memory ingest` now treats + a non-2xx HTTP response as an error instead of storing the 404/500 error page as + source content; the SSRF guard additionally rejects NAT64 (`64:ff9b::/96`) and + 6to4 (`2002::/16`) IPv6 addresses that embed a metadata/loopback/private IPv4; a + local source file is size-capped like the URL path; and a `~user` path is left + literal rather than being mangled into `home`+`user`. - **Spec-store hardening** (iss-68). The spec-store reader now opens a file once with `O_NOFOLLOW`+`O_NONBLOCK` and validates the file descriptor before reading, closing a symlink-swap window (and never blocking on a FIFO leaf). `NextID` fails diff --git a/internal/core/memory/ingest.go b/internal/core/memory/ingest.go index 222a0889..01d8e305 100644 --- a/internal/core/memory/ingest.go +++ b/internal/core/memory/ingest.go @@ -481,8 +481,42 @@ func acquireSource(source string, fetcher Fetcher, pdf PDFExtractor) (sourceMate // the SSRF guard that keeps cloud metadata endpoints (e.g. 169.254.169.254) and // internal services out of reach. func blockedFetchIP(ip net.IP) bool { - return ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || - ip.IsPrivate() || ip.IsUnspecified() || ip.IsMulticast() + if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || + ip.IsPrivate() || ip.IsUnspecified() || ip.IsMulticast() { + return true + } + // NAT64 (64:ff9b::/96) and 6to4 (2002::/16) embed an IPv4 destination in an + // IPv6 address the checks above do not flag; a metadata/loopback/private IPv4 + // wrapped in one of these would otherwise slip through. Extract the embedded + // v4 and re-check it (embeddedIPv4 returns nil for a v4, so no recursion loop). + if v4 := embeddedIPv4(ip); v4 != nil { + return blockedFetchIP(v4) + } + return false +} + +// embeddedIPv4 returns the IPv4 address a NAT64 (64:ff9b::/96) or 6to4 (2002::/16) +// IPv6 address embeds, or nil when ip is not one of those transition forms. Scope +// is the WELL-KNOWN prefixes only: deprecated IPv4-compatible (::/96, non-routable) +// and site-specific NAT64 prefixes (RFC 8215) are out of scope — the DNS64 default +// is the well-known /96 covered here. v4-mapped ::ffff:/96 needs no extraction (the +// standard IsPrivate/IsLoopback checks already fold through To4()). +func embeddedIPv4(ip net.IP) net.IP { + v6 := ip.To16() + if v6 == nil || ip.To4() != nil { + return nil // not IPv6 (a plain v4 or v4-mapped needs no extraction) + } + // NAT64 well-known prefix 64:ff9b::/96 → last 4 bytes are the v4. + if v6[0] == 0x00 && v6[1] == 0x64 && v6[2] == 0xff && v6[3] == 0x9b && + v6[4] == 0 && v6[5] == 0 && v6[6] == 0 && v6[7] == 0 && + v6[8] == 0 && v6[9] == 0 && v6[10] == 0 && v6[11] == 0 { + return net.IPv4(v6[12], v6[13], v6[14], v6[15]) + } + // 6to4 prefix 2002::/16 → bytes 2..5 are the v4. + if v6[0] == 0x20 && v6[1] == 0x02 { + return net.IPv4(v6[2], v6[3], v6[4], v6[5]) + } + return nil } // guardFetchHost refuses a host that is an internal/metadata name or that @@ -560,6 +594,16 @@ func defaultFetch(rawURL string) (FetchedSource, error) { return FetchedSource{}, newIngestError("fetch failed for %s: %v", rawURL, err) } defer resp.Body.Close() + return readFetchedResponse(rawURL, resp) +} + +// readFetchedResponse validates the HTTP status and reads the size-capped body. +// A non-2xx response (a 404/500 error page) is an ingest ERROR, not source +// content — otherwise the error page's HTML would be stored as knowledge. +func readFetchedResponse(rawURL string, resp *http.Response) (FetchedSource, error) { + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return FetchedSource{}, newIngestError("fetch failed for %s: HTTP %d %s", rawURL, resp.StatusCode, http.StatusText(resp.StatusCode)) + } body, err := io.ReadAll(io.LimitReader(resp.Body, maxFetchBytes+1)) if err != nil { return FetchedSource{}, newIngestError("fetch failed for %s: %v", rawURL, err) @@ -577,7 +621,9 @@ func defaultFetch(rawURL string) (FetchedSource, error) { func materialFromLocal(source string, pdf PDFExtractor) (sourceMaterial, error) { expanded := source - if strings.HasPrefix(expanded, "~") { + // Expand only a leading "~" or "~/…" to the home dir. A "~user" form is NOT the + // caller's home and must not be mangled into home+"user" — leave it literal. + if expanded == "~" || strings.HasPrefix(expanded, "~/") { if home, err := os.UserHomeDir(); err == nil { expanded = home + expanded[1:] } @@ -597,6 +643,11 @@ func materialFromLocal(source string, pdf PDFExtractor) (sourceMaterial, error) if !st.Mode().IsRegular() { return sourceMaterial{}, newIngestError("source path is not a regular file (directories, devices and symlinks-to-special are rejected): %s", resolved) } + // Cap the local read the same as the URL path, so a huge local file cannot be + // slurped whole into memory before any text/NUL sniffing. + if st.Size() > maxFetchBytes { + return sourceMaterial{}, newIngestError("source file exceeds the %d-byte cap: %s", maxFetchBytes, resolved) + } raw, err := os.ReadFile(resolved) if err != nil { return sourceMaterial{}, newIngestError("cannot read source: %s (%v)", resolved, err) diff --git a/internal/core/memory/memory_test.go b/internal/core/memory/memory_test.go index 86d31bfb..235976a3 100644 --- a/internal/core/memory/memory_test.go +++ b/internal/core/memory/memory_test.go @@ -1,6 +1,8 @@ package memory import ( + "io" + "net/http" "os" "path/filepath" "strings" @@ -309,6 +311,9 @@ func TestIngestRefusesSSRFTargets(t *testing.T) { "http://172.16.5.5/x", // private "http://metadata.google.internal/x", // metadata name "http://svc.internal/x", // .internal name + "http://[64:ff9b::a9fe:a9fe]/x", // NAT64 embedding 169.254.169.254 (metadata) + "http://[64:ff9b::7f00:1]/x", // NAT64 embedding 127.0.0.1 (loopback) + "http://[2002:a9fe:a9fe::]/x", // 6to4 embedding 169.254.169.254 (metadata) } for _, target := range cases { t.Run(target, func(t *testing.T) { @@ -358,3 +363,76 @@ func TestValidateDistilledPageComputesTopicHash(t *testing.T) { t.Fatalf("filename = %q", p.Filename()) } } + +// TestReadFetchedResponseRejectsNon2xx (iss-30 C12) proves a non-2xx HTTP +// response (a 404/500 error page) is an ingest error, not stored as source +// content. +func TestReadFetchedResponseRejectsNon2xx(t *testing.T) { + for _, code := range []int{404, 500, 301, 403} { + resp := &http.Response{ + StatusCode: code, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("error page body")), + } + if _, err := readFetchedResponse("http://x/y", resp); err == nil { + t.Fatalf("HTTP %d must be an ingest error, not accepted as content", code) + } + } + // A 200 is read normally. + ok := &http.Response{ + StatusCode: 200, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("real content")), + } + fs, err := readFetchedResponse("http://x/y", ok) + if err != nil || string(fs.Body) != "real content" { + t.Fatalf("200 response = %q, %v", fs.Body, err) + } +} + +// TestMaterialFromLocalSizeCap (iss-30 C13) proves a local file larger than the +// fetch cap is rejected before it is read whole into memory. +func TestMaterialFromLocalSizeCap(t *testing.T) { + big := filepath.Join(t.TempDir(), "big.txt") + f, err := os.Create(big) + if err != nil { + t.Fatal(err) + } + if err := f.Truncate(maxFetchBytes + 1); err != nil { // sparse; fast + t.Fatal(err) + } + f.Close() + // Assert the SIZE cap is what rejects it — a sparse (all-zero) file would also + // trip the downstream NUL-byte check, so a bare "err != nil" would pass even + // with the cap removed (false pin). The "exceeds" message is produced only by + // the size guard, so this fails on revert. + _, err = materialFromLocal(big, nil) + if err == nil || !strings.Contains(err.Error(), "exceeds") { + t.Fatalf("an over-cap local file must be rejected by the size cap, got: %v", err) + } +} + +// TestMaterialFromLocalTildeUser (iss-30) proves a "~/…" path expands to the home +// dir but a "~user" path is left literal (not mangled into home+"user"). +func TestMaterialFromLocalTildeUser(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if err := os.WriteFile(filepath.Join(home, "real.txt"), []byte("hello\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := materialFromLocal("~/real.txt", nil); err != nil { + t.Fatalf("~/real.txt must expand to the home dir and be found: %v", err) + } + // Create home/baduser/real.txt — the file a MANGLED "~baduser" would resolve + // to (home + "baduser/real.txt"). Left literal, "~baduser/real.txt" must not + // find it. + if err := os.MkdirAll(filepath.Join(home, "baduser"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(home, "baduser", "real.txt"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := materialFromLocal("~baduser/real.txt", nil); err == nil { + t.Fatal("~baduser must be left literal, not expanded to home+baduser (which would wrongly succeed)") + } +}