mapcidr icon indicating copy to clipboard operation
mapcidr copied to clipboard

[Issue] Multiple bugs with AggregateApproxIPV4s and CoalesceCIDRs when used as library

Open nomaed opened this issue 2 years ago • 0 comments

There are multiple very unexpected results when working with mapcidr as a library. I assume this mostly won't normally happen on CLI runs (except item 1 in the list):

Describe the bug

  1. AggregateApproxIPV4s works incorrect when input contains masks (not always)
  2. AggregateApproxIPV4s modifies the input list (directly modifies ips[].IP)
  3. CoalesceCIDRs modifies the internal representation of the input array such that IPv4s are represented as 16-bytes long
  4. AggregateApproxIPV4s produces incorrect output when working directly on the result of CoalesceCIDRs due to item 3

Mapcidr version

  • v1.1.2
  • v1.1.16

Test file

package mapcidrtester

import (
	"net"
	"testing"

	"github.com/projectdiscovery/mapcidr"
	"github.com/stretchr/testify/assert"
)

// TestMaskedApproximation shows that mapcidr.AggregateApproxIPV4s is incorrectly treating input
// with a /24 mask, effectively reducing matched IPs compared to original
func TestMaskedApproximation(t *testing.T) {
	ipStrs := []string{
		"152.58.98.0/24",
		"152.58.98.5/32",
	}
	expected := []string{
		"152.58.98.0/24",
	}

	inputs := parseCIDRs(ipStrs)

	approximated := mapcidr.AggregateApproxIPV4s(inputs)
	assert.Equal(t, expected, ipNetsToStrs(approximated))
}

// TestApproximationInputModify shows that mapcidr.AggregateApproxIPV4s modifies the input value, thus
// making it unpredictable if value is later used by other code
func TestApproximationInputModify(t *testing.T) {
	ipStrs := []string{
		"10.10.0.0/31",
		"10.10.0.2/32",
		"10.10.0.255/32",
		"10.10.1.0/31",
		"10.10.1.2/32",
		"10.10.1.255/32",
	}
	expected := []string{
		"10.10.0.0/24",
		"10.10.1.0/24",
	}

	inputs := parseCIDRs(ipStrs)

	approximated := mapcidr.AggregateApproxIPV4s(inputs)
	assert.Equal(t, expected, ipNetsToStrs(approximated))

	assert.Equalf(t, parseCIDRs(ipStrs), inputs, "AggregateApproxIPV4s shouldn't modify inputs")
}

// TestApproximateCoalesced show that running mapcidr.AggregateApproxIPV4s on the result of mapcidr.CoalesceCIDRs
// produces an unexpected result, even though CoalesceCIDRs didn't actually modify the eventual "string" values.
// This is further demonstrated in TestUseCoalesced
func TestApproximateCoalesced(t *testing.T) {
	ipStrs := []string{
		"10.10.0.0/31",
		"10.10.0.2/32",
		"10.10.0.255/32",
		"10.10.1.0/31",
		"10.10.1.2/32",
		"10.10.1.255/32",
	}
	expected := []string{
		"10.10.0.0/24",
		"10.10.1.0/24",
	}

	// convert to IPNet masks
	inputs := parseCIDRs(ipStrs)

	coalesced, _ := mapcidr.CoalesceCIDRs(inputs)
	assert.Equalf(t, parseCIDRs(ipStrs), inputs, "CoalesceCIDRs shouldn't modify inputs")

	approximated := mapcidr.AggregateApproxIPV4s(coalesced)
	assert.Equal(t, expected, ipNetsToStrs(approximated))
}

// TestUseCoalesced shows that mapcidr.CoalesceCIDRs modifies the internal representation of IPv4 to be
// 16-bytes long, which affects how mapcidr.AggregateApproxIPV4s works, causing incorrect results, as
// demonstrated in TestApproximateCoalesced
func TestUseCoalesced(t *testing.T) {
	// these are already coalesced values, no further coalescing is needed
	ipStrs := []string{
		"10.10.0.0/31",
		"10.10.0.2/32",
		"10.10.0.255/32",
		"10.10.1.0/31",
		"10.10.1.2/32",
		"10.10.1.255/32",
	}
	expectedApprox := []string{
		"10.10.0.0/24",
		"10.10.1.0/24",
	}

	// convert to IPNet masks
	inputs := parseCIDRs(ipStrs)
	t.Logf("Inputs (%d): %s", len(inputs), inputs)

	// compact
	coalesced, _ := mapcidr.CoalesceCIDRs(inputs)
	assert.Equalf(t, ipStrs, ipNetsToStrs(coalesced), "CoalesceCIDRs should produce the same string output as was the input")
	assert.Equalf(t, inputs, coalesced, "CoalesceCIDRs should keep the same internal values in IPv4s")

	approximated := mapcidr.AggregateApproxIPV4s(coalesced)
	assert.Equal(t, expectedApprox, ipNetsToStrs(approximated))
}

func parseCIDRs(inp []string) []*net.IPNet {
	out := make([]*net.IPNet, len(inp))
	for i, pfx := range inp {
		_, mask, err := net.ParseCIDR(pfx)
		if err != nil {
			panic(err)
		}
		out[i] = mask
	}
	return out
}

func ipNetsToStrs(inp []*net.IPNet) []string {
	out := make([]string, len(inp))
	for i, ip := range inp {
		out[i] = ip.String()
	}
	return out
}

nomaed avatar Dec 12 '23 11:12 nomaed