Skip to content

Commit

Permalink
search: fix ACL filtering for plugins and variables
Browse files Browse the repository at this point in the history
ACL permissions for the search endpoints are done in three passes. The
first (the `sufficientSearchPerms` method) is for performance and coarsely
rejects requests based on the passed-in context parameter if the user has no
permissions to any object in that context. The second (the
`filteredSearchContexts` method) filters out contexts based on whether the user
has permissions either to the requested namespace or again by context (to catch
the "all" context). Finally, when iterating over the objects available, we do
the usual filtering in the iterator.

Internal testing found several bugs in this filtering:
* CSI plugins can be searched by any authenticated user.
* Variables can be searched if the user has `job:read` permissions to the
  variable's namespace instead of `variable:list`.
* Variables cannot be searched by wildcard namespace.

This is an information leak of the plugin names and variable paths, which we
don't consider to be privileged information but intended to protect anyways.

This changeset fixes these bugs by ensuring CSI plugins are filtered in the 1st
and 2nd pass ACL filters, and changes variables to check `variable:list` in the
2nd pass filter unless the wildcard namespace is passed (at which point we'll
fallback to filtering in the iterator).

Fixes: CVE-2023-3300
Fixes: #17906
  • Loading branch information
tgross authored and lgfa29 committed Jul 19, 2023
1 parent 54c45ed commit a8789d3
Show file tree
Hide file tree
Showing 5 changed files with 273 additions and 120 deletions.
3 changes: 3 additions & 0 deletions .changelog/17906.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:security
search: Fixed a bug where ACL did not filter plugin and variable names in search endpoint. [CVE-2023-3300](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-3300)
```
4 changes: 4 additions & 0 deletions acl/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,10 @@ func (a *ACL) AllowVariableSearch(ns string) bool {
if a.management {
return true
}
if ns == "*" {
return a.variables.Len() > 0 || a.wildcardVariables.Len() > 0
}

iter := a.variables.Root().Iterator()
iter.SeekPrefix([]byte(ns))
_, _, ok := iter.Next()
Expand Down
40 changes: 21 additions & 19 deletions nomad/search_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -659,48 +659,46 @@ func (s *Search) PrefixSearch(args *structs.SearchRequest, reply *structs.Search
return s.srv.blockingRPC(&opts)
}

// sufficientSearchPerms returns true if the provided ACL has access to any
// capabilities required for prefix searching.
// sufficientSearchPerms returns true if the provided ACL has access to *any*
// capabilities required for prefix searching. This is intended as a performance
// improvement so that we don't do expensive queries and then filter the results
// if the user will never get any results. The caller still needs to filter
// anything it gets from the state store.
//
// Returns true if aclObj is nil or is for a management token
func sufficientSearchPerms(aclObj *acl.ACL, namespace string, context structs.Context) bool {
if aclObj == nil || aclObj.IsManagement() {
return true
}

nodeRead := aclObj.AllowNodeRead()
allowNodePool := aclObj.AllowNodePoolSearch()
allowNS := aclObj.AllowNamespace(namespace)
jobRead := aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob)
allowEnt := sufficientSearchPermsEnt(aclObj)

if !nodeRead && !allowNodePool && !allowNS && !allowEnt && !jobRead {
return false
}

// Reject requests that explicitly specify a disallowed context. This
// should give the user better feedback than simply filtering out all
// results and returning an empty list.
switch context {
case structs.Nodes:
return nodeRead
return aclObj.AllowNodeRead()
case structs.NodePools:
// The search term alone is not enough to determine if the token is
// allowed to access the given prefix since it may not match node pool
// label in the policy. Node pools will be filtered when iterating over
// the results.
return allowNodePool
return aclObj.AllowNodePoolSearch()
case structs.Namespaces:
return allowNS
case structs.Allocs, structs.Deployments, structs.Evals, structs.Jobs:
return jobRead
return aclObj.AllowNamespace(namespace)
case structs.Allocs, structs.Deployments, structs.Evals, structs.Jobs,
structs.ScalingPolicies, structs.Recommendations:
return aclObj.AllowNsOp(namespace, acl.NamespaceCapabilityReadJob)
case structs.Volumes:
return acl.NamespaceValidator(acl.NamespaceCapabilityCSIListVolume,
acl.NamespaceCapabilityCSIReadVolume,
acl.NamespaceCapabilityListJobs,
acl.NamespaceCapabilityReadJob)(aclObj, namespace)
case structs.Variables:
return aclObj.AllowVariableSearch(namespace)
case structs.Plugins:
return aclObj.AllowPluginList()
case structs.Quotas:
return aclObj.AllowQuotaRead()
}

return true
Expand All @@ -716,7 +714,7 @@ func sufficientSearchPerms(aclObj *acl.ACL, namespace string, context structs.Co
//
// These types are available for fuzzy searching:
//
// Nodes, Node Pools, Namespaces, Jobs, Allocs, Plugins
// Nodes, Node Pools, Namespaces, Jobs, Allocs, Plugins, Variables
//
// Jobs are a special case that expand into multiple types, and whose return
// values include Scope which is a descending list of IDs of parent objects,
Expand Down Expand Up @@ -927,7 +925,7 @@ func filteredSearchContexts(aclObj *acl.ACL, namespace string, context structs.C
available = append(available, c)
}
case structs.Variables:
if jobRead {
if aclObj.AllowVariableSearch(namespace) {
available = append(available, c)
}
case structs.Nodes:
Expand All @@ -942,6 +940,10 @@ func filteredSearchContexts(aclObj *acl.ACL, namespace string, context structs.C
if volRead {
available = append(available, c)
}
case structs.Plugins:
if aclObj.AllowPluginList() {
available = append(available, c)
}
default:
if ok := filteredSearchContextsEnt(aclObj, namespace, c); ok {
available = append(available, c)
Expand Down
4 changes: 0 additions & 4 deletions nomad/search_endpoint_oss.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@ func getEnterpriseFuzzyResourceIter(context structs.Context, _ *acl.ACL, _ strin
return nil, fmt.Errorf("context must be one of %v or 'all' for all contexts; got %q", allContexts, context)
}

func sufficientSearchPermsEnt(aclObj *acl.ACL) bool {
return true
}

func filteredSearchContextsEnt(aclObj *acl.ACL, namespace string, context structs.Context) bool {
return true
}
Loading

0 comments on commit a8789d3

Please sign in to comment.