Fix crash on ListPathRequest with malformed prefix

When ListPathRequest is done by a gRPC client including a malformed prefix, the server would crash an invalid memory address reference. This commit fixes the crash by checking whether the parseCIDR method returned an error.
osrg · Sep 7, 2023 · b6be999 · b6be999
1 parent e7e0007
commit b6be999
Show file tree

Hide file tree

Showing 4 changed files with 128 additions and 12 deletions.
diff --git a/internal/pkg/table/table.go b/internal/pkg/table/table.go
@@ -467,20 +467,24 @@ func (t *Table) Select(option ...TableSelectOption) (*Table, error) {
 	if len(prefixes) != 0 {
 		switch t.routeFamily {
 		case bgp.RF_IPv4_UC, bgp.RF_IPv6_UC:
-			f := func(prefixStr string) bool {
+			f := func(prefixStr string) (bool, error) {
 				var nlri bgp.AddrPrefixInterface
+				var err error
 				if t.routeFamily == bgp.RF_IPv4_UC {
-					nlri, _ = bgp.NewPrefixFromRouteFamily(bgp.AFI_IP, bgp.SAFI_UNICAST, prefixStr)
+					nlri, err = bgp.NewPrefixFromRouteFamily(bgp.AFI_IP, bgp.SAFI_UNICAST, prefixStr)
 				} else {
-					nlri, _ = bgp.NewPrefixFromRouteFamily(bgp.AFI_IP6, bgp.SAFI_UNICAST, prefixStr)
+					nlri, err = bgp.NewPrefixFromRouteFamily(bgp.AFI_IP6, bgp.SAFI_UNICAST, prefixStr)
+				}
+				if err != nil {
+					return false, err
 				}
 				if dst := t.GetDestination(nlri); dst != nil {
 					if d := dst.Select(dOption); d != nil {
 						r.setDestination(d)
-						return true
+						return true, nil
 					}
 				}
-				return false
+				return false, nil
 			}
 
 			for _, p := range prefixes {
@@ -517,7 +521,11 @@ func (t *Table) Select(option ...TableSelectOption) (*Table, error) {
 							if err != nil {
 								return nil, err
 							}
-							if f(prefix.String()) {
+							ret, err := f(prefix.String())
+							if err != nil {
+								return nil, err
+							}
+							if ret {
 								break
 							}
 						}

diff --git a/internal/pkg/table/table_test.go b/internal/pkg/table/table_test.go
@@ -103,6 +103,84 @@ func TestTableKey(t *testing.T) {
 	assert.Equal(t, len(tb.GetDestinations()), 2)
 }
 
+func TestTableSelectMalformedIPv4UCPrefixes(t *testing.T) {
+	table := NewTable(logger, bgp.RF_IPv4_UC)
+	assert.Equal(t, 0, len(table.GetDestinations()))
+
+	tests := []struct {
+		name   string
+		prefix string
+		option LookupOption
+		found  int
+	}{
+		{
+			name:   "Malformed IPv4 Address",
+			prefix: "2.2.2.2.2",
+			option: LOOKUP_EXACT,
+			found:  0,
+		},
+		{
+			name:   "exact match with RD and prefix that does not exist",
+			prefix: "foo",
+			option: LOOKUP_EXACT,
+			found:  0,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			filteredTable, _ := table.Select(
+				TableSelectOption{
+					LookupPrefixes: []*LookupPrefix{{
+						Prefix:       tt.prefix,
+						LookupOption: tt.option,
+					}},
+				},
+			)
+			assert.Equal(t, tt.found, len(filteredTable.GetDestinations()))
+		})
+	}
+}
+
+func TestTableSelectMalformedIPv6UCPrefixes(t *testing.T) {
+	table := NewTable(logger, bgp.RF_IPv6_UC)
+	assert.Equal(t, 0, len(table.GetDestinations()))
+
+	tests := []struct {
+		name   string
+		prefix string
+		option LookupOption
+		found  int
+	}{
+		{
+			name:   "Malformed IPv6 Address: 3343:faba:3903:128::::/63",
+			prefix: "3343:faba:3903:128::::/63",
+			option: LOOKUP_EXACT,
+			found:  0,
+		},
+		{
+			name:   "Malformed IPv6 Address: foo",
+			prefix: "foo",
+			option: LOOKUP_EXACT,
+			found:  0,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			filteredTable, _ := table.Select(
+				TableSelectOption{
+					LookupPrefixes: []*LookupPrefix{{
+						Prefix:       tt.prefix,
+						LookupOption: tt.option,
+					}},
+				},
+			)
+			assert.Equal(t, tt.found, len(filteredTable.GetDestinations()))
+		})
+	}
+}
+
 func TestTableSelectVPNv4(t *testing.T) {
 	prefixes := []string{
 		"100:100:2.2.2.0/25",

diff --git a/pkg/packet/bgp/bgp.go b/pkg/packet/bgp/bgp.go
@@ -9604,26 +9604,29 @@ func GetRouteFamily(name string) (RouteFamily, error) {
 func NewPrefixFromRouteFamily(afi uint16, safi uint8, prefixStr ...string) (prefix AddrPrefixInterface, err error) {
 	family := AfiSafiToRouteFamily(afi, safi)
 
-	f := func(s string) AddrPrefixInterface {
-		addr, net, _ := net.ParseCIDR(s)
+	f := func(s string) (AddrPrefixInterface, error) {
+		addr, net, err := net.ParseCIDR(s)
+		if err != nil {
+			return nil, err
+		}
 		len, _ := net.Mask.Size()
 		switch family {
 		case RF_IPv4_UC, RF_IPv4_MC:
-			return NewIPAddrPrefix(uint8(len), addr.String())
+			return NewIPAddrPrefix(uint8(len), addr.String()), nil
 		}
-		return NewIPv6AddrPrefix(uint8(len), addr.String())
+		return NewIPv6AddrPrefix(uint8(len), addr.String()), nil
 	}
 
 	switch family {
 	case RF_IPv4_UC, RF_IPv4_MC:
 		if len(prefixStr) > 0 {
-			prefix = f(prefixStr[0])
+			prefix, err = f(prefixStr[0])
 		} else {
 			prefix = NewIPAddrPrefix(0, "")
 		}
 	case RF_IPv6_UC, RF_IPv6_MC:
 		if len(prefixStr) > 0 {
-			prefix = f(prefixStr[0])
+			prefix, err = f(prefixStr[0])
 		} else {
 			prefix = NewIPv6AddrPrefix(0, "")
 		}

diff --git a/pkg/packet/bgp/bgp_test.go b/pkg/packet/bgp/bgp_test.go
@@ -88,6 +88,33 @@ func Test_IPAddrPrefixString(t *testing.T) {
 	assert.Equal(t, "3343:faba:3903:128::/63", ipv6.String())
 }
 
+func Test_MalformedPrefixLookup(t *testing.T) {
+	assert := assert.New(t)
+
+	var tests = []struct {
+		inPrefix    string
+		routeFamily RouteFamily
+		want        AddrPrefixInterface
+		err         bool
+	}{
+		{"129.6.128/22", RF_IPv4_UC, nil, true},
+		{"foo", RF_IPv4_UC, nil, true},
+		{"3343:faba:3903:128::::/63", RF_IPv6_UC, nil, true},
+		{"foo", RF_IPv6_UC, nil, true},
+	}
+
+	for _, test := range tests {
+		afi, safi := RouteFamilyToAfiSafi(RF_IPv4_UC)
+		p, err := NewPrefixFromRouteFamily(afi, safi, test.inPrefix)
+		if test.err {
+			assert.Error(err)
+		} else {
+			assert.Equal(test.want, p)
+		}
+	}
+
+}
+
 func Test_IPAddrDecode(t *testing.T) {
 	r := IPAddrPrefixDefault{}
 	b := make([]byte, 16)