From 82b7156952f50fbfbd993a5171b05859fdd7bc6e Mon Sep 17 00:00:00 2001 From: Roberto Dip Date: Thu, 14 Dec 2023 12:38:54 -0300 Subject: [PATCH] ensure distributed queries can't be empty strings (#15641) There's a bug somewhere (Fleet, osquery or both?) that causes hosts to check-in in a loop if you send an empty query string. we previously fixed this for detail query overrides (see #14286, #14296) but I'm also adding a general check as a safeguard for issues like #15524 --- server/service/integration_mdm_test.go | 39 +++++++++++++++++++++++++ server/service/osquery.go | 14 ++++++++- server/service/osquery_test.go | 40 ++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/server/service/integration_mdm_test.go b/server/service/integration_mdm_test.go index 2bc1402fa9..25602ffd58 100644 --- a/server/service/integration_mdm_test.go +++ b/server/service/integration_mdm_test.go @@ -35,6 +35,7 @@ import ( "github.com/fleetdm/fleet/v4/server/datastore/redis/redistest" "github.com/fleetdm/fleet/v4/server/fleet" mdm_types "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/live_query/live_query_mock" servermdm "github.com/fleetdm/fleet/v4/server/mdm" apple_mdm "github.com/fleetdm/fleet/v4/server/mdm/apple" "github.com/fleetdm/fleet/v4/server/mdm/apple/mobileconfig" @@ -122,6 +123,7 @@ func (s *integrationMDMTestSuite) SetupSuite() { ) mdmCommander := apple_mdm.NewMDMAppleCommander(mdmStorage, mdmPushService) redisPool := redistest.SetupRedis(s.T(), "zz", false, false, false) + s.withServer.lq = live_query_mock.New(s.T()) var depSchedule *schedule.Schedule var profileSchedule *schedule.Schedule @@ -135,6 +137,7 @@ func (s *integrationMDMTestSuite) SetupSuite() { SCEPStorage: scepStorage, MDMPusher: mdmPushService, Pool: redisPool, + Lq: s.lq, StartCronSchedules: []TestNewScheduleFunc{ func(ctx context.Context, ds fleet.Datastore) fleet.NewCronScheduleFunc { return func() (fleet.CronSchedule, error) { @@ -10275,3 +10278,39 @@ func (s *integrationMDMTestSuite) TestBatchSetMDMProfiles() { 0, ) } + +func (s *integrationMDMTestSuite) TestWindowsFreshEnrollEmptyQuery() { + t := s.T() + host, _ := createWindowsHostThenEnrollMDM(s.ds, s.server.URL, t) + + // make sure we don't have any profiles + s.Do( + "POST", + "/api/v1/fleet/mdm/profiles/batch", + batchSetMDMProfilesRequest{Profiles: map[string][]byte{}}, + http.StatusNoContent, + ) + + // Ensure we can read distributed queries for the host. + err := s.ds.UpdateHostRefetchRequested(context.Background(), host.ID, true) + require.NoError(t, err) + + s.lq.On("QueriesForHost", host.ID).Return(map[string]string{fmt.Sprintf("%d", host.ID): "SELECT 1 FROM osquery;"}, nil) + + req := getDistributedQueriesRequest{NodeKey: *host.NodeKey} + var dqResp getDistributedQueriesResponse + s.DoJSON("POST", "/api/osquery/distributed/read", req, http.StatusOK, &dqResp) + require.NotContains(t, dqResp.Queries, "fleet_detail_query_mdm_config_profiles_windows") + + // add two profiles + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: map[string][]byte{ + "N1": mobileconfigForTest("N1", "I1"), + "N2": syncMLForTest("./Foo/Bar"), + }}, http.StatusNoContent) + + req = getDistributedQueriesRequest{NodeKey: *host.NodeKey} + dqResp = getDistributedQueriesResponse{} + s.DoJSON("POST", "/api/osquery/distributed/read", req, http.StatusOK, &dqResp) + require.Contains(t, dqResp.Queries, "fleet_detail_query_mdm_config_profiles_windows") + require.NotEmpty(t, dqResp.Queries, "fleet_detail_query_mdm_config_profiles_windows") +} diff --git a/server/service/osquery.go b/server/service/osquery.go index b0a3e869dc..97bdfb0ac9 100644 --- a/server/service/osquery.go +++ b/server/service/osquery.go @@ -654,7 +654,19 @@ func (svc *Service) GetDistributedQueries(ctx context.Context) (queries map[stri // // Thus, we set the alwaysTrueQuery for all queries, except for those where we set // an explicit discovery query (e.g. orbit_info, google_chrome_profiles). - for name := range queries { + for name, query := range queries { + // there's a bug somewhere (Fleet, osquery or both?) + // that causes hosts to check-in in a loop if you send + // an empty query string. + // + // we previously fixed this for detail query overrides (see + // #14286, #14296) but I'm also adding this here as a safeguard + // for issues like #15524 + if query == "" { + delete(queries, name) + delete(discovery, name) + continue + } discoveryQuery := discovery[name] if discoveryQuery == "" { discoveryQuery = alwaysTrueQuery diff --git a/server/service/osquery_test.go b/server/service/osquery_test.go index 53fcf15e06..3e029bf890 100644 --- a/server/service/osquery_test.go +++ b/server/service/osquery_test.go @@ -1164,6 +1164,46 @@ func TestGetDistributedQueriesMissingHost(t *testing.T) { assert.Contains(t, err.Error(), "missing host") } +func TestGetDistributedQueriesEmptyQuery(t *testing.T) { + mockClock := clock.NewMockClock() + ds := new(mock.Store) + lq := live_query_mock.New(t) + svc, ctx := newTestServiceWithClock(t, ds, nil, lq, mockClock) + + host := &fleet.Host{ + Platform: "darwin", + } + + ds.LabelQueriesForHostFunc = func(ctx context.Context, host *fleet.Host) (map[string]string, error) { + return map[string]string{"empty_label_query": ""}, nil + } + ds.HostLiteFunc = func(ctx context.Context, id uint) (*fleet.Host, error) { + return host, nil + } + ds.UpdateHostFunc = func(ctx context.Context, gotHost *fleet.Host) error { + return nil + } + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &fleet.AppConfig{Features: fleet.Features{EnableHostUsers: true}}, nil + } + ds.PolicyQueriesForHostFunc = func(ctx context.Context, host *fleet.Host) (map[string]string, error) { + return map[string]string{"empty_policy_query": ""}, nil + } + + lq.On("QueriesForHost", uint(0)).Return(map[string]string{"empty_live_query": ""}, nil) + + ctx = hostctx.NewContext(ctx, host) + queries, discovery, _, err := svc.GetDistributedQueries(ctx) + require.NoError(t, err) + require.NotEmpty(t, queries) + for n, q := range queries { + require.NotEmpty(t, q, n) + } + for n, q := range discovery { + require.NotEmpty(t, q, n) + } +} + func TestLabelQueries(t *testing.T) { mockClock := clock.NewMockClock() ds := new(mock.Store)