From 76023bd2ad0b5bfb5b3c625a03dd0ec6000bd988 Mon Sep 17 00:00:00 2001 From: Zach Koopmans Date: Thu, 28 Apr 2022 22:58:27 -0700 Subject: [PATCH] Don't send security events for clone/exit tasks that are threads. Two security messages points are PointCloneProcess and PointExitNotifyParent (see pkg/sentry/seccheck/seccheck.go). Both of these should only trigger when we have a process starting or exiting respectively. Becuase of this, only send a start message if "clone()" is called without CLONE_THREAD set, and only send an exit message if the thread group (read: process) is exiting. PiperOrigin-RevId: 445334328 --- pkg/sentry/kernel/task_clone.go | 4 ++-- pkg/sentry/kernel/task_exit.go | 8 +++++++- pkg/sentry/seccheck/metadata.go | 2 +- pkg/sentry/seccheck/seccheck.go | 2 +- pkg/sentry/seccheck/seccheck_test.go | 22 +++++++++++----------- 5 files changed, 22 insertions(+), 16 deletions(-) diff --git a/pkg/sentry/kernel/task_clone.go b/pkg/sentry/kernel/task_clone.go index fbf8a26bd..a5ddc2a17 100644 --- a/pkg/sentry/kernel/task_clone.go +++ b/pkg/sentry/kernel/task_clone.go @@ -246,7 +246,7 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) { tid := nt.k.tasks.Root.IDOfTask(nt) defer nt.Start(tid) - if seccheck.Global.Enabled(seccheck.PointClone) { + if args.Flags&linux.CLONE_THREAD == 0 && seccheck.Global.Enabled(seccheck.PointCloneProcess) { mask, info := getCloneSeccheckInfo(t, nt) if err := seccheck.Global.SendToCheckers(func(c seccheck.Checker) error { return c.Clone(t, mask, info) @@ -309,7 +309,7 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) { } func getCloneSeccheckInfo(t, nt *Task) (seccheck.FieldSet, *pb.CloneInfo) { - fields := seccheck.Global.GetFieldSet(seccheck.PointClone) + fields := seccheck.Global.GetFieldSet(seccheck.PointCloneProcess) t.k.tasks.mu.RLock() defer t.k.tasks.mu.RUnlock() diff --git a/pkg/sentry/kernel/task_exit.go b/pkg/sentry/kernel/task_exit.go index 3c37e01d9..5ce711051 100644 --- a/pkg/sentry/kernel/task_exit.go +++ b/pkg/sentry/kernel/task_exit.go @@ -661,7 +661,13 @@ func (t *Task) exitNotifyLocked(fromPtraceDetach bool) { // should return ECHILD). t.parent.tg.eventQueue.Notify(EventExit | EventChildGroupStop | EventGroupContinue) } - if seccheck.Global.Enabled(seccheck.PointExitNotifyParent) { + + // We don't send exit events for threads because we don't send CloneProcessStart events + // for threads (clone calls with CLONE_THREAD set). + // We also don't send exit events for the root process because we don't send + // Clone or Exec events for the initial process. + shouldSendExit := t == t.tg.leader && t.tg != t.k.globalInit + if seccheck.Global.Enabled(seccheck.PointExitNotifyParent) && shouldSendExit { mask, info := getExitNotifyParentSeccheckInfo(t) if err := seccheck.Global.SendToCheckers(func(c seccheck.Checker) error { return c.ExitNotifyParent(t, mask, info) diff --git a/pkg/sentry/seccheck/metadata.go b/pkg/sentry/seccheck/metadata.go index d4a0ba287..7309094b3 100644 --- a/pkg/sentry/seccheck/metadata.go +++ b/pkg/sentry/seccheck/metadata.go @@ -172,7 +172,7 @@ func init() { // Points from the sentry namespace. registerPoint(PointDesc{ - ID: PointClone, + ID: PointCloneProcess, Name: "sentry/clone", ContextFields: defaultContextFields, }) diff --git a/pkg/sentry/seccheck/seccheck.go b/pkg/sentry/seccheck/seccheck.go index e73807576..986aee34e 100644 --- a/pkg/sentry/seccheck/seccheck.go +++ b/pkg/sentry/seccheck/seccheck.go @@ -28,7 +28,7 @@ type Point uint // PointX represents the checkpoint X. const ( - PointClone Point = iota + PointCloneProcess Point = iota PointExecve PointExitNotifyParent PointContainerStart diff --git a/pkg/sentry/seccheck/seccheck_test.go b/pkg/sentry/seccheck/seccheck_test.go index 109fbfb2e..9d80ecb3a 100644 --- a/pkg/sentry/seccheck/seccheck_test.go +++ b/pkg/sentry/seccheck/seccheck_test.go @@ -38,7 +38,7 @@ func (c *testChecker) Clone(ctx context.Context, fields FieldSet, info *pb.Clone func TestNoChecker(t *testing.T) { var s State - if s.Enabled(PointClone) { + if s.Enabled(PointCloneProcess) { t.Errorf("Enabled(PointClone): got true, wanted false") } } @@ -46,7 +46,7 @@ func TestNoChecker(t *testing.T) { func TestCheckerNotRegisteredForPoint(t *testing.T) { var s State s.AppendChecker(&testChecker{}, nil) - if s.Enabled(PointClone) { + if s.Enabled(PointCloneProcess) { t.Errorf("Enabled(PointClone): got true, wanted false") } } @@ -62,16 +62,16 @@ func TestCheckerRegistered(t *testing.T) { } req := []PointReq{ { - Pt: PointClone, + Pt: PointCloneProcess, Fields: FieldSet{Context: MakeFieldMask(FieldCtxtCredentials)}, }, } s.AppendChecker(checker, req) - if !s.Enabled(PointClone) { + if !s.Enabled(PointCloneProcess) { t.Errorf("Enabled(PointClone): got false, wanted true") } - fields := s.GetFieldSet(PointClone) + fields := s.GetFieldSet(PointCloneProcess) if !fields.Context.Contains(FieldCtxtCredentials) { t.Errorf("fields.Context.Contains(PointContextCredentials): got false, wanted true") } @@ -95,7 +95,7 @@ func TestMultipleCheckersRegistered(t *testing.T) { }, } reqs := []PointReq{ - {Pt: PointClone}, + {Pt: PointCloneProcess}, } s.AppendChecker(checker, reqs) @@ -104,16 +104,16 @@ func TestMultipleCheckersRegistered(t *testing.T) { return nil }} reqs = []PointReq{ - {Pt: PointClone}, + {Pt: PointCloneProcess}, } s.AppendChecker(checker, reqs) - if !s.Enabled(PointClone) { + if !s.Enabled(PointCloneProcess) { t.Errorf("Enabled(PointClone): got false, wanted true") } // CloneReq() should return the union of requested fields from all calls to // AppendChecker. - fields := s.GetFieldSet(PointClone) + fields := s.GetFieldSet(PointCloneProcess) if err := s.SendToCheckers(func(c Checker) error { return c.Clone(context.Background(), fields, &pb.CloneInfo{}) }); err != nil { @@ -139,7 +139,7 @@ func TestCheckpointReturnsFirstCheckerError(t *testing.T) { }, } reqs := []PointReq{ - {Pt: PointClone}, + {Pt: PointCloneProcess}, } s.AppendChecker(checker, reqs) @@ -152,7 +152,7 @@ func TestCheckpointReturnsFirstCheckerError(t *testing.T) { } s.AppendChecker(checker, reqs) - if !s.Enabled(PointClone) { + if !s.Enabled(PointCloneProcess) { t.Errorf("Enabled(PointClone): got false, wanted true") } if err := s.SendToCheckers(func(c Checker) error {