NET-11737 - sec vulnerability - remediate ability to use bexpr to fil…

…ter results without ACL read on endpoint (#21950)

* NET-11737 - sec vulnerability - remediate ability to use bexpr to filter results without ACL read on endpoint

* add changelog

* update test descriptions to make more sense

.changelog/21950.txt

@@ -0,0 +1,3 @@
+```release-note:security
+Removed ability to use bexpr to filter results without ACL read on endpoint
+```

agent/agent_endpoint.go

@@ -380,16 +380,14 @@ func (s *HTTPHandlers) AgentServices(resp http.ResponseWriter, req *http.Request
 		return nil, err
 	}
 
-	raw, err := filter.Execute(agentSvcs)
-	if err != nil {
-		return nil, err
-	}
-	agentSvcs = raw.(map[string]*api.AgentService)
-
-	// Note: we filter the results with ACLs *after* applying the user-supplied
-	// bexpr filter, to ensure total (and the filter-by-acls header we set below)
-	// do not include results that would be filtered out even if the user did have
-	// permission.
+	// Note: we filter the results with ACLs *before* applying the user-supplied
+	// bexpr filter to ensure that the user can only run expressions on data that
+	// they have access to.  This is a security measure to prevent users from
+	// running arbitrary expressions on data they don't have access to.
+	// QueryMeta.ResultsFilteredByACLs being true already indicates to the user
+	// that results they don't have access to have been removed.  If they were
+	// also allowed to run the bexpr filter on the data, they could potentially
+	// infer the specific attributes of data they don't have access to.
 	total := len(agentSvcs)
 	if err := s.agent.filterServicesWithAuthorizer(authz, agentSvcs); err != nil {
 		return nil, err
@@ -407,6 +405,12 @@ func (s *HTTPHandlers) AgentServices(resp http.ResponseWriter, req *http.Request
 		setResultsFilteredByACLs(resp, total != len(agentSvcs))
 	}
 
+	raw, err := filter.Execute(agentSvcs)
+	if err != nil {
+		return nil, err
+	}
+	agentSvcs = raw.(map[string]*api.AgentService)
+
 	return agentSvcs, nil
 }
 
@@ -540,16 +544,14 @@ func (s *HTTPHandlers) AgentChecks(resp http.ResponseWriter, req *http.Request)
 		}
 	}
 
-	raw, err := filter.Execute(agentChecks)
-	if err != nil {
-		return nil, err
-	}
-	agentChecks = raw.(map[types.CheckID]*structs.HealthCheck)
-
-	// Note: we filter the results with ACLs *after* applying the user-supplied
-	// bexpr filter, to ensure total (and the filter-by-acls header we set below)
-	// do not include results that would be filtered out even if the user did have
-	// permission.
+	// Note: we filter the results with ACLs *before* applying the user-supplied
+	// bexpr filter to ensure that the user can only run expressions on data that
+	// they have access to.  This is a security measure to prevent users from
+	// running arbitrary expressions on data they don't have access to.
+	// QueryMeta.ResultsFilteredByACLs being true already indicates to the user
+	// that results they don't have access to have been removed.  If they were
+	// also allowed to run the bexpr filter on the data, they could potentially
+	// infer the specific attributes of data they don't have access to.
 	total := len(agentChecks)
 	if err := s.agent.filterChecksWithAuthorizer(authz, agentChecks); err != nil {
 		return nil, err
@@ -567,6 +569,12 @@ func (s *HTTPHandlers) AgentChecks(resp http.ResponseWriter, req *http.Request)
 		setResultsFilteredByACLs(resp, total != len(agentChecks))
 	}
 
+	raw, err := filter.Execute(agentChecks)
+	if err != nil {
+		return nil, err
+	}
+	agentChecks = raw.(map[types.CheckID]*structs.HealthCheck)
+
 	return agentChecks, nil
 }
 
@@ -623,21 +631,14 @@ func (s *HTTPHandlers) AgentMembers(resp http.ResponseWriter, req *http.Request)
 		}
 	}
 
-	// filter the members by parsed filter expression
-	var filterExpression string
-	s.parseFilter(req, &filterExpression)
-	if filterExpression != "" {
-		filter, err := bexpr.CreateFilter(filterExpression, nil, members)
-		if err != nil {
-			return nil, err
-		}
-		raw, err := filter.Execute(members)
-		if err != nil {
-			return nil, err
-		}
-		members = raw.([]serf.Member)
-	}
-
+	// Note: we filter the results with ACLs *before* applying the user-supplied
+	// bexpr filter to ensure that the user can only run expressions on data that
+	// they have access to.  This is a security measure to prevent users from
+	// running arbitrary expressions on data they don't have access to.
+	// QueryMeta.ResultsFilteredByACLs being true already indicates to the user
+	// that results they don't have access to have been removed.  If they were
+	// also allowed to run the bexpr filter on the data, they could potentially
+	// infer the specific attributes of data they don't have access to.
 	total := len(members)
 	if err := s.agent.filterMembers(token, &members); err != nil {
 		return nil, err
@@ -655,6 +656,21 @@ func (s *HTTPHandlers) AgentMembers(resp http.ResponseWriter, req *http.Request)
 		setResultsFilteredByACLs(resp, total != len(members))
 	}
 
+	// filter the members by parsed filter expression
+	var filterExpression string
+	s.parseFilter(req, &filterExpression)
+	if filterExpression != "" {
+		filter, err := bexpr.CreateFilter(filterExpression, nil, members)
+		if err != nil {
+			return nil, err
+		}
+		raw, err := filter.Execute(members)
+		if err != nil {
+			return nil, err
+		}
+		members = raw.([]serf.Member)
+	}
+
 	return members, nil
 }
 

