From 0cc3fac6c4b148918bcd34322d32afd737496b50 Mon Sep 17 00:00:00 2001 From: Freddy Date: Mon, 14 Nov 2022 12:35:20 -0700 Subject: [PATCH] Ensure that NodeDump imported nodes are filtered (#15356) --- .changelog/15356.txt | 3 + agent/structs/aclfilter/filter.go | 7 +- agent/structs/aclfilter/filter_test.go | 278 +++++++++++++++++++++---- agent/structs/structs_oss.go | 4 +- 4 files changed, 244 insertions(+), 48 deletions(-) create mode 100644 .changelog/15356.txt diff --git a/.changelog/15356.txt b/.changelog/15356.txt new file mode 100644 index 000000000..3fd854e0b --- /dev/null +++ b/.changelog/15356.txt @@ -0,0 +1,3 @@ +```release-note:security +Ensure that data imported from peers is filtered by ACLs at the UI Nodes/Services endpoints [CVE-2022-3920](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-3920) +``` \ No newline at end of file diff --git a/agent/structs/aclfilter/filter.go b/agent/structs/aclfilter/filter.go index b976627dc..a6c5301d1 100644 --- a/agent/structs/aclfilter/filter.go +++ b/agent/structs/aclfilter/filter.go @@ -61,7 +61,12 @@ func (f *Filter) Filter(subject any) { v.QueryMeta.ResultsFilteredByACLs = f.filterIntentions(&v.Intentions) case *structs.IndexedNodeDump: - v.QueryMeta.ResultsFilteredByACLs = f.filterNodeDump(&v.Dump) + if f.filterNodeDump(&v.Dump) { + v.QueryMeta.ResultsFilteredByACLs = true + } + if f.filterNodeDump(&v.ImportedDump) { + v.QueryMeta.ResultsFilteredByACLs = true + } case *structs.IndexedServiceDump: v.QueryMeta.ResultsFilteredByACLs = f.filterServiceDump(&v.Dump) diff --git a/agent/structs/aclfilter/filter_test.go b/agent/structs/aclfilter/filter_test.go index fb54906af..4afb00bb5 100644 --- a/agent/structs/aclfilter/filter_test.go +++ b/agent/structs/aclfilter/filter_test.go @@ -1444,12 +1444,105 @@ func TestACL_filterNodeDump(t *testing.T) { }, }, }, + ImportedDump: structs.NodeDump{ + { + // The node and service names are intentionally the same to ensure that + // local permissions for the same names do not allow reading imports. + Node: "node1", + PeerName: "cluster-02", + Services: []*structs.NodeService{ + { + ID: "foo", + Service: "foo", + PeerName: "cluster-02", + }, + }, + Checks: []*structs.HealthCheck{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + PeerName: "cluster-02", + }, + }, + }, + }, } } + type testCase struct { + authzFn func() acl.Authorizer + expect *structs.IndexedNodeDump + } - t.Run("allowed", func(t *testing.T) { + run := func(t *testing.T, tc testCase) { + authz := tc.authzFn() - policy, err := acl.NewPolicyFromSource(` + list := makeList() + New(authz, logger).Filter(list) + + require.Equal(t, tc.expect, list) + } + + tt := map[string]testCase{ + "denied": { + authzFn: func() acl.Authorizer { + return acl.DenyAll() + }, + expect: &structs.IndexedNodeDump{ + Dump: structs.NodeDump{}, + ImportedDump: structs.NodeDump{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "can read local service but not the node": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` + service "foo" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(t, err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) + + return authz + }, + expect: &structs.IndexedNodeDump{ + Dump: structs.NodeDump{}, + ImportedDump: structs.NodeDump{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "can read the local node but not the service": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` + node "node1" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(t, err) + + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) + + return authz + }, + expect: &structs.IndexedNodeDump{ + Dump: structs.NodeDump{ + { + Node: "node1", + Services: []*structs.NodeService{}, + Checks: structs.HealthChecks{}, + }, + }, + ImportedDump: structs.NodeDump{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "can read local data": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` service "foo" { policy = "read" } @@ -1457,65 +1550,158 @@ func TestACL_filterNodeDump(t *testing.T) { policy = "read" } `, acl.SyntaxLegacy, nil, nil) - require.NoError(t, err) + require.NoError(t, err) - authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - require.NoError(t, err) + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) - list := makeList() - New(authz, logger).Filter(list) - - require.Len(t, list.Dump, 1) - require.False(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be false") - }) - - t.Run("allowed to read the service, but not the node", func(t *testing.T) { - - policy, err := acl.NewPolicyFromSource(` - service "foo" { + return authz + }, + expect: &structs.IndexedNodeDump{ + Dump: structs.NodeDump{ + { + Node: "node1", + Services: []*structs.NodeService{ + { + ID: "foo", + Service: "foo", + }, + }, + Checks: []*structs.HealthCheck{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, + }, + }, + }, + ImportedDump: structs.NodeDump{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "can read imported service but not the node": { + authzFn: func() acl.Authorizer { + // Wildcard service read also grants read to imported services. + policy, err := acl.NewPolicyFromSource(` + service "" { policy = "read" } `, acl.SyntaxLegacy, nil, nil) - require.NoError(t, err) + require.NoError(t, err) - authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - require.NoError(t, err) + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) - list := makeList() - New(authz, logger).Filter(list) - - require.Empty(t, list.Dump) - require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") - }) - - t.Run("allowed to read the node, but not the service", func(t *testing.T) { - - policy, err := acl.NewPolicyFromSource(` - node "node1" { + return authz + }, + expect: &structs.IndexedNodeDump{ + Dump: structs.NodeDump{}, + ImportedDump: structs.NodeDump{}, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "can read the imported node but not the service": { + authzFn: func() acl.Authorizer { + // Wildcard node read also grants read to imported nodes. + policy, err := acl.NewPolicyFromSource(` + node "" { policy = "read" } `, acl.SyntaxLegacy, nil, nil) - require.NoError(t, err) + require.NoError(t, err) - authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) - require.NoError(t, err) + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) - list := makeList() - New(authz, logger).Filter(list) + return authz + }, + expect: &structs.IndexedNodeDump{ + Dump: structs.NodeDump{ + { + Node: "node1", + Services: []*structs.NodeService{}, + Checks: structs.HealthChecks{}, + }, + }, + ImportedDump: structs.NodeDump{ + { + Node: "node1", + PeerName: "cluster-02", + Services: []*structs.NodeService{}, + Checks: structs.HealthChecks{}, + }, + }, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: true}, + }, + }, + "can read all data": { + authzFn: func() acl.Authorizer { + policy, err := acl.NewPolicyFromSource(` + service "" { + policy = "read" + } + node "" { + policy = "read" + } + `, acl.SyntaxLegacy, nil, nil) + require.NoError(t, err) - require.Len(t, list.Dump, 1) - require.Empty(t, list.Dump[0].Services) - require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") - }) + authz, err := acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil) + require.NoError(t, err) - t.Run("denied", func(t *testing.T) { + return authz + }, + expect: &structs.IndexedNodeDump{ + Dump: structs.NodeDump{ + { + Node: "node1", + Services: []*structs.NodeService{ + { + ID: "foo", + Service: "foo", + }, + }, + Checks: []*structs.HealthCheck{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + }, + }, + }, + }, + ImportedDump: structs.NodeDump{ + { + Node: "node1", + PeerName: "cluster-02", + Services: []*structs.NodeService{ + { + ID: "foo", + Service: "foo", + PeerName: "cluster-02", + }, + }, + Checks: []*structs.HealthCheck{ + { + Node: "node1", + CheckID: "check1", + ServiceName: "foo", + PeerName: "cluster-02", + }, + }, + }, + }, + QueryMeta: structs.QueryMeta{ResultsFilteredByACLs: false}, + }, + }, + } - list := makeList() - New(acl.DenyAll(), logger).Filter(list) - - require.Empty(t, list.Dump) - require.True(t, list.QueryMeta.ResultsFilteredByACLs, "ResultsFilteredByACLs should be true") - }) + for name, tc := range tt { + t.Run(name, func(t *testing.T) { + run(t, tc) + }) + } } func TestACL_filterNodes(t *testing.T) { diff --git a/agent/structs/structs_oss.go b/agent/structs/structs_oss.go index e687e7620..68d6f7ed4 100644 --- a/agent/structs/structs_oss.go +++ b/agent/structs/structs_oss.go @@ -67,7 +67,9 @@ func (n *Node) OverridePartition(_ string) { func (_ *Coordinate) FillAuthzContext(_ *acl.AuthorizerContext) {} -func (_ *NodeInfo) FillAuthzContext(_ *acl.AuthorizerContext) {} +func (n *NodeInfo) FillAuthzContext(ctx *acl.AuthorizerContext) { + ctx.Peer = n.PeerName +} // FillAuthzContext stub func (_ *DirEntry) FillAuthzContext(_ *acl.AuthorizerContext) {}