Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nomaed opened this issue Dec 12, 2023 · 0 comments
Labels
Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.

Comments

@nomaed
Copy link

nomaed commented Dec 12, 2023

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
}
@ehsandeep ehsandeep added the Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors. label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Development

No branches or pull requests

2 participants