fix(file): rotate active file if it points to a symbolic link (#274)

* fix: rotate active file if it points to a symlink

* use lowercase naming for assert func

* close logger in test

* explicitly cleanup files to avoid windows issue

* add missing file close

Author: mauri870

file/rotator.go

@@ -20,6 +20,7 @@ package file
 import (
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"sort"
@@ -245,13 +246,27 @@ func (r *Rotator) openNew() error {
 		return fmt.Errorf("failed to make directories for new file: %w", err)
 	}
 
-	_, err = os.Stat(r.rot.ActiveFile())
+	stat, err := os.Lstat(r.rot.ActiveFile())
 	if err == nil {
+		isSymlink := stat.Mode()&fs.ModeSymlink != 0
+
 		// check if the file has to be rotated before writing to it
 		reason, t := r.isRotationTriggered(0)
 		if reason == rotateReasonNoRotate {
-			return r.appendToFile()
+			// To avoid symlink following attacks, if the active file is a symlink
+			// we need to rotate it to avoid writing to the symlink target, which
+			// could be a sensitive or protected file not owned by us.
+			if isSymlink {
+				if r.log != nil {
+					r.log.Debugw("Active file is a symlink, forcing rotation", "filename", r.rot.ActiveFile())
+				}
+				reason = rotateReasonInitializing
+				t = r.clock.Now()
+			} else {
+				return r.appendToFile()
+			}
 		}
+
 		if err = r.rot.Rotate(reason, t); err != nil {
 			return fmt.Errorf("failed to rotate backups: %w", err)
 		}
@@ -284,7 +299,7 @@ func (r *Rotator) openFile() error {
 		return fmt.Errorf("failed to make directories for new file: %w", err)
 	}
 
-	r.file, err = os.OpenFile(r.rot.ActiveFile(), os.O_CREATE|os.O_WRONLY|os.O_TRUNC, r.permissions)
+	r.file, err = os.OpenFile(r.rot.ActiveFile(), os.O_EXCL|os.O_CREATE|os.O_WRONLY|os.O_TRUNC, r.permissions)
 	if err != nil {
 		return fmt.Errorf("failed to open new file '%s': %w", r.rot.ActiveFile(), err)
 	}

file/rotator_test.go

@@ -255,6 +255,49 @@ func TestRotate(t *testing.T) {
 	AssertDirContents(t, dir, secondFile, thirdFile)
 }
 
+func TestRotateSymlink(t *testing.T) {
+	dir := t.TempDir()
+
+	logname := "beatname"
+	filename := filepath.Join(dir, logname)
+
+	c := &testClock{time.Date(2021, 11, 11, 0, 0, 0, 0, time.Local)}
+
+	privateFileContents := []byte("original contents")
+	privateFile := filepath.Join(dir, "private")
+	err := os.WriteFile(privateFile, privateFileContents, 0644)
+	require.NoError(t, err)
+
+	// Plant a symlink to the private file by guessing the log filename.
+	guessedFilename := filepath.Join(dir, fmt.Sprintf("%s-%s.ndjson", logname, c.Now().Format(file.DateFormat)))
+	err = os.Symlink(privateFile, guessedFilename)
+	require.NoError(t, err)
+
+	logger, buf := logp.NewInMemory("rotator", logp.ConsoleEncoderConfig())
+
+	r, err := file.NewFileRotator(filename,
+		file.MaxBackups(1),
+		file.WithClock(c),
+		file.RotateOnStartup(false),
+		file.WithLogger(logger),
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+
+	WriteMsg(t, r)
+
+	// The file rotation should have detected the destination is a symlink and rotated before writing.
+	rotatedFilename := filepath.Join(dir, fmt.Sprintf("%s-%s-1.ndjson", logname, c.Now().Format(file.DateFormat)))
+	AssertDirContents(t, dir, filepath.Base(privateFile), filepath.Base(guessedFilename), filepath.Base(rotatedFilename))
+	require.Contains(t, buf.String(), "Active file is a symlink, forcing rotation")
+
+	got, err := os.ReadFile(privateFile)
+	require.NoError(t, err)
+	assert.Equal(t, privateFileContents, got, "The symlink target should not have been modified")
+}
+
 func TestRotateExtension(t *testing.T) {
 	dir := t.TempDir()
 
@@ -312,6 +355,7 @@ func AssertDirContents(t *testing.T, dir string, files ...string) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer f.Close()
 
 	names, err := f.Readdirnames(-1)
 	if err != nil {

logp/defaults_test.go

@@ -32,8 +32,10 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zapcore"
 
+	"github.com/elastic/elastic-agent-libs/file"
 	"github.com/elastic/elastic-agent-libs/logp"
 )
 
@@ -437,3 +439,66 @@ func takeAllLogsFromPath(t *testing.T, path string) []map[string]any {
 
 	return entries
 }
+
+func TestLoggerRotateSymlink(t *testing.T) {
+	dir := t.TempDir()
+
+	cfg := logp.DefaultConfig(logp.DefaultEnvironment)
+	cfg.Beat = "logger"
+	cfg.ToFiles = true
+	cfg.Files.Path = dir
+	cfg.Files.MaxBackups = 1
+	cfg.Files.RotateOnStartup = false
+
+	logname := cfg.Beat
+
+	privateFileContents := []byte("original contents")
+	privateFile := filepath.Join(dir, "private")
+	err := os.WriteFile(privateFile, privateFileContents, 0644)
+	require.NoError(t, err)
+
+	// Plant a symlink to the private file by guessing the log filename.
+	guessedFilename := filepath.Join(dir, fmt.Sprintf("%s-%s.ndjson", logname, time.Now().Format(file.DateFormat)))
+	err = os.Symlink(privateFile, guessedFilename)
+	require.NoError(t, err)
+
+	err = logp.Configure(cfg)
+	require.NoError(t, err)
+
+	logLine := "a info message"
+	logp.L().Info(logLine)
+
+	// The file rotation should have detected the destination is a symlink and rotated before writing.
+	rotatedFilename := filepath.Join(dir, fmt.Sprintf("%s-%s-1.ndjson", logname, time.Now().Format(file.DateFormat)))
+	assertDirContents(t, dir, filepath.Base(privateFile), filepath.Base(guessedFilename), filepath.Base(rotatedFilename))
+
+	got, err := os.ReadFile(privateFile)
+	require.NoError(t, err)
+	assert.Equal(t, privateFileContents, got, "The symlink target should not have been modified")
+
+	got, err = os.ReadFile(rotatedFilename)
+	require.NoError(t, err)
+	assert.Contains(t, string(got), logLine, "The rotated file should contain the log message")
+
+	assert.NoError(t, logp.L().Close())
+
+	// Error: TempDir RemoveAll cleanup: remove t.TempDir() The process cannot access the file because it is being used by another process.
+	require.NoError(t, os.RemoveAll(dir))
+}
+
+func assertDirContents(t *testing.T, dir string, files ...string) {
+	t.Helper()
+
+	f, err := os.Open(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+
+	names, err := f.Readdirnames(-1)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	assert.ElementsMatch(t, files, names)
+}