privileged-logs: add NoFollow transport + fix directory permission check

Add end-to-end support for requesting a symlink-rejecting ("no-follow") open
through the privileged-logs client/module RPC:

- common.OpenFileRequest gets a NoFollow bool field.
- client.Open/OpenPrivileged gain OpenNoFollow/OpenPrivilegedNoFollow
  counterparts. On Linux, OpenNoFollow calls common.OpenPathWithoutSymlinks
  directly instead of os.Open; OpenPrivilegedNoFollow sets NoFollow on the
  wire request. On non-Linux, OpenNoFollow falls back to a plain open (no
  error) since symlink rejection is only meaningful for paths discovered via
  /proc/<pid>/fd, which is Linux-only.
- module.handler dispatches to a new validateAndOpenNoFollow when the
  request's NoFollow flag is set, which skips filepath.EvalSymlinks (the
  caller guarantees the path is already canonical) and goes straight to
  common.OpenPathWithoutSymlinks.

Also fixes a permission bug in common.OpenPathWithoutSymlinks, found via
Codex review: directory components were opened with O_RDONLY|O_DIRECTORY,
which requires *read* permission on every directory component. That's
stricter than the *search* (execute) permission a plain os.Open(path) needs,
and would incorrectly reject files sitting under directories that are
traversable but not listable (e.g. mode 0711). The existing module-side
caller (root-running system-probe) never hit this, since root bypasses the
extra permission check - it only becomes reachable with this change's new
unprivileged client-side caller. Switched to O_PATH for directory-component
opens, which only requires search permission, matching os.Open's semantics.

Note: a regression test for the permission bug specifically would need a
non-owner UID (root-only, or os/user + a helper process), which isn't added
here - flagging as a possible follow-up rather than skipping silently.

No caller uses OpenNoFollow/OpenPrivilegedNoFollow yet - checkFileReadable
and the file tailer still call the plain Open/OpenLogFile in this PR, so the
symlink-swap protection isn't active end-to-end here. That's intentional:
this PR is scoped to the transport capability itself; the next two PRs in
the stack add the LogsConfig.NoFollow field + tailer wiring, then flip
process_log discovery/tailing over to the no-follow calls.

Part of a stack towards DSCVR-475; split out of PR #51746 for easier review.
Depends on the "extract OpenPathWithoutSymlinks into common" refactor.

Environment: Datadog workspace

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>

Author: vitkyrka

pkg/privileged-logs/client/open.go

@@ -11,22 +11,32 @@ package client
 import (
 	"errors"
 	"os"
+	"syscall"
 
 	"bytes"
 	"encoding/json"
 	"fmt"
 	"net"
 	"net/http"
-	"syscall"
 
 	pkgconfigsetup "github.com/DataDog/datadog-agent/pkg/config/setup"
 	"github.com/DataDog/datadog-agent/pkg/privileged-logs/common"
 	"github.com/DataDog/datadog-agent/pkg/util/log"
 )
 
