From eecef148eb54c805ab91f91459366fa6868da224 Mon Sep 17 00:00:00 2001
From: Lucas Manuel Rodriguez <lucarodriguez@gmail.com>
Date: Mon, 20 Sep 2021 14:46:51 -0300
Subject: [PATCH] Fail early if process does not have permissions to write to
 log file (#2138)

* Fail early if process does not have permissions to write to log file

* Open file once on NewFilesystemLogWriter
---
 .../issue-1950-logging-filesystem-fail-early  |  1 +
 server/logging/filesystem.go                  | 71 ++++++++++---------
 server/logging/filesystem_test.go             | 23 ++++++
 3 files changed, 63 insertions(+), 32 deletions(-)
 create mode 100644 changes/issue-1950-logging-filesystem-fail-early

diff --git a/changes/issue-1950-logging-filesystem-fail-early b/changes/issue-1950-logging-filesystem-fail-early
new file mode 100644
index 0000000000..306f88436e
--- /dev/null
+++ b/changes/issue-1950-logging-filesystem-fail-early
@@ -0,0 +1 @@
+* Fail early if the process does not have permissions to write to the logging file.
diff --git a/server/logging/filesystem.go b/server/logging/filesystem.go
index ac82ade98b..5b9a7e98bf 100644
--- a/server/logging/filesystem.go
+++ b/server/logging/filesystem.go
@@ -24,35 +24,42 @@ type filesystemLogWriter struct {
 // NewFilesystemLogWriter creates a log file for osquery status/result logs.
 // The logFile can be rotated by sending a `SIGHUP` signal to Fleet if
 // enableRotation is true
+//
+// The enableCompression argument is only used when enableRotation is true.
 func NewFilesystemLogWriter(path string, appLogger log.Logger, enableRotation bool, enableCompression bool) (*filesystemLogWriter, error) {
-	if enableRotation {
-		// Use lumberjack logger that supports rotation
-		osquerydLogger := &lumberjack.Logger{
-			Filename:   path,
-			MaxSize:    500, // megabytes
-			MaxBackups: 3,
-			MaxAge:     28, //days
-			Compress:   enableCompression,
-		}
-		appLogger = log.With(appLogger, "component", "osqueryd-logger")
-		sig := make(chan os.Signal, 1)
-		signal.Notify(sig, syscall.SIGHUP)
-		go func() {
-			for {
-				<-sig //block on signal
-				if err := osquerydLogger.Rotate(); err != nil {
-					appLogger.Log("err", err)
-				}
-			}
-		}()
-		return &filesystemLogWriter{osquerydLogger}, nil
-	}
-	// no log rotation, use "raw" bufio implementation
-	writer, err := newRawLogWriter(path)
+	// Fail early if the process does not have the necessary
+	// permissions to open the file at path.
+	file, err := openFile(path)
 	if err != nil {
-		return nil, errors.Wrap(err, "create new raw logger")
+		return nil, errors.Wrap(err, "perm check")
 	}
-	return &filesystemLogWriter{writer}, nil
+	if !enableRotation {
+		// no log rotation, use "raw" bufio implementation
+		return &filesystemLogWriter{
+			writer: newRawLogWriter(file),
+		}, nil
+	}
+	// Use lumberjack logger that supports rotation
+	file.Close()
+	osquerydLogger := &lumberjack.Logger{
+		Filename:   path,
+		MaxSize:    500, // megabytes
+		MaxBackups: 3,
+		MaxAge:     28, //days
+		Compress:   enableCompression,
+	}
+	appLogger = log.With(appLogger, "component", "osqueryd-logger")
+	sig := make(chan os.Signal, 1)
+	signal.Notify(sig, syscall.SIGHUP)
+	go func() {
+		for {
+			<-sig //block on signal
+			if err := osquerydLogger.Rotate(); err != nil {
+				appLogger.Log("err", err)
+			}
+		}
+	}()
+	return &filesystemLogWriter{osquerydLogger}, nil
 }
 
 // If writer is based on bufio we want to flush after a batch of
@@ -85,13 +92,9 @@ type rawLogWriter struct {
 	mtx  sync.Mutex
 }
 
-func newRawLogWriter(path string) (*rawLogWriter, error) {
-	file, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
-	if err != nil {
-		return nil, err
-	}
+func newRawLogWriter(file *os.File) *rawLogWriter {
 	buff := bufio.NewWriter(file)
-	return &rawLogWriter{file: file, buff: buff}, nil
+	return &rawLogWriter{file: file, buff: buff}
 }
 
 // Write bytes to file
@@ -141,3 +144,7 @@ func (l *rawLogWriter) Close() error {
 
 	return nil
 }
+
+func openFile(path string) (*os.File, error) {
+	return os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0644)
+}
diff --git a/server/logging/filesystem_test.go b/server/logging/filesystem_test.go
index ae6328ec44..912e3e5542 100644
--- a/server/logging/filesystem_test.go
+++ b/server/logging/filesystem_test.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"crypto/rand"
 	"encoding/json"
+	stderrors "errors"
+	"io/fs"
 	"io/ioutil"
 	"os"
 	"path"
@@ -59,6 +61,27 @@ func TestFilesystemLogger(t *testing.T) {
 
 }
 
+// TestFilesystemLoggerPermission tests that NewFilesystemLogWriter fails
+// if the process does not have permissions to write to the provided path.
+func TestFilesystemLoggerPermission(t *testing.T) {
+	tempPath := t.TempDir()
+	require.NoError(t, os.Chmod(tempPath, 0000))
+	fileName := path.Join(tempPath, "filesystemLogWriter")
+	for _, tc := range []struct {
+		name     string
+		rotation bool
+	}{
+		{name: "with-rotation", rotation: true},
+		{name: "without-rotation", rotation: false},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := NewFilesystemLogWriter(fileName, log.NewNopLogger(), tc.rotation, false)
+			require.Error(t, err)
+			require.True(t, stderrors.Is(err, fs.ErrPermission), err)
+		})
+	}
+}
+
 func BenchmarkFilesystemLogger(b *testing.B) {
 	ctx := context.Background()
 	tempPath, err := ioutil.TempDir("", "test")