amadan

carlos / platform Public

geoip: the verifier child decodes every record through the lookup schema, so a record that would overflow a serving lookup's stack is refused (Astra code review R11 P1)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude d16d43c6cf58de52a399a5f04148436a79b33a1e parent 98c0139
3 files changed, +118 −0
  • internal/console/geoippublish_test.go +80 −0
  • internal/console/geoipverify.go +5 −0
  • internal/geo/mmdb.go +33 −0

CI passed — run details

diff --git a/internal/console/geoippublish_test.go b/internal/console/geoippublish_test.go
index 1f2082dc0..61098f3ab 100644
--- a/internal/console/geoippublish_test.go
+++ b/internal/console/geoippublish_test.go
@@ -515,3 +515,83 @@ func TestPublishSurvivesDatabasesThatHangOrPanicTheParser(t *testing.T) {
}
}
}
+
+// junkRecordMMDB writes an IPv4 database whose lower half answers GB and
+// whose upper half carries a 6M-element ignored array beside its country
+// (code review R11-1): generic verification walks the array, but the
+// production schema's skip recurses once per element.
+func junkRecordMMDB(t *testing.T) string {
+ t.Helper()
+ var buf bytes.Buffer
+ str := func(s string) { buf.WriteByte(0x40 | byte(len(s))); buf.WriteString(s) }
+ u16 := func(v uint16) { buf.WriteByte(0xA2); buf.WriteByte(byte(v >> 8)); buf.WriteByte(byte(v)) }
+ // One node: left -> data at 0 (A), right -> data at offsetB (B).
+ recA := []byte{0xE1, 0x47, 'c', 'o', 'u', 'n', 't', 'r', 'y', 0xE1, 0x48, 'i', 's', 'o', '_', 'c', 'o', 'd', 'e', 0x42, 'G', 'B'}
+ offsetB := len(recA)
+ ptr := func(dataOff int) []byte { v := 1 + 16 + dataOff; return []byte{byte(v >> 16), byte(v >> 8), byte(v)} }
+ buf.Write(ptr(0))
+ buf.Write(ptr(offsetB))
+ buf.Write(make([]byte, 16))
+ buf.Write(recA)
+ // B: {country: {iso_code: FR}, junk: [16,000,000 x false]}
+ buf.Write([]byte{0xE2, 0x47, 'c', 'o', 'u', 'n', 't', 'r', 'y', 0xE1, 0x48, 'i', 's', 'o', '_', 'c', 'o', 'd', 'e', 0x42, 'F', 'R', 0x44, 'j', 'u', 'n', 'k'})
+ const n = 6_000_000
+ v := n - 65821
+ buf.Write([]byte{0x1F, 0x04, byte(v >> 16), byte(v >> 8), byte(v)}) // array, size 31 form
+ falseBool := []byte{0x00, 0x07}
+ for i := 0; i < n; i++ {
+ buf.Write(falseBool)
+ }
+ buf.WriteString("\xAB\xCD\xEFMaxMind.com")
+ buf.WriteByte(0xE0 | 9)
+ str("node_count")
+ buf.Write([]byte{0xC1, 1})
+ str("record_size")
+ u16(24)
+ str("ip_version")
+ u16(4)
+ str("database_type")
+ str("Junk-Record")
+ str("languages")
+ buf.Write([]byte{0x01, 0x04})
+ str("en")
+ str("binary_format_major_version")
+ u16(2)
+ str("binary_format_minor_version")
+ u16(0)
+ str("build_epoch")
+ buf.Write([]byte{0x04, 0x02, 0x6a, 0xbc, 0x00, 0x00})
+ str("description")
+ buf.WriteByte(0xE1)
+ str("en")
+ str("junk")
+ p := filepath.Join(t.TempDir(), "junk.mmdb")
+ if err := os.WriteFile(p, buf.Bytes(), 0o644); err != nil {
+ t.Fatal(err)
+ }
+ return p
+}
+
+// Code review R11-1: a record that would overflow a serving lookup's stack
+// is refused by the verifier child, even when every probe address is fine.
+func TestPublishRefusesARecordThatWouldCrashALookup(t *testing.T) {
+ prevProbes := geoipProbeAddrs
+ geoipProbeAddrs = []string{"1.1.1.1", "8.8.8.8", "9.9.9.9"} // all in the clean half
+ t.Cleanup(func() { geoipProbeAddrs = prevProbes })
+ prev := geoipVerifyTimeout
+ geoipVerifyTimeout = 90 * time.Second
+ t.Cleanup(func() { geoipVerifyTimeout = prev })
+ ctx := context.Background()
+ p, _ := newTestPublisher(t)
+ f, err := os.Open(junkRecordMMDB(t))
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer f.Close()
+ if _, err := p.Publish(ctx, f, geoipPublishOpts{Origin: "manual"}); !errors.Is(err, errGeoipRefused) {
+ t.Fatalf("a crash-on-lookup database = %v, want refused", err)
+ }
+ if _, _, present, _ := p.Current(ctx); present {
+ t.Fatal("it was published")
+ }
+}
diff --git a/internal/console/geoipverify.go b/internal/console/geoipverify.go
index 496abaa90..1879f8d36 100644
--- a/internal/console/geoipverify.go
+++ b/internal/console/geoipverify.go
@@ -54,6 +54,11 @@ func RunGeoipVerify(args []string, w io.Writer) int {
return 2
}
defer db.Close()
+ // Every record through the production schema, here where a pathological
+ // one can only kill this process (code review R11-1).
+ if err := db.CheckRecords(context.Background()); err != nil {
+ return 2
+ }
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 != "" {
diff --git a/internal/geo/mmdb.go b/internal/geo/mmdb.go
index 9c12d240d..346672644 100644
--- a/internal/geo/mmdb.go
+++ b/internal/geo/mmdb.go
@@ -67,6 +67,39 @@ func OpenMMDBVerified(ctx context.Context, path string) (*MMDB, error) {
return newMMDB(r), nil
}
+// CheckRecords decodes every network's record through the schema live
+// lookups use (code review R11-1). maxminddb's Verify decodes records
+// generically, but a lookup decodes into mmdbRecord, which skips unknown
+// fields with a recursion one frame per value: a record carrying a huge
+// ignored array passes Verify and then overflows a serving process's stack
+// on the first lookup in its network. Run ONLY in the verifier child, under
+// its memory cap and deadline: there such a record kills the child, which
+// refuses the database. Aliased IPv4 networks are skipped (same records).
+func (m *MMDB) CheckRecords(ctx context.Context) error {
+ if m == nil || m.r == nil {
+ return errors.New("geo: no database")
+ }
+ limit := 4*m.nodes + 1024
+ it := m.r.Networks(maxminddb.SkipAliasedNetworks)
+ var n uint64
+ for it.Next() {
+ var rec mmdbRecord
+ if _, err := it.Network(&rec); err != nil {
+ return err
+ }
+ n++
+ if n > limit {
+ return ErrSearchTreeTooLarge
+ }
+ if n&0xffff == 0 {
+ if err := ctx.Err(); err != nil {
+ return err
+ }
+ }
+ }
+ return it.Err()
+}
+
func boundedTreeWalk(ctx context.Context, r *maxminddb.Reader) error {
limit := 4*uint64(r.Metadata.NodeCount) + 1024
it := r.Networks()