Skip to content

Commit 4fdb83e

Browse files
authored
xds/rbac: avoid nil deref on unset CidrRange prefix_len (#9250)
`newRemoteIPMatcher` and `newLocalIPMatcher` build the CIDR string from `cidrRange.PrefixLen.Value`, dereferencing the `PrefixLen` wrapper field directly. An RBAC policy from the control plane whose `destination_ip`/`direct_remote_ip`/`source_ip`/`remote_ip` CidrRange sets `address_prefix` but leaves `prefix_len` unset makes `PrefixLen == nil`, so `NewChainEngine` panics while the resource is parsed. Nothing recovers on the xdsclient decode path, so the process goes down. RELEASE NOTES: - xds/rbac: Fix a potential panic when parsing a CIDR range that does not contain a prefix length.
1 parent 89d4d61 commit 4fdb83e

2 files changed

Lines changed: 38 additions & 2 deletions

File tree

internal/xds/rbac/matchers.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ type remoteIPMatcher struct {
365365
func newRemoteIPMatcher(cidrRange *v3corepb.CidrRange) (*remoteIPMatcher, error) {
366366
// Convert configuration to a cidrRangeString, as Go standard library has
367367
// methods that parse cidr string.
368-
cidrRangeString := fmt.Sprintf("%s/%d", cidrRange.AddressPrefix, cidrRange.PrefixLen.Value)
368+
cidrRangeString := fmt.Sprintf("%s/%d", cidrRange.GetAddressPrefix(), cidrRange.GetPrefixLen().GetValue())
369369
ipNet, err := netip.ParsePrefix(cidrRangeString)
370370
if err != nil {
371371
return nil, err
@@ -391,7 +391,7 @@ type localIPMatcher struct {
391391
}
392392

393393
func newLocalIPMatcher(cidrRange *v3corepb.CidrRange) (*localIPMatcher, error) {
394-
cidrRangeString := fmt.Sprintf("%s/%d", cidrRange.AddressPrefix, cidrRange.PrefixLen.Value)
394+
cidrRangeString := fmt.Sprintf("%s/%d", cidrRange.GetAddressPrefix(), cidrRange.GetPrefixLen().GetValue())
395395
ipNet, err := netip.ParsePrefix(cidrRangeString)
396396
if err != nil {
397397
return nil, err

internal/xds/rbac/rbac_engine_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,42 @@ func (s) TestNewChainEngine(t *testing.T) {
300300
},
301301
wantErr: true,
302302
},
303+
{
304+
name: "RemoteIpMatcherUnsetPrefixLen",
305+
policies: []*v3rbacpb.RBAC{
306+
{
307+
Action: v3rbacpb.RBAC_ALLOW,
308+
Policies: map[string]*v3rbacpb.Policy{
309+
"certain-source-ip": {
310+
Permissions: []*v3rbacpb.Permission{
311+
{Rule: &v3rbacpb.Permission_Any{Any: true}},
312+
},
313+
Principals: []*v3rbacpb.Principal{
314+
{Identifier: &v3rbacpb.Principal_DirectRemoteIp{DirectRemoteIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0"}}},
315+
},
316+
},
317+
},
318+
},
319+
},
320+
},
321+
{
322+
name: "DestinationIpMatcherUnsetPrefixLen",
323+
policies: []*v3rbacpb.RBAC{
324+
{
325+
Action: v3rbacpb.RBAC_ALLOW,
326+
Policies: map[string]*v3rbacpb.Policy{
327+
"certain-destination-ip": {
328+
Permissions: []*v3rbacpb.Permission{
329+
{Rule: &v3rbacpb.Permission_DestinationIp{DestinationIp: &v3corepb.CidrRange{AddressPrefix: "0.0.0.0"}}},
330+
},
331+
Principals: []*v3rbacpb.Principal{
332+
{Identifier: &v3rbacpb.Principal_Any{Any: true}},
333+
},
334+
},
335+
},
336+
},
337+
},
338+
},
303339
{
304340
name: "MatcherToNotPolicy",
305341
policies: []*v3rbacpb.RBAC{

0 commit comments

Comments
 (0)