Skip to content

Commit 8059668

Browse files
oschwaldoschwald
committed
Fix integer overflow vulnerability in MMDB parsing
Add overflow protection to prevent potential security issues from malformed databases with excessive NodeCount values. Enhanced bounds checking in readNodeBySize to return proper errors instead of silent failures when encountering bounds violations. The fix validates that NodeCount * (RecordSize / 4) will not overflow before performing the calculation, and ensures tree traversal functions properly handle malformed database structures.
1 parent 31c2c6c commit 8059668

File tree

2 files changed

+161
-24
lines changed

2 files changed

+161
-24
lines changed

‎reader.go

Lines changed: 88 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,17 @@ func FromBytes(buffer []byte, options ...ReaderOption) (*Reader, error) {
301301
return nil, err
302302
}
303303

304+
// Check for integer overflow in search tree size calculation
305+
if metadata.NodeCount > 0 && metadata.RecordSize > 0 {
306+
recordSizeQuarter := metadata.RecordSize / 4
307+
if recordSizeQuarter > 0 {
308+
maxNodes := ^uint(0) / recordSizeQuarter
309+
if metadata.NodeCount > maxNodes {
310+
return nil, mmdberrors.NewInvalidDatabaseError("database tree size would overflow")
311+
}
312+
}
313+
}
314+
304315
searchTreeSize := metadata.NodeCount * (metadata.RecordSize / 4)
305316
dataSectionStart := searchTreeSize + dataSectionSeparatorSize
306317
dataSectionEnd := uint(metadataStart - len(metadataStartMarker))
@@ -376,7 +387,13 @@ func (r *Reader) setIPv4Start() {
376387
node := uint(0)
377388
i := 0
378389
for ; i < 96 && node < nodeCount; i++ {
379-
node = readNodeBySize(r.buffer, node*r.nodeOffsetMult, 0, r.Metadata.RecordSize)
390+
var err error
391+
node, err = readNodeBySize(r.buffer, node*r.nodeOffsetMult, 0, r.Metadata.RecordSize)
392+
if err != nil {
393+
// If we encounter a bounds error during IPv4 start calculation,
394+
// fall back to treating it as an IPv4-only database
395+
break
396+
}
380397
}
381398
r.ipv4Start = node
382399
r.ipv4StartBitDepth = i
@@ -409,46 +426,64 @@ func (r *Reader) lookupPointer(ip netip.Addr) (uint, int, error) {
409426
}
410427

411428
// readNodeBySize reads a node value from the buffer based on record size and bit.
412-
func readNodeBySize(buffer []byte, offset, bit, recordSize uint) uint {
429+
func readNodeBySize(buffer []byte, offset, bit, recordSize uint) (uint, error) {
430+
bufferLen := uint(len(buffer))
413431
switch recordSize {
414432
case 24:
415433
offset += bit * 3
434+
if offset > bufferLen-3 {
435+
return 0, mmdberrors.NewInvalidDatabaseError(
436+
"bounds check failed: insufficient buffer for 24-bit node read",
437+
)
438+
}
416439
return (uint(buffer[offset]) << 16) |
417440
(uint(buffer[offset+1]) << 8) |
418-
uint(buffer[offset+2])
441+
uint(buffer[offset+2]), nil
419442
case 28:
420443
if bit == 0 {
444+
if offset > bufferLen-4 {
445+
return 0, mmdberrors.NewInvalidDatabaseError(
446+
"bounds check failed: insufficient buffer for 28-bit node read",
447+
)
448+
}
421449
return ((uint(buffer[offset+3]) & 0xF0) << 20) |
422450
(uint(buffer[offset]) << 16) |
423451
(uint(buffer[offset+1]) << 8) |
424-
uint(buffer[offset+2])
452+
uint(buffer[offset+2]), nil
453+
}
454+
if offset > bufferLen-7 {
455+
return 0, mmdberrors.NewInvalidDatabaseError(
456+
"bounds check failed: insufficient buffer for 28-bit node read",
457+
)
425458
}
426459
return ((uint(buffer[offset+3]) & 0x0F) << 24) |
427460
(uint(buffer[offset+4]) << 16) |
428461
(uint(buffer[offset+5]) << 8) |
429-
uint(buffer[offset+6])
462+
uint(buffer[offset+6]), nil
430463
case 32:
431464
offset += bit * 4
465+
if offset > bufferLen-4 {
466+
return 0, mmdberrors.NewInvalidDatabaseError(
467+
"bounds check failed: insufficient buffer for 32-bit node read",
468+
)
469+
}
432470
return (uint(buffer[offset]) << 24) |
433471
(uint(buffer[offset+1]) << 16) |
434472
(uint(buffer[offset+2]) << 8) |
435-
uint(buffer[offset+3])
473+
uint(buffer[offset+3]), nil
436474
default:
437-
return 0
475+
return 0, mmdberrors.NewInvalidDatabaseError("unsupported record size")
438476
}
439477
}
440478

441479
func (r *Reader) traverseTree(ip netip.Addr, node uint, stopBit int) (uint, int, error) {
442480
switch r.Metadata.RecordSize {
443481
case 24:
444-
n, i := r.traverseTree24(ip, node, stopBit)
445-
return n, i, nil
482+
return r.traverseTree24(ip, node, stopBit)
446483
case 28:
447-
n, i := r.traverseTree28(ip, node, stopBit)
448-
return n, i, nil
484+
return r.traverseTree28(ip, node, stopBit)
449485
case 32:
450-
n, i := r.traverseTree32(ip, node, stopBit)
451-
return n, i, nil
486+
return r.traverseTree32(ip, node, stopBit)
452487
default:
453488
return 0, 0, mmdberrors.NewInvalidDatabaseError(
454489
"unsupported record size: %d",
@@ -457,14 +492,15 @@ func (r *Reader) traverseTree(ip netip.Addr, node uint, stopBit int) (uint, int,
457492
}
458493
}
459494

460-
func (r *Reader) traverseTree24(ip netip.Addr, node uint, stopBit int) (uint, int) {
495+
func (r *Reader) traverseTree24(ip netip.Addr, node uint, stopBit int) (uint, int, error) {
461496
i := 0
462497
if ip.Is4() {
463498
i = r.ipv4StartBitDepth
464499
node = r.ipv4Start
465500
}
466501
nodeCount := r.Metadata.NodeCount
467502
buffer := r.buffer
503+
bufferLen := uint(len(buffer))
468504
ip16 := ip.As16()
469505

470506
for ; i < stopBit && node < nodeCount; i++ {
@@ -475,22 +511,29 @@ func (r *Reader) traverseTree24(ip netip.Addr, node uint, stopBit int) (uint, in
475511
baseOffset := node * 6
476512
offset := baseOffset + bit*3
477513

514+
if offset > bufferLen-3 {
515+
return 0, 0, mmdberrors.NewInvalidDatabaseError(
516+
"bounds check failed during tree traversal",
517+
)
518+
}
519+
478520
node = (uint(buffer[offset]) << 16) |
479521
(uint(buffer[offset+1]) << 8) |
480522
uint(buffer[offset+2])
481523
}
482524

483-
return node, i
525+
return node, i, nil
484526
}
485527

486-
func (r *Reader) traverseTree28(ip netip.Addr, node uint, stopBit int) (uint, int) {
528+
func (r *Reader) traverseTree28(ip netip.Addr, node uint, stopBit int) (uint, int, error) {
487529
i := 0
488530
if ip.Is4() {
489531
i = r.ipv4StartBitDepth
490532
node = r.ipv4Start
491533
}
492534
nodeCount := r.Metadata.NodeCount
493535
buffer := r.buffer
536+
bufferLen := uint(len(buffer))
494537
ip16 := ip.As16()
495538

496539
for ; i < stopBit && node < nodeCount; i++ {
@@ -499,29 +542,37 @@ func (r *Reader) traverseTree28(ip netip.Addr, node uint, stopBit int) (uint, in
499542
bit := (uint(ip16[byteIdx]) >> bitPos) & 1
500543

501544
baseOffset := node * 7
545+
offset := baseOffset + bit*4
546+
547+
if baseOffset > bufferLen-4 || offset > bufferLen-3 {
548+
return 0, 0, mmdberrors.NewInvalidDatabaseError(
549+
"bounds check failed during tree traversal",
550+
)
551+
}
552+
502553
sharedByte := uint(buffer[baseOffset+3])
503554
mask := uint(0xF0 >> (bit * 4))
504555
shift := 20 + bit*4
505556
nibble := ((sharedByte & mask) << shift)
506-
offset := baseOffset + bit*4
507557

508558
node = nibble |
509559
(uint(buffer[offset]) << 16) |
510560
(uint(buffer[offset+1]) << 8) |
511561
uint(buffer[offset+2])
512562
}
513563

514-
return node, i
564+
return node, i, nil
515565
}
516566

517-
func (r *Reader) traverseTree32(ip netip.Addr, node uint, stopBit int) (uint, int) {
567+
func (r *Reader) traverseTree32(ip netip.Addr, node uint, stopBit int) (uint, int, error) {
518568
i := 0
519569
if ip.Is4() {
520570
i = r.ipv4StartBitDepth
521571
node = r.ipv4Start
522572
}
523573
nodeCount := r.Metadata.NodeCount
524574
buffer := r.buffer
575+
bufferLen := uint(len(buffer))
525576
ip16 := ip.As16()
526577

527578
for ; i < stopBit && node < nodeCount; i++ {
@@ -532,20 +583,33 @@ func (r *Reader) traverseTree32(ip netip.Addr, node uint, stopBit int) (uint, in
532583
baseOffset := node * 8
533584
offset := baseOffset + bit*4
534585

586+
if offset > bufferLen-4 {
587+
return 0, 0, mmdberrors.NewInvalidDatabaseError(
588+
"bounds check failed during tree traversal",
589+
)
590+
}
591+
535592
node = (uint(buffer[offset]) << 24) |
536593
(uint(buffer[offset+1]) << 16) |
537594
(uint(buffer[offset+2]) << 8) |
538595
uint(buffer[offset+3])
539596
}
540597

541-
return node, i
598+
return node, i, nil
542599
}
543600

544601
func (r *Reader) resolveDataPointer(pointer uint) (uintptr, error) {
545-
resolved := uintptr(pointer - r.Metadata.NodeCount - dataSectionSeparatorSize)
546-
547-
if resolved >= uintptr(len(r.buffer)) {
602+
// Check for integer underflow: pointer must be greater than nodeCount + separator
603+
minPointer := r.Metadata.NodeCount + dataSectionSeparatorSize
604+
if pointer >= minPointer {
605+
resolved := uintptr(pointer - minPointer)
606+
bufferLen := uintptr(len(r.buffer))
607+
if resolved < bufferLen {
608+
return resolved, nil
609+
}
610+
// Error case - bounds exceeded
548611
return 0, mmdberrors.NewInvalidDatabaseError("the MaxMind DB file's search tree is corrupt")
549612
}
550-
return resolved, nil
613+
// Error case - underflow
614+
return 0, mmdberrors.NewInvalidDatabaseError("the MaxMind DB file's search tree is corrupt")
551615
}

‎reader_test.go

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,3 +1248,76 @@ func TestMetadataBuildTime(t *testing.T) {
12481248
assert.True(t, buildTime.After(time.Date(2010, 1, 1, 0, 0, 0, 0, time.UTC)))
12491249
assert.True(t, buildTime.Before(time.Date(2030, 1, 1, 0, 0, 0, 0, time.UTC)))
12501250
}
1251+
1252+
func TestIntegerOverflowProtection(t *testing.T) {
1253+
// Test that FromBytes detects integer overflow in search tree size calculation
1254+
t.Run("NodeCount overflow protection", func(t *testing.T) {
1255+
// Create metadata that would cause overflow: very large NodeCount
1256+
// For a 64-bit system with RecordSize=32, this should trigger overflow
1257+
// RecordSize/4 = 8, so maxNodes would be ^uint(0)/8
1258+
// We'll use a NodeCount larger than this limit
1259+
overflowNodeCount := ^uint(0)/8 + 1000 // Guaranteed to overflow
1260+
1261+
// Build minimal metadata map structure in MMDB format
1262+
// This is simplified - in a real MMDB, metadata is encoded differently
1263+
// But we can't easily create a valid MMDB file structure in a unit test
1264+
// So this test verifies the logic with mocked values
1265+
1266+
// Create a test by directly calling the validation logic
1267+
metadata := Metadata{
1268+
NodeCount: overflowNodeCount,
1269+
RecordSize: 32, // 32 bits = 4 bytes, so RecordSize/4 = 8
1270+
}
1271+
1272+
// Test the overflow detection logic directly
1273+
recordSizeQuarter := metadata.RecordSize / 4
1274+
maxNodes := ^uint(0) / recordSizeQuarter
1275+
1276+
// Verify our test setup is correct
1277+
assert.Greater(t, metadata.NodeCount, maxNodes,
1278+
"Test setup error: NodeCount should exceed maxNodes for overflow test")
1279+
1280+
// Since we can't easily create an invalid MMDB file that parses but has overflow values,
1281+
// we test the core logic validation here and rely on integration tests
1282+
// for the full FromBytes flow
1283+
1284+
if metadata.NodeCount > 0 && metadata.RecordSize > 0 {
1285+
recordSizeQuarter := metadata.RecordSize / 4
1286+
if recordSizeQuarter > 0 {
1287+
maxNodes := ^uint(0) / recordSizeQuarter
1288+
if metadata.NodeCount > maxNodes {
1289+
// This is what should happen in FromBytes
1290+
err := mmdberrors.NewInvalidDatabaseError("database tree size would overflow")
1291+
assert.Equal(t, "database tree size would overflow", err.Error())
1292+
}
1293+
}
1294+
}
1295+
})
1296+
1297+
t.Run("Valid large values should not trigger overflow", func(t *testing.T) {
1298+
// Test that reasonable large values don't trigger false positives
1299+
metadata := Metadata{
1300+
NodeCount: 1000000, // 1 million nodes
1301+
RecordSize: 32,
1302+
}
1303+
1304+
recordSizeQuarter := metadata.RecordSize / 4
1305+
maxNodes := ^uint(0) / recordSizeQuarter
1306+
1307+
// Verify this doesn't trigger overflow
1308+
assert.LessOrEqual(t, metadata.NodeCount, maxNodes,
1309+
"Valid large NodeCount should not trigger overflow protection")
1310+
})
1311+
1312+
t.Run("Edge case: RecordSize/4 is 0", func(t *testing.T) {
1313+
// Test edge case where RecordSize/4 could be 0
1314+
recordSize := uint(3) // 3/4 = 0 in integer division
1315+
1316+
recordSizeQuarter := recordSize / 4
1317+
// Should be 0, which means no overflow check is performed
1318+
assert.Equal(t, uint(0), recordSizeQuarter)
1319+
1320+
// The overflow protection should skip when recordSizeQuarter is 0
1321+
// This tests the condition: if recordSizeQuarter > 0
1322+
})
1323+
}

0 commit comments

Comments
 (0)