maxmind / mmdbwriter

Go library for writing MaxMind DB (mmdb) files
Apache License 2.0
116 stars 28 forks source link

mismatched ip and mask size upon tree lookup results in invalid ipnet #62

Closed oltdaniel closed 1 year ago

oltdaniel commented 1 year ago

NOTE: This is a issue that does not happen if the developer does not work with netip or manual constructions of net.IP that does not match the maximum size of an IP of the given tree options. More details below.

I'm currently playing around with this library and I ran into the issue of mismatched IP Mask sizes when using a mixed IPV4/IPv6 compatible tree. This targets the following line within this library: https://github.com/maxmind/mmdbwriter/blob/bb691ac3530cb14dad0f9c26a2d3d59ac68dbd63/tree.go#L423 or https://github.com/maxmind/mmdbwriter/blob/bb691ac3530cb14dad0f9c26a2d3d59ac68dbd63/tree.go#L431

NOTE: The fix can be matching the size with the input ip, or matching the input ip with the mask size. More details below.

Example

Here is a minimal example code to produce the issue I had including "the correct way" for a lookup:

package main

import (
    "fmt"
    "log"
    "net"

    "github.com/maxmind/mmdbwriter"
    "github.com/maxmind/mmdbwriter/mmdbtype"
)

func main() {
    // Create a new writer
    writer, err := mmdbwriter.New(mmdbwriter.Options{DatabaseType: "Bug Report"})
    if err != nil {
        log.Fatal(err)
    }

    // IP we will use
    ip, network, _ := net.ParseCIDR("1.0.0.0/24")

    // Write a small sample
    err = writer.Insert(network, mmdbtype.Map{"country_code": mmdbtype.String("AU")})
    if err != nil {
        log.Fatal(err)
    }

    // "Incorrect" way for lookup
    {
        rNetwork, rData := writer.Get(net.IP{0x01, 0, 0, 0}) // This is a valid IP struct as referenced below
        fmt.Println(rNetwork, rData.(mmdbtype.Map)["country_code"]) // rNetwork should not print <nil>
    }

    // "Correct" way for lookup
    {
        rNetwork, rData := writer.Get(ip) // `net.IPv4(0x01, 0, 0, 0)` would also be possible as this is more explicit to represent a IPv4 with 16bytes
        fmt.Println(rNetwork, rData.(mmdbtype.Map)["country_code"])
    }
}

"Bug" in this library

The only issue is the manual construction of the *net.IPNet here: https://github.com/maxmind/mmdbwriter/blob/bb691ac3530cb14dad0f9c26a2d3d59ac68dbd63/tree.go#L430

Everything can be followed back to this specific sentence in the documentation for net.IP (here): "[...] accept either 4-byte (IPv4) or 16-byte (IPv6) slices as input. [...]". Internally they are not able to to automatically match the IP and Mask sizes, e.g. https://cs.opensource.google/go/go/+/refs/tags/go1.20.6:src/net/ip.go;l=497 , which results in an "incorrect" net.IPNet struct.

Thereby, matching the byte length of the input ip with the mask should fix this bug (or the other way around). A fix would be really helpful, as using netip is really useful to e.g. jump to the next ip. I'm also willing to submit a fix on my own if requested.

oschwald commented 1 year ago

Thanks for the detailed report! I believe #65 will fix the issue.

oltdaniel commented 1 year ago

Thanks for the detailed report! I believe #65 will fix the issue.

Thanks for the quick fix! Will test the code and give some feedback in the PR.