Add more defensive bounds and input checks #180
Conversation
oschwald
commented
Jul 27, 2025
oschwald
commented
- Fix integer overflow vulnerability in MMDB parsing
- Add input validation for NetworksWithin API
There was a problem hiding this comment.
Pull Request Overview
This PR adds defensive bounds checking and input validation to prevent security vulnerabilities in MMDB (MaxMind Database) parsing and traversal operations.
- Fixes integer overflow vulnerability in search tree size calculation during database initialization
- Adds comprehensive bounds checking to tree traversal functions to prevent out-of-bounds memory access
- Validates input prefixes in NetworksWithin API to handle invalid prefix parameters gracefully
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| reader.go | Adds integer overflow protection in FromBytes and comprehensive bounds checking in tree traversal functions |
| traverse.go | Adds input validation for NetworksWithin API and error handling for bounds check failures |
| reader_test.go | Adds test coverage for integer overflow protection and invalid prefix handling |
| CHANGELOG.md | Documents security fixes and new validation features |
Comments suppressed due to low confidence (1)
reader_test.go:1290
- The test creates an error object but doesn't verify that the actual FromBytes function would return this error. This test validates the overflow detection logic but doesn't test the integration with FromBytes. Consider adding an integration test that actually calls FromBytes with a malformed database file.
err := mmdberrors.NewInvalidDatabaseError("database tree size would overflow")
| @@ -499,29 +542,37 @@ func (r *Reader) traverseTree28(ip netip.Addr, node uint, stopBit int) (uint, in | |||
| bit := (uint(ip16[byteIdx]) >> bitPos) & 1 | |||
|
|
|||
| baseOffset := node * 7 | |||
| offset := baseOffset + bit*4 | |||
|
|
|||
| if baseOffset > bufferLen-4 || offset > bufferLen-3 { | |||
There was a problem hiding this comment.
The bounds check logic is inconsistent. For 28-bit records, both baseOffset and offset calculations need 4 bytes, but the check uses bufferLen-4 for baseOffset and bufferLen-3 for offset. Both should use bufferLen-4 since offset = baseOffset + bit*4 and we need to read 4 bytes from offset.
| if baseOffset > bufferLen-4 || offset > bufferLen-3 { | |
| if baseOffset > bufferLen-4 || offset > bufferLen-4 { |
Copilot uses AI. Check for mistakes.
| uint(buffer[offset+2]) | ||
| uint(buffer[offset+2]), nil | ||
| } | ||
| if offset > bufferLen-7 { |
There was a problem hiding this comment.
The bounds check for the second branch of 28-bit records uses bufferLen-7, but the actual read accesses buffer[offset+6]. This should be bufferLen-7+1 or bufferLen-6 to ensure buffer[offset+6] is within bounds.
| if offset > bufferLen-7 { | |
| if offset > bufferLen-6 { |
Copilot uses AI. Check for mistakes.
| @@ -101,6 +108,13 @@ func (r *Reader) NetworksWithin(prefix netip.Prefix, options ...NetworksOption) | |||
| stopBit += 96 | |||
| } | |||
|
|
|||
| if stopBit > 128 { | |||
There was a problem hiding this comment.
The validation stopBit > 128 should be stopBit >= 128 since IPv6 addresses have exactly 128 bits (0-127), so a stopBit of 128 would be invalid and could cause array bounds issues in tree traversal.
| if stopBit > 128 { | |
| if stopBit >= 128 { |
Copilot uses AI. Check for mistakes.
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.
Add validation for invalid prefixes in NetworksWithin to prevent unexpected behavior with malformed input. Invalid prefixes now return proper errors instead of potentially causing undefined behavior. Includes test coverage demonstrating proper error handling patterns for invalid prefix scenarios.
Add required default cases to all switch statements to satisfy golangci-lint enforce-switch-style rule. Uses fall-through pattern to maintain clean code without duplicating error returns.
