amadan

carlos / platform Public

geoip: untrusted databases are parsed only in a killable verifier child (feed, uploads and the CONSOLE_GEOIP_DB override); fixtures that hang or panic the parser (Astra code review R9 P1s)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 00428606739bb7c1c04e81a594d99e3969820bdf parent 171e7cc
10 files changed, +240 −28
  • cmd/console/main.go +13 −1
  • cmd/console/main_test.go +6 −0
  • internal/console/geoippublish.go +18 −27
  • internal/console/geoippublish_test.go +34 −0
  • internal/console/geoipverify.go +135 −0
  • internal/console/geoipverify_main_test.go +28 −0
  • internal/geo/testdata/README +6 −0
  • internal/geo/testdata/pathological-dag.mmdb binary
  • internal/geo/testdata/pathological-empty-dag.mmdb binary
  • internal/geo/testdata/pathological-metadata.mmdb binary

CI passed — run details

diff --git a/cmd/console/main.go b/cmd/console/main.go
index 30c54ed7b..7b640278f 100644
--- a/cmd/console/main.go
+++ b/cmd/console/main.go
@@ -105,6 +105,10 @@ func main() {
usage()
}
switch os.Args[1] {
+ case console.GeoipVerifyVerb:
+ // The GeoIP verifier child (internal/console/geoipverify.go): parses
+ // an untrusted .mmdb so the serving process never has to.
+ os.Exit(console.RunGeoipVerify(os.Args[2:], os.Stdout))
case "serve":
serve()
case "grant", "revoke":
@@ -1176,7 +1180,15 @@ func openGeoIPAt(path string, now time.Time) *geo.Swappable {
if path == "" {
return nil
}
- m, err := geo.OpenMMDB(path)
+ // Parsed first in the verifier child: a malformed file must not crash
+ // the console (code review R9), only turn the IP step off.
+ vctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+ err := console.VerifyGeoIPFile(vctx, path)
+ cancel()
+ var m *geo.MMDB
+ if err == nil {
+ m, err = geo.OpenMMDBTrusted(path)
+ }
if err != nil {
log.Printf("console: CONSOLE_GEOIP_DB %s is unusable, so the IP step of region guesses is off: %v", path, firstLineCapped(err))
return nil
diff --git a/cmd/console/main_test.go b/cmd/console/main_test.go
index fb1c6ae87..0c889abc5 100644
--- a/cmd/console/main_test.go
+++ b/cmd/console/main_test.go
@@ -64,6 +64,12 @@ func runner(t *testing.T, bin string, env ...string) func(args ...string) (strin
}
func TestMain(m *testing.M) {
+ // The GeoIP verifier verb: openGeoIP re-runs its own binary to verify
+ // the override in a child process, and under `go test` that binary is
+ // this one.
+ if len(os.Args) > 1 && os.Args[1] == console.GeoipVerifyVerb {
+ os.Exit(console.RunGeoipVerify(os.Args[2:], os.Stdout))
+ }
code := m.Run()
if buildDir != "" {
os.RemoveAll(buildDir)
diff --git a/internal/console/geoippublish.go b/internal/console/geoippublish.go
index 7df532af7..e17085620 100644
--- a/internal/console/geoippublish.go
+++ b/internal/console/geoippublish.go
@@ -9,14 +9,12 @@ import (
"fmt"
"io"
"log"
- "net"
"os"
"path/filepath"
"strings"
"sync"
"time"
- "github.com/carlosframework/platform/internal/geo"
"github.com/carlosframework/platform/internal/geoip"
"github.com/carlosframework/platform/internal/release"
)
@@ -213,42 +211,45 @@ func (p *geoipPublisher) Commit(ctx context.Context, st *geoipStaged, o geoipPub
ctx, cancel = context.WithTimeout(ctx, p.deadline)
defer cancel()
}
- db, err := geo.OpenMMDBVerified(ctx, st.path)
- if errors.Is(err, geo.ErrSearchTreeTooLarge) {
+ // The file is untrusted: it is parsed only in a child process (code
+ // review R9), never here.
+ db, err := verifyInChild(ctx, st.path, geoipProbeAddrs)
+ switch {
+ case ctx.Err() != nil:
+ return geoip.Pointer{}, ctx.Err()
+ case errors.Is(err, errGeoipVerifyTimeout):
+ return geoip.Pointer{}, refused("the database took too long to verify")
+ case errors.Is(err, errGeoipVerifyTree):
return geoip.Pointer{}, refused("the database's search tree is malformed")
- }
- if err != nil {
- if ctx.Err() != nil {
- return geoip.Pointer{}, ctx.Err()
- }
+ case err != nil:
return geoip.Pointer{}, refused("not a valid MaxMind-format database")
}
- defer db.Close()
- if !hasCountryData(db) {
+ if db.Countries < geoipProbeNeed {
return geoip.Pointer{}, refused("the database has no country data")
}
+ dbBuild := time.Unix(db.BuildEpoch, 0).UTC()
// D1 at the door (codex R7-1): an already-expired GeoLite2 would replace
// a usable database with one every edge must reject. force never
// overrides it; the check runs again just before the pointer write.
- if p.expired(db.Source(), db.BuildEpoch(), p.now()) {
+ if p.expired(db.Source, dbBuild, p.now()) {
return geoip.Pointer{}, refused("the database is already expired (GeoLite2, 30 days after build)")
}
- if o.RequireType != "" && db.Source() != o.RequireType {
- return geoip.Pointer{}, refused("the feed returned a " + db.Source() + " database, not " + o.RequireType)
+ if o.RequireType != "" && db.Source != o.RequireType {
+ return geoip.Pointer{}, refused("the feed returned a " + db.Source + " database, not " + o.RequireType)
}
cur, etag, present, curErr := st.cur, st.curEtag, st.curPresent, st.curErr
- if o.Monotonic && present && curErr == nil && cur.Origin == o.Origin && !db.BuildEpoch().After(cur.Build()) {
+ if o.Monotonic && present && curErr == nil && cur.Origin == o.Origin && !dbBuild.After(cur.Build()) {
return geoip.Pointer{}, refused("not newer than the published edition")
}
switch {
case curErr != nil:
// A pointer exists but does not parse: replace it (etag still guards).
log.Printf("console: geoip publish: the current pointer is unreadable and will be replaced")
- case present && !o.Force && cur.DatabaseType == db.Source() && db.NodeCount() < cur.NodeCount/2:
+ case present && !o.Force && cur.DatabaseType == db.Source && db.NodeCount < cur.NodeCount/2:
return geoip.Pointer{}, refused("fewer than half the current database's nodes (use -force if intended)")
}
next := geoip.Pointer{V: 1, SHA256: st.sha, Key: geoip.BlobKey(st.sha), Size: st.size,
- DatabaseType: db.Source(), BuildEpoch: db.BuildEpoch().Unix(), NodeCount: db.NodeCount(),
+ DatabaseType: db.Source, BuildEpoch: db.BuildEpoch, NodeCount: db.NodeCount,
Published: p.now().UTC().Truncate(time.Second), Origin: o.Origin, Edition: o.Edition}
f, err := os.Open(st.path)
if err != nil {
@@ -347,16 +348,6 @@ func (p *geoipPublisher) verifyExistingBlob(ctx context.Context, key, sha string
return nil
}
-func hasCountryData(db *geo.MMDB) bool {
- n := 0
- for _, a := range geoipProbeAddrs {
- if r, ok := db.Lookup(net.ParseIP(a)); ok && r.Country != "" {
- n++
- }
- }
- return n >= geoipProbeNeed
-}
-
// Off deletes the pointer, then every blob (spec §3). Unconditional delete:
// safe under the single-writer mutex (codex R2-6).
func (p *geoipPublisher) Off(ctx context.Context) error {
diff --git a/internal/console/geoippublish_test.go b/internal/console/geoippublish_test.go
index dba5b3d33..473432029 100644
--- a/internal/console/geoippublish_test.go
+++ b/internal/console/geoippublish_test.go
@@ -481,3 +481,37 @@ func TestPublishRefusesAPathologicalTreeInTime(t *testing.T) {
t.Fatalf("staging left behind: %v", entries)
}
}
+
+// Code review R9: databases that hang or panic the parser are refused in
+// time, by the child, and the console survives with its slot and pointer.
+func TestPublishSurvivesDatabasesThatHangOrPanicTheParser(t *testing.T) {
+ withFixtureProbes(t)
+ prev := geoipVerifyTimeout
+ geoipVerifyTimeout = 2 * time.Second
+ t.Cleanup(func() { geoipVerifyTimeout = prev })
+ ctx := context.Background()
+ p, _ := newTestPublisher(t)
+ good, err := p.Publish(ctx, bytes.NewReader(fixture(t, "GeoIP2-City-Test.mmdb")), geoipPublishOpts{Origin: "manual"})
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, f := range []string{"pathological-empty-dag.mmdb", "pathological-metadata.mmdb"} {
+ start := time.Now()
+ if _, err := p.Publish(ctx, bytes.NewReader(fixture(t, f)), geoipPublishOpts{Origin: "manual", Force: true}); !errors.Is(err, errGeoipRefused) {
+ t.Fatalf("%s: err = %v", f, err)
+ }
+ if d := time.Since(start); d > 15*time.Second {
+ t.Fatalf("%s took %s", f, d)
+ }
+ if cur, _, _, _ := p.Current(ctx); cur.SHA256 != good.SHA256 {
+ t.Fatalf("%s replaced the pointer", f)
+ }
+ if !p.mu.TryLock() {
+ t.Fatalf("%s left the slot held", f)
+ }
+ p.mu.Unlock()
+ if entries, _ := os.ReadDir(p.tmpRoot); len(entries) != 0 {
+ t.Fatalf("%s left staging: %v", f, entries)
+ }
+ }
+}
diff --git a/internal/console/geoipverify.go b/internal/console/geoipverify.go
new file mode 100644
index 000000000..fce816964
--- /dev/null
+++ b/internal/console/geoipverify.go
@@ -0,0 +1,135 @@
+package console
+
+// Verifying an untrusted GeoIP database in a child process (code review
+// R9). A .mmdb from a feed or an upload is parsed by maxminddb, which
+// trusts its metadata: a crafted file can panic it (node_count arithmetic)
+// or make its tree walk run for 2^128 steps inside a single call. Neither
+// may take the console down or hold the publish slot past its deadline, so
+// the console never parses such a file itself: it runs its own binary with
+// a hidden verb, kills it at the deadline, and reads back a small report.
+
+import (
+ "bytes"
+ "context"
+ "encoding/json"
+ "errors"
+ "fmt"
+ "io"
+ "net"
+ "os"
+ "os/exec"
+ "time"
+
+ "github.com/carlosframework/platform/internal/geo"
+)
+
+// GeoipVerifyVerb is cmd/console's hidden verb for the verifier child.
+const GeoipVerifyVerb = "__geoip-verify"
+
+// geoipVerifyResult is the child's report.
+type geoipVerifyResult struct {
+ Source string `json:"source"`
+ BuildEpoch int64 `json:"build_epoch"`
+ NodeCount uint64 `json:"node_count"`
+ Countries int `json:"countries"` // how many probe addresses answered a country
+}
+
+// RunGeoipVerify is the child: args are the file, then the probe
+// addresses. It writes the report to w and returns the exit code (0 ok, 2
+// refused). A panic or a hang here kills only this process.
+func RunGeoipVerify(args []string, w io.Writer) int {
+ if len(args) < 1 {
+ return 2
+ }
+ db, err := geo.OpenMMDBVerified(context.Background(), args[0])
+ if errors.Is(err, geo.ErrSearchTreeTooLarge) {
+ return geoipVerifyExitTree
+ }
+ if err != nil {
+ return 2
+ }
+ defer db.Close()
+ res := geoipVerifyResult{Source: db.Source(), BuildEpoch: db.BuildEpoch().Unix(), NodeCount: db.NodeCount()}
+ for _, a := range args[1:] {
+ if r, ok := db.Lookup(net.ParseIP(a)); ok && r.Country != "" {
+ res.Countries++
+ }
+ }
+ if err := json.NewEncoder(w).Encode(res); err != nil {
+ return 2
+ }
+ return 0
+}
+
+// geoipVerifyTimeout bounds the child; a var so a test need not wait.
+var geoipVerifyTimeout = 5 * time.Minute
+
+// geoipVerifyExec starts the child; tests point it at the test binary.
+var geoipVerifyExec = func(ctx context.Context, args ...string) *exec.Cmd {
+ self, err := os.Executable()
+ if err != nil {
+ self = os.Args[0]
+ }
+ return exec.CommandContext(ctx, self, append([]string{GeoipVerifyVerb}, args...)...)
+}
+
+var (
+ errGeoipVerifyTimeout = errors.New("verification took too long")
+ errGeoipVerifyTree = errors.New("malformed search tree")
+)
+
+// geoipVerifyExitTree is the child's exit code for a search tree with far
+// more networks than nodes.
+const geoipVerifyExitTree = 3
+
+// verifyInChild runs the child and returns its report. A crash, a refusal
+// or a malformed report is an invalid database; the deadline is its own
+// category; the caller's own cancellation is returned as it is.
+func verifyInChild(ctx context.Context, path string, probes []string) (geoipVerifyResult, error) {
+ vctx, cancel := context.WithTimeout(ctx, geoipVerifyTimeout)
+ defer cancel()
+ cmd := geoipVerifyExec(vctx, append([]string{path}, probes...)...)
+ var out bytes.Buffer
+ cmd.Stdout = &cappedBuffer{buf: &out, max: 4 << 10}
+ cmd.Stderr = io.Discard
+ cmd.WaitDelay = 5 * time.Second
+ err := cmd.Run()
+ switch {
+ case ctx.Err() != nil:
+ return geoipVerifyResult{}, ctx.Err()
+ case vctx.Err() != nil:
+ return geoipVerifyResult{}, errGeoipVerifyTimeout
+ case err != nil:
+ var ee *exec.ExitError
+ if errors.As(err, &ee) && ee.ExitCode() == geoipVerifyExitTree {
+ return geoipVerifyResult{}, errGeoipVerifyTree
+ }
+ return geoipVerifyResult{}, fmt.Errorf("verifier: %w", err)
+ }
+ var res geoipVerifyResult
+ if err := json.Unmarshal(out.Bytes(), &res); err != nil {
+ return geoipVerifyResult{}, fmt.Errorf("verifier report: %w", err)
+ }
+ return res, nil
+}
+
+type cappedBuffer struct {
+ buf *bytes.Buffer
+ max int
+}
+
+func (c *cappedBuffer) Write(p []byte) (int, error) {
+ if c.buf.Len()+len(p) > c.max {
+ return 0, errors.New("verifier report too large")
+ }
+ return c.buf.Write(p)
+}
+
+// VerifyGeoIPFile checks a database in the verifier child; nil means it is
+// safe to open in this process with geo.OpenMMDBTrusted. cmd/console uses
+// it for the CONSOLE_GEOIP_DB override, so even an operator's broken file
+// cannot crash the console at startup.
+func VerifyGeoIPFile(ctx context.Context, path string) error {
+ _, err := verifyInChild(ctx, path, nil)
+ return err
+}
diff --git a/internal/console/geoipverify_main_test.go b/internal/console/geoipverify_main_test.go
new file mode 100644
index 000000000..92ff7f034
--- /dev/null
+++ b/internal/console/geoipverify_main_test.go
@@ -0,0 +1,28 @@
+package console
+
+import (
+ "context"
+ "os"
+ "os/exec"
+ "testing"
+)
+
+// TestMain doubles as the GeoIP verifier child: the console re-runs its own
+// binary to verify an untrusted database (geoipverify.go), and in tests that
+// binary is this one.
+func TestMain(m *testing.M) {
+ if os.Getenv("CONSOLE_TEST_GEOIP_VERIFY_CHILD") == "1" {
+ for i, a := range os.Args {
+ if a == "--" {
+ os.Exit(RunGeoipVerify(os.Args[i+1:], os.Stdout))
+ }
+ }
+ os.Exit(2)
+ }
+ geoipVerifyExec = func(ctx context.Context, args ...string) *exec.Cmd {
+ cmd := exec.CommandContext(ctx, os.Args[0], append([]string{"-test.run=^$", "--"}, args...)...)
+ cmd.Env = append(os.Environ(), "CONSOLE_TEST_GEOIP_VERIFY_CHILD=1")
+ return cmd
+ }
+ os.Exit(m.Run())
+}
diff --git a/internal/geo/testdata/README b/internal/geo/testdata/README
index 5d6cc08e5..3a2460922 100644
--- a/internal/geo/testdata/README
+++ b/internal/geo/testdata/README
@@ -5,3 +5,9 @@ data from github.com/maxmind/MaxMind-DB, dual licensed MIT or Apache-2.0
`pathological-dag.mmdb` is not MaxMind's: a 32-node IPv4 tree whose two
children are both the next node (2^32 network paths), built for the geoip
code review R8-1 regression. It must be refused by `OpenMMDBVerified`.
+`pathological-empty-dag.mmdb` is a 128-node IPv6 tree whose children share
+the next node and end in the empty sentinel: maxminddb's walk spends 2^128
+steps inside one call. `pathological-metadata.mmdb` declares node_count
+2^63, which overflows the library's own arithmetic and panics it. Both are
+for the geoip code review R9 regressions: only the console's verifier
+child may ever open them.
diff --git a/internal/geo/testdata/pathological-dag.mmdb b/internal/geo/testdata/pathological-dag.mmdb
index 579d1fb21..bd190a8b6 100644
Binary files a/internal/geo/testdata/pathological-dag.mmdb and b/internal/geo/testdata/pathological-dag.mmdb differ
diff --git a/internal/geo/testdata/pathological-empty-dag.mmdb b/internal/geo/testdata/pathological-empty-dag.mmdb
new file mode 100644
index 000000000..d657a2880
Binary files /dev/null and b/internal/geo/testdata/pathological-empty-dag.mmdb differ
diff --git a/internal/geo/testdata/pathological-metadata.mmdb b/internal/geo/testdata/pathological-metadata.mmdb
new file mode 100644
index 000000000..337af8155
Binary files /dev/null and b/internal/geo/testdata/pathological-metadata.mmdb differ