agent/agent_endpoint_test.go

@@ -433,6 +433,60 @@ func TestAgent_Services_ACLFilter(t *testing.T) {
 		require.Len(t, val, 2)
 		require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
 	})
+
+	// ensure ACL filtering occurs before bexpr filtering.
+	const bexprMatchingUserTokenPermissions = "Service matches `web.*`"
+	const bexprNotMatchingUserTokenPermissions = "Service matches `api.*`"
+
+	tokenWithWebRead := testCreateToken(t, a, `
+			service "web" {
+				policy = "read"
+			}
+		`)
+
+	t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/services?filter="+url.QueryEscape(bexprMatchingUserTokenPermissions), nil)
+		req.Header.Add("X-Consul-Token", tokenWithWebRead)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		var val map[string]*api.AgentService
+		err := dec.Decode(&val)
+		if err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 1)
+		require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
+
+	t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/services?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil)
+		req.Header.Add("X-Consul-Token", tokenWithWebRead)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		var val map[string]*api.AgentService
+		err := dec.Decode(&val)
+		if err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 0)
+		require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
+
+	t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/services?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		var val map[string]*api.AgentService
+		err := dec.Decode(&val)
+		if err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 0)
+		require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
 }
 
 func TestAgent_Service(t *testing.T) {
@@ -1432,6 +1486,57 @@ func TestAgent_Checks_ACLFilter(t *testing.T) {
 		require.Len(t, val, 2)
 		require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
 	})
+
+	// ensure ACL filtering occurs before bexpr filtering.
+	const bexprMatchingUserTokenPermissions = "ServiceName matches `web.*`"
+	const bexprNotMatchingUserTokenPermissions = "ServiceName matches `api.*`"
+
+	tokenWithWebRead := testCreateToken(t, a, `
+			service "web" {
+				policy = "read"
+			}
+		`)
+
+	t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/checks?filter="+url.QueryEscape(bexprMatchingUserTokenPermissions), nil)
+		req.Header.Add("X-Consul-Token", tokenWithWebRead)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		val := make(map[types.CheckID]*structs.HealthCheck)
+		if err := dec.Decode(&val); err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 1)
+		require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
+
+	t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/checks?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil)
+		req.Header.Add("X-Consul-Token", tokenWithWebRead)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		val := make(map[types.CheckID]*structs.HealthCheck)
+		if err := dec.Decode(&val); err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 0)
+		require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
+
+	t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/checks?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		val := make(map[types.CheckID]*structs.HealthCheck)
+		if err := dec.Decode(&val); err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 0)
+		require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
 }
 
 func TestAgent_Self(t *testing.T) {
@@ -2110,6 +2215,57 @@ func TestAgent_Members_ACLFilter(t *testing.T) {
 		require.Len(t, val, 2)
 		require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
 	})
+
+	// ensure ACL filtering occurs before bexpr filtering.
+	bexprMatchingUserTokenPermissions := fmt.Sprintf("Name matches `%s.*`", b.Config.NodeName)
+	bexprNotMatchingUserTokenPermissions := fmt.Sprintf("Name matches `%s.*`", a.Config.NodeName)
+
+	tokenWithReadOnMemberB := testCreateToken(t, a, fmt.Sprintf(`
+			node "%s" {
+				policy = "read"
+			}
+		`, b.Config.NodeName))
+
+	t.Run("request with filter that matches token permissions returns 1 result and ResultsFilteredByACLs equal to true", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/members?filter="+url.QueryEscape(bexprMatchingUserTokenPermissions), nil)
+		req.Header.Add("X-Consul-Token", tokenWithReadOnMemberB)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		val := make([]serf.Member, 0)
+		if err := dec.Decode(&val); err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 1)
+		require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
+
+	t.Run("request with filter that does not match token permissions returns 0 results and ResultsFilteredByACLs equal to true", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/members?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil)
+		req.Header.Add("X-Consul-Token", tokenWithReadOnMemberB)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		val := make([]serf.Member, 0)
+		if err := dec.Decode(&val); err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 0)
+		require.NotEmpty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
+
+	t.Run("request with filter that would normally match but without any token returns zero results and ResultsFilteredByACLs equal to false", func(t *testing.T) {
+		req, _ := http.NewRequest("GET", "/v1/agent/members?filter="+url.QueryEscape(bexprNotMatchingUserTokenPermissions), nil)
+		resp := httptest.NewRecorder()
+		a.srv.h.ServeHTTP(resp, req)
+		dec := json.NewDecoder(resp.Body)
+		val := make([]serf.Member, 0)
+		if err := dec.Decode(&val); err != nil {
+			t.Fatalf("Err: %v", err)
+		}
+		require.Len(t, val, 0)
+		require.Empty(t, resp.Header().Get("X-Consul-Results-Filtered-By-ACLs"))
+	})
 }
 
 func TestAgent_Join(t *testing.T) {

agent/catalog_endpoint_test.go

@@ -122,7 +122,7 @@ func TestCatalogDeregister(t *testing.T) {
 	a := NewTestAgent(t, "")
 	defer a.Shutdown()
 
-	// Register node
+	// Deregister node
 	args := &structs.DeregisterRequest{Node: "foo"}
 	req, _ := http.NewRequest("PUT", "/v1/catalog/deregister", jsonReader(args))
 	obj, err := a.srv.CatalogDeregister(nil, req)