From cc1ac562a83bef33c702d635ec1714a3fcd75274 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Tue, 17 Dec 2024 23:26:30 +0530 Subject: [PATCH 01/12] chore: ignore lsass --- metric/system/process/process_windows.go | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/metric/system/process/process_windows.go b/metric/system/process/process_windows.go index 01ea65012..238d73bce 100644 --- a/metric/system/process/process_windows.go +++ b/metric/system/process/process_windows.go @@ -199,6 +199,9 @@ func FillMetricsRequiringMoreAccess(pid int, state ProcState) (ProcState, error) } func getProcArgs(pid int) ([]string, error) { + if ok := shouldIgnore(pid); ok { + return []string{}, nil + } handle, err := syscall.OpenProcess( windows.PROCESS_QUERY_LIMITED_INFORMATION| windows.PROCESS_VM_READ, @@ -463,3 +466,25 @@ func fillIdleProcess(state ProcState) (ProcState, error) { state.CPU.Total.Value = opt.FloatWith(idle) return state, nil } + +func shouldIgnore(pid int) (bool, err) { + // shouldIgnore checks if we should ignore the pid, to avoid elevated permissions + + // LSASS.exe is a process which has no useful cmdline arguments, we should ignore acessing such process to avoid triggering Windows ASR rules + // we can query pid for LASASS.exe from registry + + key, err := registry.OpenKey(registry.LOCAL_MACHINE, "SYSTEM\\CurrentControlSet\\Control\\Lsa", registry.READ) + if err != nil { + logp.L().Warnw("Failed to read registry path SYSTEM\\CurrentControlSet\\Control\\Lsa", "error", err) + return false + } + lsassPid, _, err := key.GetIntegerValue("LasPid") + if err != nil { + logp.L().Warnw("Failed to read pid for lsass.exe", "error", err) + return false + } + if lsassPid == pid { + return true + } + return false +} From 288153b53b3130733af5f4cb66b16ad30125e248 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Tue, 17 Dec 2024 23:57:13 +0530 Subject: [PATCH 02/12] lint --- metric/system/process/process_windows.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/metric/system/process/process_windows.go b/metric/system/process/process_windows.go index 238d73bce..b6af0563e 100644 --- a/metric/system/process/process_windows.go +++ b/metric/system/process/process_windows.go @@ -27,7 +27,9 @@ import ( "unsafe" xsyswindows "golang.org/x/sys/windows" + "golang.org/x/sys/windows/registry" + "github.com/elastic/elastic-agent-libs/logp" "github.com/elastic/elastic-agent-libs/opt" "github.com/elastic/elastic-agent-system-metrics/metric/system/resolve" gowindows "github.com/elastic/go-windows" @@ -467,7 +469,7 @@ func fillIdleProcess(state ProcState) (ProcState, error) { return state, nil } -func shouldIgnore(pid int) (bool, err) { +func shouldIgnore(pid int) bool { // shouldIgnore checks if we should ignore the pid, to avoid elevated permissions // LSASS.exe is a process which has no useful cmdline arguments, we should ignore acessing such process to avoid triggering Windows ASR rules From 0edee1afc6213d5a9fb3834b467699aebacd8d22 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Wed, 18 Dec 2024 00:00:22 +0530 Subject: [PATCH 03/12] avoid leaks --- metric/system/process/process_windows.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/metric/system/process/process_windows.go b/metric/system/process/process_windows.go index b6af0563e..533ee7e7b 100644 --- a/metric/system/process/process_windows.go +++ b/metric/system/process/process_windows.go @@ -202,7 +202,7 @@ func FillMetricsRequiringMoreAccess(pid int, state ProcState) (ProcState, error) func getProcArgs(pid int) ([]string, error) { if ok := shouldIgnore(pid); ok { - return []string{}, nil + return nil, nil } handle, err := syscall.OpenProcess( windows.PROCESS_QUERY_LIMITED_INFORMATION| @@ -480,6 +480,7 @@ func shouldIgnore(pid int) bool { logp.L().Warnw("Failed to read registry path SYSTEM\\CurrentControlSet\\Control\\Lsa", "error", err) return false } + defer key.Close() lsassPid, _, err := key.GetIntegerValue("LasPid") if err != nil { logp.L().Warnw("Failed to read pid for lsass.exe", "error", err) From a13779e9dd31369f1334f78ed7f8bcd8a4c2563d Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Wed, 18 Dec 2024 17:20:54 +0530 Subject: [PATCH 04/12] chore: move to global --- metric/system/process/helpers_others.go | 4 ++++ metric/system/process/helpers_windows.go | 22 +++++++++++++++++++ metric/system/process/process.go | 18 ++++++++------- metric/system/process/process_common.go | 2 ++ metric/system/process/process_windows.go | 28 ------------------------ 5 files changed, 38 insertions(+), 36 deletions(-) diff --git a/metric/system/process/helpers_others.go b/metric/system/process/helpers_others.go index 2873a5e4c..7800fadac 100644 --- a/metric/system/process/helpers_others.go +++ b/metric/system/process/helpers_others.go @@ -33,3 +33,7 @@ func isNonFatal(err error) bool { errors.Is(err, syscall.EINVAL) || errors.Is(err, NonFatalErr{})) } + +func processesToIgnore() (m map[uint64]struct{}) { + return +} diff --git a/metric/system/process/helpers_windows.go b/metric/system/process/helpers_windows.go index 94e4b882d..e278643c7 100644 --- a/metric/system/process/helpers_windows.go +++ b/metric/system/process/helpers_windows.go @@ -23,7 +23,9 @@ import ( "errors" "syscall" + "github.com/elastic/elastic-agent-libs/logp" "golang.org/x/sys/windows" + "golang.org/x/sys/windows/registry" ) func isNonFatal(err error) bool { @@ -35,3 +37,23 @@ func isNonFatal(err error) bool { errors.Is(err, syscall.EINVAL) || errors.Is(err, windows.ERROR_INVALID_PARAMETER) || errors.Is(err, NonFatalErr{}) } + +func processesToIgnore() (m map[uint64]struct{}) { + // shouldIgnore checks if we should ignore the pid, to avoid elevated permissions + + // LSASS.exe is a process which has no useful cmdline arguments, we should ignore acessing such process to avoid triggering Windows ASR rules + // we can query pid for LASASS.exe from registry + key, err := registry.OpenKey(registry.LOCAL_MACHINE, "SYSTEM\\CurrentControlSet\\Control\\Lsa", registry.READ) + if err != nil { + logp.L().Warnw("Failed to read registry path SYSTEM\\CurrentControlSet\\Control\\Lsa", "error", err) + return m + } + defer key.Close() + lsassPid, _, err := key.GetIntegerValue("LasPid") + if err != nil { + logp.L().Warnw("Failed to read pid for lsass.exe", "error", err) + return m + } + m[lsassPid] = struct{}{} + return +} diff --git a/metric/system/process/process.go b/metric/system/process/process.go index 7045af80d..d9d99898f 100644 --- a/metric/system/process/process.go +++ b/metric/system/process/process.go @@ -291,15 +291,17 @@ func (procStats *Stats) pidFill(pid int, filter bool) (ProcState, bool, error) { } // end cgroups processor - status, err = FillMetricsRequiringMoreAccess(pid, status) - if err != nil { - procStats.logger.Debugf("error calling FillMetricsRequiringMoreAccess for pid %d: %w", pid, err) - } + if _, ok := procStats.excludedPIDs[uint64(pid)]; !ok { + status, err = FillMetricsRequiringMoreAccess(pid, status) + if err != nil { + procStats.logger.Debugf("error calling FillMetricsRequiringMoreAccess for pid %d: %w", pid, err) + } - // Generate `status.Cmdline` here for compatibility because on Windows - // `status.Args` is set by `FillMetricsRequiringMoreAccess`. - if len(status.Args) > 0 && status.Cmdline == "" { - status.Cmdline = strings.Join(status.Args, " ") + // Generate `status.Cmdline` here for compatibility because on Windows + // `status.Args` is set by `FillMetricsRequiringMoreAccess`. + if len(status.Args) > 0 && status.Cmdline == "" { + status.Cmdline = strings.Join(status.Args, " ") + } } // network data diff --git a/metric/system/process/process_common.go b/metric/system/process/process_common.go index 7427c3991..bb2dd5766 100644 --- a/metric/system/process/process_common.go +++ b/metric/system/process/process_common.go @@ -108,6 +108,7 @@ type Stats struct { cgroups *cgroup.Reader logger *logp.Logger host types.Host + excludedPIDs map[uint64]struct{} // List of PIDs to ignore while calling FillMetricsRequiringMoreAccess } // PidState are the constants for various PID states @@ -207,6 +208,7 @@ func (procStats *Stats) Init() error { } procStats.cgroups = cgReader } + procStats.excludedPIDs = processesToIgnore() return nil } diff --git a/metric/system/process/process_windows.go b/metric/system/process/process_windows.go index 533ee7e7b..01ea65012 100644 --- a/metric/system/process/process_windows.go +++ b/metric/system/process/process_windows.go @@ -27,9 +27,7 @@ import ( "unsafe" xsyswindows "golang.org/x/sys/windows" - "golang.org/x/sys/windows/registry" - "github.com/elastic/elastic-agent-libs/logp" "github.com/elastic/elastic-agent-libs/opt" "github.com/elastic/elastic-agent-system-metrics/metric/system/resolve" gowindows "github.com/elastic/go-windows" @@ -201,9 +199,6 @@ func FillMetricsRequiringMoreAccess(pid int, state ProcState) (ProcState, error) } func getProcArgs(pid int) ([]string, error) { - if ok := shouldIgnore(pid); ok { - return nil, nil - } handle, err := syscall.OpenProcess( windows.PROCESS_QUERY_LIMITED_INFORMATION| windows.PROCESS_VM_READ, @@ -468,26 +463,3 @@ func fillIdleProcess(state ProcState) (ProcState, error) { state.CPU.Total.Value = opt.FloatWith(idle) return state, nil } - -func shouldIgnore(pid int) bool { - // shouldIgnore checks if we should ignore the pid, to avoid elevated permissions - - // LSASS.exe is a process which has no useful cmdline arguments, we should ignore acessing such process to avoid triggering Windows ASR rules - // we can query pid for LASASS.exe from registry - - key, err := registry.OpenKey(registry.LOCAL_MACHINE, "SYSTEM\\CurrentControlSet\\Control\\Lsa", registry.READ) - if err != nil { - logp.L().Warnw("Failed to read registry path SYSTEM\\CurrentControlSet\\Control\\Lsa", "error", err) - return false - } - defer key.Close() - lsassPid, _, err := key.GetIntegerValue("LasPid") - if err != nil { - logp.L().Warnw("Failed to read pid for lsass.exe", "error", err) - return false - } - if lsassPid == pid { - return true - } - return false -} From d2221d61e47dba2e38b360d7bfa285c9677509c4 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Wed, 18 Dec 2024 17:24:05 +0530 Subject: [PATCH 05/12] lint --- metric/system/process/helpers_others.go | 2 +- metric/system/process/helpers_windows.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/metric/system/process/helpers_others.go b/metric/system/process/helpers_others.go index 7800fadac..3d7f2df16 100644 --- a/metric/system/process/helpers_others.go +++ b/metric/system/process/helpers_others.go @@ -35,5 +35,5 @@ func isNonFatal(err error) bool { } func processesToIgnore() (m map[uint64]struct{}) { - return + return m } diff --git a/metric/system/process/helpers_windows.go b/metric/system/process/helpers_windows.go index e278643c7..eb59c2cb2 100644 --- a/metric/system/process/helpers_windows.go +++ b/metric/system/process/helpers_windows.go @@ -55,5 +55,5 @@ func processesToIgnore() (m map[uint64]struct{}) { return m } m[lsassPid] = struct{}{} - return + return m } From e8dc50875f506d4a7dfcda9c2d9214a13dd5615b Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Wed, 18 Dec 2024 17:24:37 +0530 Subject: [PATCH 06/12] rename --- metric/system/process/helpers_windows.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metric/system/process/helpers_windows.go b/metric/system/process/helpers_windows.go index eb59c2cb2..f3199e77c 100644 --- a/metric/system/process/helpers_windows.go +++ b/metric/system/process/helpers_windows.go @@ -39,7 +39,7 @@ func isNonFatal(err error) bool { } func processesToIgnore() (m map[uint64]struct{}) { - // shouldIgnore checks if we should ignore the pid, to avoid elevated permissions + // processesToIgnore checks if we should ignore the pid, to avoid elevated permissions // LSASS.exe is a process which has no useful cmdline arguments, we should ignore acessing such process to avoid triggering Windows ASR rules // we can query pid for LASASS.exe from registry From 144a6e1c4408937b7fd4701cb71a250e58bb27c4 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Wed, 18 Dec 2024 17:25:22 +0530 Subject: [PATCH 07/12] rename --- metric/system/process/process.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metric/system/process/process.go b/metric/system/process/process.go index d9d99898f..a9573a554 100644 --- a/metric/system/process/process.go +++ b/metric/system/process/process.go @@ -291,7 +291,7 @@ func (procStats *Stats) pidFill(pid int, filter bool) (ProcState, bool, error) { } // end cgroups processor - if _, ok := procStats.excludedPIDs[uint64(pid)]; !ok { + if _, isExcluded := procStats.excludedPIDs[uint64(pid)]; !isExcluded { status, err = FillMetricsRequiringMoreAccess(pid, status) if err != nil { procStats.logger.Debugf("error calling FillMetricsRequiringMoreAccess for pid %d: %w", pid, err) From 33fcaec96311c1b468e7fb02284b71740b5257e9 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Wed, 18 Dec 2024 17:33:43 +0530 Subject: [PATCH 08/12] goimports --- metric/system/process/helpers_windows.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/metric/system/process/helpers_windows.go b/metric/system/process/helpers_windows.go index f3199e77c..36590335c 100644 --- a/metric/system/process/helpers_windows.go +++ b/metric/system/process/helpers_windows.go @@ -23,9 +23,10 @@ import ( "errors" "syscall" - "github.com/elastic/elastic-agent-libs/logp" "golang.org/x/sys/windows" "golang.org/x/sys/windows/registry" + + "github.com/elastic/elastic-agent-libs/logp" ) func isNonFatal(err error) bool { From 72566ffdea1b078057e931a6c3b269ceef18fdd5 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Wed, 18 Dec 2024 17:56:08 +0530 Subject: [PATCH 09/12] fix panic --- metric/system/process/helpers_others.go | 4 ++-- metric/system/process/helpers_windows.go | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/metric/system/process/helpers_others.go b/metric/system/process/helpers_others.go index 3d7f2df16..163662b2d 100644 --- a/metric/system/process/helpers_others.go +++ b/metric/system/process/helpers_others.go @@ -34,6 +34,6 @@ func isNonFatal(err error) bool { errors.Is(err, NonFatalErr{})) } -func processesToIgnore() (m map[uint64]struct{}) { - return m +func processesToIgnore() map[uint64]struct{} { + return map[uint64]struct{}{} } diff --git a/metric/system/process/helpers_windows.go b/metric/system/process/helpers_windows.go index 36590335c..8751d87d4 100644 --- a/metric/system/process/helpers_windows.go +++ b/metric/system/process/helpers_windows.go @@ -39,7 +39,8 @@ func isNonFatal(err error) bool { errors.Is(err, windows.ERROR_INVALID_PARAMETER) || errors.Is(err, NonFatalErr{}) } -func processesToIgnore() (m map[uint64]struct{}) { +func processesToIgnore() map[uint64]struct{} { + m := make(map[uint64]struct{}) // processesToIgnore checks if we should ignore the pid, to avoid elevated permissions // LSASS.exe is a process which has no useful cmdline arguments, we should ignore acessing such process to avoid triggering Windows ASR rules From bcc8909b9116472ee8cacd1bc285312931afafb2 Mon Sep 17 00:00:00 2001 From: vihas makwana Date: Fri, 20 Dec 2024 15:54:35 +0530 Subject: [PATCH 10/12] add UT --- metric/system/process/helpers_windows.go | 2 +- metric/system/process/process_test.go | 26 +++++++++++++++++++ metric/system/process/process_windows_test.go | 13 ++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/metric/system/process/helpers_windows.go b/metric/system/process/helpers_windows.go index 8751d87d4..f2abffd51 100644 --- a/metric/system/process/helpers_windows.go +++ b/metric/system/process/helpers_windows.go @@ -51,7 +51,7 @@ func processesToIgnore() map[uint64]struct{} { return m } defer key.Close() - lsassPid, _, err := key.GetIntegerValue("LasPid") + lsassPid, _, err := key.GetIntegerValue("LsaPid") if err != nil { logp.L().Warnw("Failed to read pid for lsass.exe", "error", err) return m diff --git a/metric/system/process/process_test.go b/metric/system/process/process_test.go index b51b0fb3d..014c68053 100644 --- a/metric/system/process/process_test.go +++ b/metric/system/process/process_test.go @@ -633,6 +633,32 @@ func TestIncludeTopProcesses(t *testing.T) { } } +func TestProcessesExcluded(t *testing.T) { + state := Stats{ + Procs: []string{".*"}, + Hostfs: resolve.NewTestResolver("/"), + CPUTicks: false, + } + require.NoError(t, state.Init()) + + _, processes, err := state.Get() + require.NoError(t, err) + + for _, pMap := range processes { + iPid, err := pMap.GetValue("process.pid") + require.NoError(t, err) + pid, ok := iPid.(int) // to avoid panics + require.True(t, ok) + if _, excluded := state.excludedPIDs[uint64(pid)]; excluded { + // if pid is excluded, its arglist amd commandline should be empty + ok, _ = pMap.HasKey("process.args") + require.False(t, ok) + ok, _ = pMap.HasKey("process.command_line") + require.False(t, ok) + } + } +} + // runThreads run the threads binary for the current GOOS. // //go:generate docker run --rm -v ./testdata:/app --entrypoint g++ docker.elastic.co/beats-dev/golang-crossbuild:1.21.0-main -pthread -std=c++11 -o /app/threads /app/threads.cpp diff --git a/metric/system/process/process_windows_test.go b/metric/system/process/process_windows_test.go index 6eed759eb..11cedb823 100644 --- a/metric/system/process/process_windows_test.go +++ b/metric/system/process/process_windows_test.go @@ -69,3 +69,16 @@ func TestGetInfoForPid_numThreads(t *testing.T) { numThreads, want, expected) } } + +func TestLsassFound(t *testing.T) { + processNames := []string{"lsass.exe"} + m := processesToIgnore() + require.NotEmpty(t, m) + + for pid := range m { + state, err := GetInfoForPid(resolve.NewTestResolver("/"), int(pid)) + if err == nil { + require.Contains(t, processNames, state.Name) + } + } +} From 6b19228df0f46ba12c5ac14b4033ce76e0832b86 Mon Sep 17 00:00:00 2001 From: Vihas Makwana Date: Fri, 20 Dec 2024 16:41:26 +0530 Subject: [PATCH 11/12] fix test --- metric/system/process/process_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metric/system/process/process_test.go b/metric/system/process/process_test.go index 014c68053..112127f6b 100644 --- a/metric/system/process/process_test.go +++ b/metric/system/process/process_test.go @@ -642,7 +642,7 @@ func TestProcessesExcluded(t *testing.T) { require.NoError(t, state.Init()) _, processes, err := state.Get() - require.NoError(t, err) + assert.True(t, isNonFatal(err), fmt.Sprintf("Fatal Error: %s", err)) for _, pMap := range processes { iPid, err := pMap.GetValue("process.pid") From 757c86f02a67111d289d95167b2ac4b007e3fdf7 Mon Sep 17 00:00:00 2001 From: Vihas Makwana <121151420+VihasMakwana@users.noreply.github.com> Date: Fri, 27 Dec 2024 15:33:08 +0530 Subject: [PATCH 12/12] Update metric/system/process/helpers_windows.go Co-authored-by: Gabriel Landau <42078554+gabriellandau@users.noreply.github.com> --- metric/system/process/helpers_windows.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metric/system/process/helpers_windows.go b/metric/system/process/helpers_windows.go index f2abffd51..fc198ab0e 100644 --- a/metric/system/process/helpers_windows.go +++ b/metric/system/process/helpers_windows.go @@ -47,7 +47,7 @@ func processesToIgnore() map[uint64]struct{} { // we can query pid for LASASS.exe from registry key, err := registry.OpenKey(registry.LOCAL_MACHINE, "SYSTEM\\CurrentControlSet\\Control\\Lsa", registry.READ) if err != nil { - logp.L().Warnw("Failed to read registry path SYSTEM\\CurrentControlSet\\Control\\Lsa", "error", err) + logp.L().Warnw("Failed to open registry path SYSTEM\\CurrentControlSet\\Control\\Lsa", "error", err) return m } defer key.Close()