Harden Windows System Probe named pipe (DataDog#32354)

cmd/agent/subcommands/flare/command_windows_test.go

@@ -32,7 +32,8 @@ func sysprobeSocketPath(_ *testing.T) string {
 func NewSystemProbeTestServer(handler http.Handler) (*httptest.Server, error) {
 	server := httptest.NewUnstartedServer(handler)
 
-	conn, err := sysprobeserver.NewListener(systemProbeTestPipeName)
+	// The test named pipe allows the current user.
+	conn, err := sysprobeserver.NewListenerForCurrentUser(systemProbeTestPipeName)
 	if err != nil {
 		return nil, err
 	}

cmd/system-probe/api/server/listener_windows.go

@@ -8,6 +8,10 @@ package server
 import (
 	"fmt"
 	"net"
+	"strings"
+
+	"github.com/DataDog/datadog-agent/pkg/util/log"
+	"github.com/DataDog/datadog-agent/pkg/util/winutil"
 
 	"github.com/Microsoft/go-winio"
 )
@@ -19,18 +23,76 @@ const (
 	namedPipeInputBufferSize  = int32(4096)
 	namedPipeOutputBufferSize = int32(4096)
 
-	// DACL for the system probe named pipe.
+	// DACL template for the system probe named pipe that allows a specific SID.
 	// SE_DACL_PROTECTED (P), SE_DACL_AUTO_INHERITED (AI)
-	// Allow Everyone (WD)
-	// nolint:revive // TODO: Hardened DACL and ensure the datadogagent run-as user is allowed.
-	namedPipeSecurityDescriptor = "D:PAI(A;;FA;;;WD)"
+	// Allow Administorators (BA), Local System (SY)
+	// Allow a custom SID, NO_PROPAGATE_INHERIT_ACE (NP)
+	namedPipeSecurityDescriptorTemplate = "D:PAI(A;;FA;;;BA)(A;;FA;;;SY)(A;NP;FRFW;;;%s)"
+
+	// Default DACL for the system probe named pipe.
+	// Allow Administorators (BA), Local System (SY)
+	namedPipeDefaultSecurityDescriptor = "D:PAI(A;;FA;;;BA)(A;;FA;;;SY)"
+
+	// SID representing Everyone
+	everyoneSid = "S-1-1-0"
 )
 
+// setupSecurityDescriptor prepares the security descriptor for the system probe named pipe.
+func setupSecurityDescriptor() (string, error) {
+	// Set up the DACL to allow ddagentuser.
+	sid, err := winutil.GetDDAgentUserSID()
+	if err != nil {
+		return "", fmt.Errorf("failed to get SID for ddagentuser: %s", err)
+	}
+
+	sidString := sid.String()
+
+	// Sanity checks
+	if len(sidString) == 0 {
+		return "", fmt.Errorf("failed to get SID string from ddagentuser")
+	}
+
+	if sidString == everyoneSid {
+		return "", fmt.Errorf("ddagentuser as Everyone is not supported")
+	}
+
+	sd, err := formatSecurityDescriptorWithSid(sidString)
+	if err != nil {
+		return "", fmt.Errorf("invalid SID from ddagentuser: %s", err)
+	}
+
+	log.Debugf("named pipe DACL prepared with ddagentuser %s", sidString)
+	return sd, nil
+}
+
+// formatSecurityDescriptorWithSid creates a security descriptor string for the system probe
+// named pipe that allows a set of default users and the specified SID.
+func formatSecurityDescriptorWithSid(sidString string) (string, error) {
+	// Sanity check
+	if !strings.HasPrefix(sidString, "S-") {
+		return "", fmt.Errorf("invalid SID %s", sidString)
+	}
+	return fmt.Sprintf(namedPipeSecurityDescriptorTemplate, sidString), nil
+}
+
 // NewListener sets up a named pipe listener for the system probe service.
 func NewListener(namedPipeName string) (net.Listener, error) {
-	// The DACL must allow the run-as user of datadogagent.
+	sd, err := setupSecurityDescriptor()
+	if err != nil {
+		log.Errorf("failed to setup security descriptor, ddagentuser is denied: %s", err)
+
+		// The default security descriptor does not include ddagentuser.
+		// Queries from the DD agent will fail.
+		sd = namedPipeDefaultSecurityDescriptor
+	}
+
+	return newListenerWithSecurityDescriptor(namedPipeName, sd)
+}
+
+// newListenerWithSecurityDescriptor sets up a named pipe listener with a security descriptor.
+func newListenerWithSecurityDescriptor(namedPipeName string, securityDescriptor string) (net.Listener, error) {
 	config := winio.PipeConfig{
-		SecurityDescriptor: namedPipeSecurityDescriptor,
+		SecurityDescriptor: securityDescriptor,
 		InputBufferSize:    namedPipeInputBufferSize,
 		OutputBufferSize:   namedPipeOutputBufferSize,
 	}
@@ -41,5 +103,8 @@ func NewListener(namedPipeName string) (net.Listener, error) {
 	if err != nil {
 		return nil, fmt.Errorf("named pipe listener %q: %s", namedPipeName, err)
 	}
+
+	log.Infof("named pipe %s ready", namedPipeName)
+
 	return namedPipe, nil
 }

cmd/system-probe/api/server/listener_windows_testutil.go

@@ -0,0 +1,30 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2024-present Datadog, Inc.
+
+//go:build test && windows
+
+package server
+
+import (
+	"net"
+	"os/user"
+)
+
+// NewListenerForCurrentUser sets up a named pipe listener for tests that mock system probe.
+// Do not use this for the normal system probe named pipe.
+func NewListenerForCurrentUser(namedPipeName string) (net.Listener, error) {
+	// Prepare a security descriptor that allows the current user.
+	currentUser, err := user.Current()
+	if err != nil {
+		return nil, err
+	}
+
+	sd, err := formatSecurityDescriptorWithSid(currentUser.Uid)
+	if err != nil {
+		return nil, err
+	}
+
+	return newListenerWithSecurityDescriptor(namedPipeName, sd)
+}

pkg/collector/corechecks/system/wincrashdetect/wincrashdetect_windows_test.go

@@ -49,7 +49,8 @@ func TestWinCrashReporting(t *testing.T) {
 	mockSysProbeConfig.SetWithoutSource("system_probe_config.enabled", true)
 	mockSysProbeConfig.SetWithoutSource("system_probe_config.sysprobe_socket", systemProbeTestPipeName)
 
-	listener, err := server.NewListener(systemProbeTestPipeName)
+	// The test named pipe allows the current user.
+	listener, err := server.NewListenerForCurrentUser(systemProbeTestPipeName)
 	require.NoError(t, err)
 	t.Cleanup(func() { _ = listener.Close() })
 
@@ -182,7 +183,8 @@ func TestCrashReportingStates(t *testing.T) {
 
 	var crashStatus *probe.WinCrashStatus
 
-	listener, err := server.NewListener(systemProbeTestPipeName)
+	// The test named pipe allows the current user.
+	listener, err := server.NewListenerForCurrentUser(systemProbeTestPipeName)
 	require.NoError(t, err)
 	t.Cleanup(func() { _ = listener.Close() })
 

releasenotes/notes/windows_system_probe_namedpipe_hardening-cfc08b47465f1f2b.yaml

@@ -0,0 +1,4 @@
+security:
+  - |
+    On Windows, the named pipe \\.\pipe\dd_system_probe from system probe is now restricted to
+    Local System, Administrators, and the ddagentuser.  Any other custom users are not supported.