-// OpenPrivileged opens a file in system-probe and returns the file descriptor
-// This function uses a custom HTTP client that can handle file descriptor transfer
+// OpenPrivileged opens a file in system-probe and returns the file descriptor.
+// This function uses a custom HTTP client that can handle file descriptor transfer.
 func OpenPrivileged(socketPath string, filePath string) (*os.File, error) {
+	return openPrivileged(socketPath, filePath, false)
+}
+
+// OpenPrivilegedNoFollow opens a file in system-probe without following symbolic
+// links in any path component.
+func OpenPrivilegedNoFollow(socketPath string, filePath string) (*os.File, error) {
+	return openPrivileged(socketPath, filePath, true)
+}
+
+func openPrivileged(socketPath string, filePath string, noFollow bool) (*os.File, error) {
 	// Create a new connection instead of reusing the shared connection from
 	// pkg/system-probe/api/client/client.go, since the connection is hijacked
 	// from the control of the HTTP server library on the server side.  It also
@@ -39,7 +49,8 @@ func OpenPrivileged(socketPath string, filePath string) (*os.File, error) {
 	defer conn.Close()
 
 	req := common.OpenFileRequest{
-		Path: filePath,
+		Path:     filePath,
+		NoFollow: noFollow,
 	}
 
 	reqBody, err := json.Marshal(req)
@@ -111,7 +122,7 @@ func OpenPrivileged(socketPath string, filePath string) (*os.File, error) {
 	return nil, errors.New("no file descriptor received")
 }
 
-func maybeOpenPrivileged(path string, originalError error) (*os.File, error) {
+func maybeOpenPrivileged(path string, originalError error, noFollow bool) (*os.File, error) {
 	enabled := pkgconfigsetup.SystemProbe().GetBool("privileged_logs.enabled")
 	if !enabled {
 		return nil, originalError
@@ -120,7 +131,7 @@ func maybeOpenPrivileged(path string, originalError error) (*os.File, error) {
 	log.Debugf("Permission denied, opening file with system-probe: %v", path)
 
 	socketPath := pkgconfigsetup.SystemProbe().GetString("system_probe_config.sysprobe_socket")
-	file, spErr := OpenPrivileged(socketPath, path)
+	file, spErr := openPrivileged(socketPath, path, noFollow)
 	log.Tracef("Opened file with system-probe: %v, err: %v", path, spErr)
 	if spErr != nil {
 		return nil, fmt.Errorf("failed to open file with system-probe: %w, original error: %w", spErr, originalError)
@@ -129,20 +140,34 @@ func maybeOpenPrivileged(path string, originalError error) (*os.File, error) {
 	return file, nil
 }
 
-// Open attempts to open a file, and if it fails due to permissions, it opens
-// the file using system-probe if the privileged logs module is available.
+// Open attempts to open a file with normal path resolution. If opening fails
+// with permission denied, the file may be opened via system-probe when
+// privileged logs is enabled.
 func Open(path string) (*os.File, error) {
-	file, err := os.Open(path)
+	return open(path, false)
+}
+
+// OpenNoFollow attempts to open a file without following symbolic links in any
+// path component. If opening fails with permission denied, the file may be
+// opened via system-probe with the same no-follow behavior.
+func OpenNoFollow(path string) (*os.File, error) {
+	return open(path, true)
+}
+
+func open(path string, noFollow bool) (*os.File, error) {
+	var file *os.File
+	var err error
+
+	if noFollow {
+		file, err = common.OpenPathWithoutSymlinks(path)
+	} else {
+		file, err = os.Open(path)
+	}
 	if err == nil || !errors.Is(err, os.ErrPermission) {
 		return file, err
 	}
 
-	file, err = maybeOpenPrivileged(path, err)
-	if err != nil {
-		return nil, err
-	}
-
-	return file, nil
+	return maybeOpenPrivileged(path, err, noFollow)
 }
 
 // Stat attempts to stat a file, and if it fails due to permissions, it opens
@@ -154,7 +179,7 @@ func Stat(path string) (os.FileInfo, error) {
 		return info, err
 	}
 
-	file, err := maybeOpenPrivileged(path, err)
+	file, err := maybeOpenPrivileged(path, err, false)
 	if err != nil {
 		return nil, err
 	}

pkg/privileged-logs/client/open_other.go

@@ -9,6 +9,7 @@
 package client
 
 import (
+	"errors"
 	"os"
 )
 
@@ -17,6 +18,24 @@ func Open(path string) (*os.File, error) {
 	return os.Open(path)
 }
 
+// OpenNoFollow falls back to a regular open on non-Linux platforms: symlink
+// rejection is only needed for process_log-discovered paths, and process_log
+// discovery (based on /proc/<pid>/fd) is Linux-only, so this path is not
+// reachable with an untrusted, attacker-controlled symlink swap here.
+func OpenNoFollow(path string) (*os.File, error) {
+	return os.Open(path)
+}
+
+// OpenPrivileged is not supported on non-Linux platforms.
+func OpenPrivileged(_, _ string) (*os.File, error) {
+	return nil, errors.ErrUnsupported
+}
+
+// OpenPrivilegedNoFollow is not supported on non-Linux platforms.
+func OpenPrivilegedNoFollow(_, _ string) (*os.File, error) {
+	return nil, errors.ErrUnsupported
+}
+
 // Stat provides a fallback for non-Linux platforms where the privileged logs module is not available.
 func Stat(path string) (os.FileInfo, error) {
 	return os.Stat(path)

pkg/privileged-logs/common/open_linux.go

@@ -26,8 +26,9 @@ import (
 // itself, but for our use case, the root directory itself can not be trusted to
 // not have changed since the time the path was validated.
 //
-// This function is shared by the privileged-logs module (server side,
-// root-running system-probe) and the privileged-logs client (agent side).
+// This function is used both by the privileged-logs module (server side,
+// root-running system-probe) and by the privileged-logs client (agent side),
+// to defend against symlink-swap attacks on process_log-discovered file paths.
 func OpenPathWithoutSymlinks(path string) (*os.File, error) {
 	if !filepath.IsAbs(path) {
 		return nil, fmt.Errorf("path must be absolute: %s", path)
@@ -36,7 +37,14 @@ func OpenPathWithoutSymlinks(path string) (*os.File, error) {
 	// Split path into components
 	parts := strings.Split(filepath.Clean(path), string(filepath.Separator))
 
-	dirFd, err := unix.Open("/", unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_DIRECTORY, 0)
+	// Directory components are opened with O_PATH rather than O_RDONLY.
+	// O_PATH only requires search (execute) permission to traverse into a
+	// directory, matching the permission check that a plain os.Open(path)
+	// would perform on the same directories.  O_RDONLY additionally
+	// requires read permission on every directory component, which would
+	// wrongly reject files under directories that are traversable but not
+	// listable (e.g. mode 0711).
+	dirFd, err := unix.Open("/", unix.O_PATH|unix.O_NOFOLLOW|unix.O_DIRECTORY, 0)
 	if err != nil {
 		return nil, fmt.Errorf("failed to open root directory: %w", err)
 	}
@@ -53,7 +61,7 @@ func OpenPathWithoutSymlinks(path string) (*os.File, error) {
 			continue
 		}
 
-		newFd, err := unix.Openat(dirFd, parts[i], unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_DIRECTORY, 0)
+		newFd, err := unix.Openat(dirFd, parts[i], unix.O_PATH|unix.O_NOFOLLOW|unix.O_DIRECTORY, 0)
 		if err != nil {
 			return nil, fmt.Errorf("failed to open directory component %s: %w", parts[i], err)
 		}
@@ -63,7 +71,9 @@ func OpenPathWithoutSymlinks(path string) (*os.File, error) {
 		dirFd = newFd
 	}
 
-	// Open the final file component with O_NOFOLLOW
+	// Open the final file component with O_NOFOLLOW. dirFd was opened with
+	// O_PATH, so this openat still enforces the normal read-permission check
+	// on the file itself.
 	fileName := parts[len(parts)-1]
 	fileFd, err := unix.Openat(dirFd, fileName, unix.O_RDONLY|unix.O_NOFOLLOW, 0)
 	if err != nil {

pkg/privileged-logs/common/types.go

@@ -9,6 +9,12 @@ package common
 // OpenFileRequest represents a request to open a file and transfer its file descriptor
 type OpenFileRequest struct {
 	Path string `json:"path"`
+	// NoFollow, when true, asks the module to open the file without following any
+	// symbolic links in the path.  This is used for process_log-discovered paths,
+	// which are canonical at discovery time; a symlink found later indicates an
+	// attacker-controlled swap.  When false (the default), symbolic links are
+	// resolved as usual.
+	NoFollow bool `json:"no_follow,omitempty"`
 }
 
 // OpenFileResponse represents the response from the file descriptor transfer

pkg/privileged-logs/module/handler.go

@@ -13,6 +13,7 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"os"
 	"syscall"
 
 	"github.com/DataDog/datadog-agent/pkg/privileged-logs/common"
@@ -85,7 +86,12 @@ func (f *privilegedLogsModule) openFileHandler(w http.ResponseWriter, r *http.Re
 
 	f.logFileAccess(req.Path)
 
-	file, err := validateAndOpen(req.Path)
+	var file *os.File
+	if req.NoFollow {
+		file, err = validateAndOpenNoFollow(req.Path)
+	} else {
+		file, err = validateAndOpen(req.Path)
+	}
 	if err != nil {
 		f.sendErrorResponse(unixConn, err.Error())
 		return

pkg/privileged-logs/module/validate.go

@@ -61,36 +61,69 @@ func isTextFile(file *os.File) bool {
 }
 
 func validateAndOpenWithPrefix(path, allowedPrefix string, toctou func()) (*os.File, error) {
+	resolvedPath, err := resolveFollowPath(path)
+	if err != nil {
+		return nil, err
+	}
+
+	// Callback for tests to change the filesystem after we called EvalSymlinks,
+	// in order to simulate a TOCTOU attack.
+	if toctou != nil {
+		toctou()
+	}
+
+	return validateResolvedAndOpen(resolvedPath, allowedPrefix)
+}
+
+func validateAndOpenNoFollowWithPrefix(path, allowedPrefix string) (*os.File, error) {
+	resolvedPath, err := resolveNoFollowPath(path)
+	if err != nil {
+		return nil, err
+	}
+	return validateResolvedAndOpen(resolvedPath, allowedPrefix)
+}
+
+func resolveFollowPath(path string) (string, error) {
 	if path == "" {
-		return nil, errors.New("empty file path provided")
+		return "", errors.New("empty file path provided")
 	}
 
 	if !filepath.IsAbs(path) {
-		return nil, fmt.Errorf("relative path not allowed: %s", path)
+		return "", fmt.Errorf("relative path not allowed: %s", path)
 	}
 
-	// Resolve symbolic links for the path and file name checks.
 	resolvedPath, err := filepath.EvalSymlinks(path)
 	if err != nil {
-		return nil, fmt.Errorf("failed to resolve path %s: %w", path, err)
+		return "", fmt.Errorf("failed to resolve path %s: %w", path, err)
 	}
+	return resolvedPath, nil
+}
 
-	// Callback for tests to change the filesystem after we called EvalSymlinks,
-	// in order to simulate a TOCTOU attack.
-	if toctou != nil {
-		toctou()
+func resolveNoFollowPath(path string) (string, error) {
+	if path == "" {
+		return "", errors.New("empty file path provided")
 	}
 
-	var file *os.File
+	if !filepath.IsAbs(path) {
+		return "", fmt.Errorf("relative path not allowed: %s", path)
+	}
 
+	// In no-follow mode the caller guarantees the path is canonical. Skip
+	// EvalSymlinks entirely: open every component with O_NOFOLLOW so that a
+	// symlink planted after the agent's discovery check causes an immediate error.
+	return filepath.Clean(path), nil
+}
+
+func validateResolvedAndOpen(resolvedPath, allowedPrefix string) (*os.File, error) {
 	if !isAllowed(resolvedPath, allowedPrefix) {
 		return nil, fmt.Errorf("non-log file not allowed: %s", resolvedPath)
 	}
 
-	// We use common.OpenPathWithoutSymlinks on the resolved path to verify each
+	// We use openPathWithoutSymlinks on the resolved path to verify each
 	// component with O_NOFOLLOW to ensure that none of the path components
-	// were replaced with symlinks after we called EvalSymlinks.
-	file, err = common.OpenPathWithoutSymlinks(resolvedPath)
+	// were replaced with symlinks after we called EvalSymlinks (follow mode),
+	// or to enforce that the path has no symlinks at all (no-follow mode).
+	file, err := common.OpenPathWithoutSymlinks(resolvedPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to open path %s: %w", resolvedPath, err)
 	}
@@ -118,3 +151,7 @@ func validateAndOpenWithPrefix(path, allowedPrefix string, toctou func()) (*os.F
 func validateAndOpen(path string) (*os.File, error) {
 	return validateAndOpenWithPrefix(path, "/var/log/", nil)
 }
+
+func validateAndOpenNoFollow(path string) (*os.File, error) {
+	return validateAndOpenNoFollowWithPrefix(path, "/var/log/")
+}

pkg/privileged-logs/module/validate_test.go

@@ -810,3 +810,35 @@ func TestValidateAndOpenWithPrefixTOCTOUFileSymlink(t *testing.T) {
 	assert.Nil(t, file)
 	assert.True(t, toctouCalled)
 }
+
+// TestValidateAndOpenWithPrefixNoFollowRejectsSymlink verifies that when noFollow
+// is true, validateAndOpenWithPrefix refuses to open a path that is (or has
+// become) a symlink, even when the symlink target would be allow-listed.  This
+// closes the residual TOCTOU where an attacker swaps a process_log-discovered file
+// to a symlink pointing at a root-readable log that the module would normally allow.
+func TestValidateAndOpenWithPrefixNoFollowRejectsSymlink(t *testing.T) {
+	testDir := t.TempDir()
+
+	// A real log file to discover initially
+	logFile := filepath.Join(testDir, "app.log")
+	require.NoError(t, os.WriteFile(logFile, []byte("log content"), 0644))
+
+	// Attacker swaps app.log for a symlink to an allow-listed target
+	targetLog := filepath.Join(testDir, "secret.log")
+	require.NoError(t, os.WriteFile(targetLog, []byte("secret content"), 0644))
+
+	require.NoError(t, os.Remove(logFile))
+	require.NoError(t, os.Symlink(targetLog, logFile))
+
+	// In noFollow mode the symlink must be rejected even though the target ends in .log
+	file, err := validateAndOpenNoFollowWithPrefix(logFile, testDir+"/")
+	assert.Error(t, err)
+	assert.Nil(t, file)
+
+	// In standard (follow) mode the symlink would succeed
+	file2, err2 := validateAndOpenWithPrefix(logFile, testDir+"/", nil)
+	if err2 == nil {
+		// accepted as expected in follow mode
+		file2.Close()
+	}
+}