amadan

carlos / platform Public

geo: bounded, cancellable verification refuses search trees with far more networks than nodes (Astra code review R8 P1)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude ad70f841286c1af4cbc2d078933b1ab109b10f4a parent 29455f2
6 files changed, +178 −1
  • internal/console/geoippublish.go +7 −1
  • internal/console/geoippublish_test.go +30 −0
  • internal/geo/mmdb.go +40 −0
  • internal/geo/pathological_test.go +97 −0
  • internal/geo/testdata/README.md +4 −0
  • internal/geo/testdata/pathological-dag.mmdb binary

CI passed — run details

diff --git a/internal/console/geoippublish.go b/internal/console/geoippublish.go
index 2f78e2cf6..7df532af7 100644
--- a/internal/console/geoippublish.go
+++ b/internal/console/geoippublish.go
@@ -213,8 +213,14 @@ func (p *geoipPublisher) Commit(ctx context.Context, st *geoipStaged, o geoipPub
ctx, cancel = context.WithTimeout(ctx, p.deadline)
defer cancel()
}
- db, err := geo.OpenMMDB(st.path)
+ db, err := geo.OpenMMDBVerified(ctx, st.path)
+ if errors.Is(err, geo.ErrSearchTreeTooLarge) {
+ return geoip.Pointer{}, refused("the database's search tree is malformed")
+ }
if err != nil {
+ if ctx.Err() != nil {
+ return geoip.Pointer{}, ctx.Err()
+ }
return geoip.Pointer{}, refused("not a valid MaxMind-format database")
}
defer db.Close()
diff --git a/internal/console/geoippublish_test.go b/internal/console/geoippublish_test.go
index de2d26e05..dba5b3d33 100644
--- a/internal/console/geoippublish_test.go
+++ b/internal/console/geoippublish_test.go
@@ -451,3 +451,33 @@ func TestStageGivesUpOnAStalledStoreAndFreesTheSlot(t *testing.T) {
}
p.mu.Unlock()
}
+
+// Code review R8-1: a crafted tree is refused in time, the slot is freed,
+// staging is cleaned and the previous pointer survives.
+func TestPublishRefusesAPathologicalTreeInTime(t *testing.T) {
+ withFixtureProbes(t)
+ 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)
+ }
+ start := time.Now()
+ _, err = p.Publish(ctx, bytes.NewReader(fixture(t, "pathological-dag.mmdb")), geoipPublishOpts{Origin: "manual", Force: true})
+ if !errors.Is(err, errGeoipRefused) || !strings.Contains(err.Error(), "search tree") {
+ t.Fatalf("err = %v", err)
+ }
+ if time.Since(start) > 5*time.Second {
+ t.Fatal("took too long")
+ }
+ if cur, _, _, _ := p.Current(ctx); cur.SHA256 != good.SHA256 {
+ t.Fatal("the previous pointer was replaced")
+ }
+ if !p.mu.TryLock() {
+ t.Fatal("slot still held")
+ }
+ p.mu.Unlock()
+ if entries, _ := os.ReadDir(p.tmpRoot); len(entries) != 0 {
+ t.Fatalf("staging left behind: %v", entries)
+ }
+}
diff --git a/internal/geo/mmdb.go b/internal/geo/mmdb.go
index d547a79d9..9c12d240d 100644
--- a/internal/geo/mmdb.go
+++ b/internal/geo/mmdb.go
@@ -1,6 +1,8 @@
package geo
import (
+ "context"
+ "errors"
"log"
"math"
"net"
@@ -34,10 +36,30 @@ type MMDB struct {
// OpenMMDB opens and verifies the file. An error means "IP step off"; the
// console logs it once and starts anyway.
func OpenMMDB(path string) (*MMDB, error) {
+ return OpenMMDBVerified(context.Background(), path)
+}
+
+// ErrSearchTreeTooLarge is a database whose search tree has far more paths
+// than nodes: shared subtrees (a DAG) make the library's verifier, which
+// enumerates every network, run for an exponential time.
+var ErrSearchTreeTooLarge = errors.New("geo: the database's search tree has far more networks than nodes")
+
+// OpenMMDBVerified is OpenMMDB with the verification bounded (code review
+// R8-1). maxminddb's Verify walks every network path with no visited set,
+// so a crafted file whose nodes share children enumerates 2^depth paths
+// and never returns. A pre-pass over the same walk, capped at a small
+// multiple of the node count (a real tree has about one network per node,
+// times a few for the IPv4 aliases) and checking ctx, refuses such a file
+// in time before Verify runs.
+func OpenMMDBVerified(ctx context.Context, path string) (*MMDB, error) {
r, err := maxminddb.Open(path)
if err != nil {
return nil, err
}
+ if err := boundedTreeWalk(ctx, r); err != nil {
+ r.Close()
+ return nil, err
+ }
if err := r.Verify(); err != nil {
r.Close()
return nil, err
@@ -45,6 +67,24 @@ func OpenMMDB(path string) (*MMDB, error) {
return newMMDB(r), nil
}
+func boundedTreeWalk(ctx context.Context, r *maxminddb.Reader) error {
+ limit := 4*uint64(r.Metadata.NodeCount) + 1024
+ it := r.Networks()
+ var n uint64
+ for it.Next() {
+ n++
+ if n > limit {
+ return ErrSearchTreeTooLarge
+ }
+ if n&0xffff == 0 {
+ if err := ctx.Err(); err != nil {
+ return err
+ }
+ }
+ }
+ return it.Err()
+}
+
// OpenMMDBTrusted opens without Verify. Only for a file whose bytes were
// verified elsewhere and proven identical by digest (geoip spec §5.3):
// Verify decodes the whole data section, which costs seconds and tens of
diff --git a/internal/geo/pathological_test.go b/internal/geo/pathological_test.go
new file mode 100644
index 000000000..5064b0473
--- /dev/null
+++ b/internal/geo/pathological_test.go
@@ -0,0 +1,97 @@
+package geo
+
+import (
+ "bytes"
+ "context"
+ "encoding/binary"
+ "errors"
+ "os"
+ "path/filepath"
+ "testing"
+ "time"
+)
+
+// pathologicalMMDB builds a structurally valid IPv4 database of 32 nodes
+// whose two children are both the next node: 2^32 network paths through 32
+// nodes (code review R8-1).
+func pathologicalMMDB(t *testing.T) string {
+ t.Helper()
+ const nodes = 32
+ var buf bytes.Buffer
+ rec := func(v uint32) { buf.Write([]byte{byte(v >> 16), byte(v >> 8), byte(v)}) } // 24-bit records
+ for i := 0; i < nodes; i++ {
+ next := uint32(i + 1)
+ if i == nodes-1 {
+ next = nodes + 16 // data section offset 0
+ }
+ rec(next)
+ rec(next)
+ }
+ buf.Write(make([]byte, 16)) // data section separator
+ buf.WriteByte(0xE0) // the one record: an empty map
+ str := func(s string) { buf.WriteByte(0x40 | byte(len(s))); buf.WriteString(s) }
+ u16 := func(v uint16) { buf.WriteByte(0xA2); binary.Write(&buf, binary.BigEndian, v) }
+ u32 := func(v uint32) { buf.WriteByte(0xC4); binary.Write(&buf, binary.BigEndian, v) }
+ buf.WriteString("\xAB\xCD\xEFMaxMind.com")
+ buf.WriteByte(0xE0 | 9) // metadata map, 9 keys
+ str("node_count")
+ u32(nodes)
+ str("record_size")
+ u16(24)
+ str("ip_version")
+ u16(4)
+ str("database_type")
+ str("Pathological")
+ str("languages")
+ buf.Write([]byte{0x00, 0x04}) // array of 0
+ str("binary_format_major_version")
+ u16(2)
+ str("binary_format_minor_version")
+ u16(0)
+ str("build_epoch")
+ buf.Write([]byte{0x08, 0x02}) // uint64, 8 bytes
+ binary.Write(&buf, binary.BigEndian, uint64(time.Now().Unix()))
+ str("description")
+ buf.WriteByte(0xE0) // empty map
+ p := filepath.Join(t.TempDir(), "dag.mmdb")
+ if err := os.WriteFile(p, buf.Bytes(), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ return p
+}
+
+func TestOpenMMDBRefusesAPathologicalTreeInTime(t *testing.T) {
+ p := pathologicalMMDB(t)
+ start := time.Now()
+ _, err := OpenMMDBVerified(context.Background(), p)
+ if !errors.Is(err, ErrSearchTreeTooLarge) {
+ t.Fatalf("err = %v, want ErrSearchTreeTooLarge", err)
+ }
+ if d := time.Since(start); d > 5*time.Second {
+ t.Fatalf("took %s", d)
+ }
+ // The real fixtures still open.
+ for _, f := range []string{"GeoIP2-City-Test.mmdb", "GeoIP2-Country-Test.mmdb", "MaxMind-DB-test-ipv4-24.mmdb"} {
+ m, err := OpenMMDBVerified(context.Background(), "testdata/"+f)
+ if err != nil {
+ t.Fatalf("%s: %v", f, err)
+ }
+ m.Close()
+ }
+}
+
+// TestRealDatabaseOpensVerified runs only with GEOIP_REAL_MMDB set to a real
+// database (DB-IP Lite, GeoLite2): the bounded walk must accept it.
+func TestRealDatabaseOpensVerified(t *testing.T) {
+ p := os.Getenv("GEOIP_REAL_MMDB")
+ if p == "" {
+ t.Skip("set GEOIP_REAL_MMDB to a real .mmdb")
+ }
+ start := time.Now()
+ m, err := OpenMMDBVerified(context.Background(), p)
+ if err != nil {
+ t.Fatal(err)
+ }
+ m.Close()
+ t.Logf("%s verified in %s", m.Source(), time.Since(start))
+}
diff --git a/internal/geo/testdata/README.md b/internal/geo/testdata/README.md
new file mode 100644
index 000000000..99d81ddbc
--- /dev/null
+++ b/internal/geo/testdata/README.md
@@ -0,0 +1,4 @@
+
+`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`.
diff --git a/internal/geo/testdata/pathological-dag.mmdb b/internal/geo/testdata/pathological-dag.mmdb
new file mode 100644
index 000000000..579d1fb21
Binary files /dev/null and b/internal/geo/testdata/pathological-dag.mmdb differ