task 261: harden server reads with OpenRoot

8 files changed, 334 insertions, 38 deletions

diff --git a/internal/io/fs/catfile.go b/internal/io/fs/catfile.go
index 1f35a95..ac42fc0 100644
--- a/internal/io/fs/catfile.go
+++ b/internal/io/fs/catfile.go
@@ -21,3 +21,12 @@ func NewCatFile(filePath string, globID string, serverMessages chan<- string,
 		},
 	}
 }
+
+// NewValidatedCatFile returns a new file catter backed by a rooted open target.
+func NewValidatedCatFile(filePath string, target ValidatedReadTarget, globID string,
+	serverMessages chan<- string, maxLineLength int) CatFile {
+
+	cat := NewCatFile(filePath, globID, serverMessages, maxLineLength)
+	cat.readFile.validatedTarget = &target
+	return cat
+}
diff --git a/internal/io/fs/readfile.go b/internal/io/fs/readfile.go
index 0ec2eca..d305c4d 100644
--- a/internal/io/fs/readfile.go
+++ b/internal/io/fs/readfile.go
@@ -38,6 +38,8 @@ type readFile struct {
 	stats
 	// Path of log file to tail.
 	filePath string
+	// Rooted target used for validated server-side re-opens.
+	validatedTarget *ValidatedReadTarget
 	// The glob identifier of the file.
 	globID string
 	// Channel to send a server message to the dtail client
@@ -134,7 +136,7 @@ func (f *readFile) makeReader() (*bufio.Reader, *os.File, io.Closer, error) {
 }
 
 func (f *readFile) makeFileReader() (reader *bufio.Reader, fd *os.File, decompressor io.Closer, err error) {
-	if fd, err = os.Open(f.filePath); err != nil {
+	if fd, err = f.openFile(); err != nil {
 		return
 	}
 
@@ -148,6 +150,13 @@ func (f *readFile) makeFileReader() (reader *bufio.Reader, fd *os.File, decompre
 	return
 }
 
+func (f *readFile) openFile() (*os.File, error) {
+	if f.validatedTarget != nil {
+		return f.validatedTarget.Open()
+	}
+	return os.Open(f.filePath)
+}
+
 func (f *readFile) makePipeReader() (*bufio.Reader, *os.File, io.Closer, error) {
 	return bufio.NewReader(os.Stdin), nil, nil, nil
 }
@@ -276,7 +285,7 @@ func (f *readFile) truncated(fd *os.File) (bool, error) {
 		return true, err
 	}
 	// Can not open file at original path.
-	pathFd, err := os.Open(f.filePath)
+	pathFd, err := f.openFile()
 	if err != nil {
 		return true, err
 	}
diff --git a/internal/io/fs/tailfile.go b/internal/io/fs/tailfile.go
index b2e9910..a5f172d 100644
--- a/internal/io/fs/tailfile.go
+++ b/internal/io/fs/tailfile.go
@@ -21,3 +21,12 @@ func NewTailFile(filePath string, globID string, serverMessages chan<- string,
 		},
 	}
 }
+
+// NewValidatedTailFile returns a new file tailer backed by a rooted open target.
+func NewValidatedTailFile(filePath string, target ValidatedReadTarget, globID string,
+	serverMessages chan<- string, maxLineLength int) TailFile {
+
+	tail := NewTailFile(filePath, globID, serverMessages, maxLineLength)
+	tail.readFile.validatedTarget = &target
+	return tail
+}
diff --git a/internal/io/fs/validatedreadtarget.go b/internal/io/fs/validatedreadtarget.go
new file mode 100644
index 0000000..83b1404
--- /dev/null
+++ b/internal/io/fs/validatedreadtarget.go
@@ -0,0 +1,90 @@
+package fs
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+)
+
+// ValidatedReadTarget stores a resolved regular file path for rooted re-opens.
+type ValidatedReadTarget struct {
+	resolvedPath string
+	rootDir      string
+	rootName     string
+}
+
+// NewValidatedReadTarget returns a rooted target for a resolved regular file.
+func NewValidatedReadTarget(resolvedPath string) (ValidatedReadTarget, error) {
+	cleanedPath := filepath.Clean(resolvedPath)
+	if !filepath.IsAbs(cleanedPath) {
+		return ValidatedReadTarget{}, fmt.Errorf("validated read target requires absolute path: %s", cleanedPath)
+	}
+
+	info, err := os.Lstat(cleanedPath)
+	if err != nil {
+		return ValidatedReadTarget{}, fmt.Errorf("lstat validated read target %s: %w", cleanedPath, err)
+	}
+	if !info.Mode().IsRegular() {
+		return ValidatedReadTarget{}, fmt.Errorf("validated read target must be a regular file: %s", cleanedPath)
+	}
+
+	return ValidatedReadTarget{
+		resolvedPath: cleanedPath,
+		rootDir:      filepath.Dir(cleanedPath),
+		rootName:     filepath.Base(cleanedPath),
+	}, nil
+}
+
+// Open re-opens the validated file beneath its resolved parent directory.
+func (t ValidatedReadTarget) Open() (*os.File, error) {
+	root, err := os.OpenRoot(t.rootDir)
+	if err != nil {
+		return nil, fmt.Errorf("open root for %s: %w", t.resolvedPath, err)
+	}
+	defer root.Close()
+
+	if err := t.validateEntry(root); err != nil {
+		return nil, err
+	}
+
+	fd, err := root.Open(t.rootName)
+	if err != nil {
+		return nil, fmt.Errorf("open rooted file %s: %w", t.resolvedPath, err)
+	}
+
+	if err := validateOpenedFile(fd, t.resolvedPath); err != nil {
+		fd.Close()
+		return nil, err
+	}
+	if err := t.validateEntry(root); err != nil {
+		fd.Close()
+		return nil, err
+	}
+
+	return fd, nil
+}
+
+func (t ValidatedReadTarget) validateEntry(root *os.Root) error {
+	info, err := root.Lstat(t.rootName)
+	if err != nil {
+		return fmt.Errorf("lstat rooted file %s: %w", t.resolvedPath, err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		return fmt.Errorf("rooted file changed to symlink: %s", t.resolvedPath)
+	}
+	if !info.Mode().IsRegular() {
+		return fmt.Errorf("rooted file changed to non-regular file: %s", t.resolvedPath)
+	}
+	return nil
+}
+
+func validateOpenedFile(fd *os.File, resolvedPath string) error {
+	info, err := fd.Stat()
+	if err != nil {
+		return fmt.Errorf("stat opened file %s: %w", resolvedPath, err)
+	}
+	if !info.Mode().IsRegular() {
+		return fmt.Errorf("opened file is not regular: %s", resolvedPath)
+	}
+	return nil
+}
diff --git a/internal/io/fs/validatedreadtarget_test.go b/internal/io/fs/validatedreadtarget_test.go
new file mode 100644
index 0000000..b256f20
--- /dev/null
+++ b/internal/io/fs/validatedreadtarget_test.go
@@ -0,0 +1,166 @@
+package fs
+
+import (
+	"context"
+	"io"
+	"os"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/mimecast/dtail/internal/io/dlog"
+	"github.com/mimecast/dtail/internal/lcontext"
+	"github.com/mimecast/dtail/internal/regex"
+)
+
+func TestValidatedCatFileStartWithProcessorOptimizedReadsAllLines(t *testing.T) {
+	resetCommonLogger(t)
+
+	filePath := writeProcessorTestFile(t, "alpha\nbeta\n")
+	target := mustValidatedReadTarget(t, filePath)
+	re := regex.NewNoop()
+
+	cat := NewValidatedCatFile(filePath, target, "glob-id", make(chan string, 1), defaultMaxLineLength)
+	processor := &captureProcessor{}
+
+	if err := cat.readFile.StartWithProcessorOptimized(
+		context.Background(),
+		lcontext.LContext{},
+		processor,
+		re,
+	); err != nil {
+		t.Fatalf("validated optimized reader start failed: %v", err)
+	}
+
+	want := []string{"alpha\n", "beta\n"}
+	if !reflect.DeepEqual(processor.lines, want) {
+		t.Fatalf("unexpected processed lines: got=%v want=%v", processor.lines, want)
+	}
+}
+
+func TestValidatedReadTargetOpenRejectsEscapingSymlinkSwap(t *testing.T) {
+	resetCommonLogger(t)
+
+	baseDir := t.TempDir()
+	rootDir := filepath.Join(baseDir, "root")
+	outsideDir := filepath.Join(baseDir, "outside")
+	if err := os.MkdirAll(rootDir, 0755); err != nil {
+		t.Fatalf("mkdir root dir: %v", err)
+	}
+	if err := os.MkdirAll(outsideDir, 0755); err != nil {
+		t.Fatalf("mkdir outside dir: %v", err)
+	}
+
+	filePath := filepath.Join(rootDir, "app.log")
+	if err := os.WriteFile(filePath, []byte("alpha\n"), 0600); err != nil {
+		t.Fatalf("write app log: %v", err)
+	}
+	escapePath := filepath.Join(outsideDir, "secret.log")
+	if err := os.WriteFile(escapePath, []byte("secret\n"), 0600); err != nil {
+		t.Fatalf("write secret log: %v", err)
+	}
+
+	target := mustValidatedReadTarget(t, filePath)
+
+	if err := os.Remove(filePath); err != nil {
+		t.Fatalf("remove app log: %v", err)
+	}
+	relativeEscape, err := filepath.Rel(rootDir, escapePath)
+	if err != nil {
+		t.Fatalf("relative escape path: %v", err)
+	}
+	if err := os.Symlink(relativeEscape, filePath); err != nil {
+		t.Fatalf("symlink escape path: %v", err)
+	}
+
+	if _, err := target.Open(); err == nil {
+		t.Fatal("expected rooted open to reject escaping symlink swap")
+	}
+}
+
+func TestValidatedReadTargetOpenRejectsSameRootSymlinkSwap(t *testing.T) {
+	resetCommonLogger(t)
+
+	baseDir := t.TempDir()
+	filePath := filepath.Join(baseDir, "app.log")
+	if err := os.WriteFile(filePath, []byte("alpha\n"), 0600); err != nil {
+		t.Fatalf("write app log: %v", err)
+	}
+	otherPath := filepath.Join(baseDir, "other.log")
+	if err := os.WriteFile(otherPath, []byte("other\n"), 0600); err != nil {
+		t.Fatalf("write other log: %v", err)
+	}
+
+	target := mustValidatedReadTarget(t, filePath)
+
+	if err := os.Remove(filePath); err != nil {
+		t.Fatalf("remove app log: %v", err)
+	}
+	if err := os.Symlink(filepath.Base(otherPath), filePath); err != nil {
+		t.Fatalf("symlink other log: %v", err)
+	}
+
+	_, err := target.Open()
+	if err == nil {
+		t.Fatal("expected rooted open to reject same-root symlink swap")
+	}
+	if !strings.Contains(err.Error(), "symlink") {
+		t.Fatalf("expected symlink rejection, got %v", err)
+	}
+}
+
+func TestValidatedTailFileTruncatedReopenDetectsTruncation(t *testing.T) {
+	resetCommonLogger(t)
+
+	filePath := writeProcessorTestFile(t, "alpha\nbeta\n")
+	target := mustValidatedReadTarget(t, filePath)
+
+	tail := NewValidatedTailFile(filePath, target, "glob-id", make(chan string, 1), defaultMaxLineLength)
+	fd, err := target.Open()
+	if err != nil {
+		t.Fatalf("open validated target: %v", err)
+	}
+	defer fd.Close()
+
+	if _, err := fd.Seek(0, io.SeekEnd); err != nil {
+		t.Fatalf("seek end: %v", err)
+	}
+	if err := os.Truncate(filePath, 1); err != nil {
+		t.Fatalf("truncate file: %v", err)
+	}
+
+	isTruncated, err := tail.readFile.truncated(fd)
+	if !isTruncated {
+		t.Fatal("expected truncation to be detected")
+	}
+	if err == nil || !strings.Contains(err.Error(), "truncated") {
+		t.Fatalf("expected truncation error, got %v", err)
+	}
+}
+
+func mustValidatedReadTarget(t *testing.T, path string) ValidatedReadTarget {
+	t.Helper()
+
+	absolutePath, err := filepath.Abs(path)
+	if err != nil {
+		t.Fatalf("abs path: %v", err)
+	}
+
+	target, err := NewValidatedReadTarget(absolutePath)
+	if err != nil {
+		t.Fatalf("create validated target: %v", err)
+	}
+
+	return target
+}
+
+func resetCommonLogger(t *testing.T) {
+	t.Helper()
+
+	originalLogger := dlog.Common
+	dlog.Common = &dlog.DLog{}
+	t.Cleanup(func() {
+		dlog.Common = originalLogger
+	})
+}
diff --git a/internal/server/handlers/readcommand.go b/internal/server/handlers/readcommand.go
index d4c9c30..7f17b14 100644
--- a/internal/server/handlers/readcommand.go
+++ b/internal/server/handlers/readcommand.go
@@ -71,7 +71,7 @@ func (r *readCommand) Start(ctx context.Context, ltx lcontext.LContext,
 	if isPipe {
 		dlog.Server.Debug("Reading data from stdin pipe")
 		// Empty file path and globID "-" represents reading from the stdin pipe.
-		r.read(ctx, ltx, "", "-", re)
+		r.read(ctx, ltx, "", nil, "-", re)
 		return
 	}
 
@@ -200,17 +200,18 @@ func (r *readCommand) readFileIfPermissions(ctx context.Context, ltx lcontext.LC
 	}()
 
 	globID := r.makeGlobID(path, glob)
-	if !r.server.CanReadFile(path) {
+	target, ok := r.server.PrepareReadTarget(path)
+	if !ok {
 		dlog.Server.Error(r.server.LogContext(), "No permission to read file", path, globID)
 		r.sendServerMessage(dlog.Server.Warn(r.server.LogContext(),
 			"Unable to read file(s), check server logs"))
 		return
 	}
-	r.read(ctx, ltx, path, globID, re)
+	r.read(ctx, ltx, path, &target, globID, re)
 }
 
 func (r *readCommand) read(ctx context.Context, ltx lcontext.LContext,
-	path, globID string, re regex.Regex) {
+	path string, target *fs.ValidatedReadTarget, globID string, re regex.Regex) {
 
 	dlog.Server.Info(r.server.LogContext(), "Start reading", path, globID)
 	r.logRegexMode(re)
@@ -222,14 +223,24 @@ func (r *readCommand) read(ctx context.Context, ltx lcontext.LContext,
 
 	switch r.mode {
 	case omode.GrepClient, omode.CatClient:
-		catFile := fs.NewCatFile(path, globID, serverMessages, r.server.MaxLineLength())
-		reader = &catFile
+		if target != nil {
+			catFile := fs.NewValidatedCatFile(path, *target, globID, serverMessages, r.server.MaxLineLength())
+			reader = &catFile
+		} else {
+			catFile := fs.NewCatFile(path, globID, serverMessages, r.server.MaxLineLength())
+			reader = &catFile
+		}
 		limiter = r.server.CatLimiter()
 	case omode.TailClient:
 		fallthrough
 	default:
-		tailFile := fs.NewTailFile(path, globID, serverMessages, r.server.MaxLineLength())
-		reader = &tailFile
+		if target != nil {
+			tailFile := fs.NewValidatedTailFile(path, *target, globID, serverMessages, r.server.MaxLineLength())
+			reader = &tailFile
+		} else {
+			tailFile := fs.NewTailFile(path, globID, serverMessages, r.server.MaxLineLength())
+			reader = &tailFile
+		}
 		limiter = r.server.TailLimiter()
 	}
 
diff --git a/internal/server/handlers/readcommand_server.go b/internal/server/handlers/readcommand_server.go
index 8c3cb96..0f98a58 100644
--- a/internal/server/handlers/readcommand_server.go
+++ b/internal/server/handlers/readcommand_server.go
@@ -4,6 +4,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/mimecast/dtail/internal/io/fs"
 	"github.com/mimecast/dtail/internal/io/line"
 	"github.com/mimecast/dtail/internal/mapr/server"
 )
@@ -13,7 +14,7 @@ type readCommandContext interface {
 }
 
 type readCommandFiles interface {
-	CanReadFile(path string) bool
+	PrepareReadTarget(path string) (fs.ValidatedReadTarget, bool)
 	CatLimiter() chan struct{}
 	TailLimiter() chan struct{}
 }
@@ -87,9 +88,9 @@ func (h *ServerHandler) SendServerMessage(message string) {
 	h.sendln(h.serverMessages, message)
 }
 
-// CanReadFile reports whether the current user can read the given path.
-func (h *ServerHandler) CanReadFile(path string) bool {
-	return h.user.HasFilePermission(path, "readfiles")
+// PrepareReadTarget validates the current user's access to the given path.
+func (h *ServerHandler) PrepareReadTarget(path string) (fs.ValidatedReadTarget, bool) {
+	return h.user.ValidateReadTarget(path, "readfiles")
 }
 
 // ServerMessagesChannel returns the server message channel.
diff --git a/internal/user/server/user.go b/internal/user/server/user.go
index 332ea96..feed385 100644
--- a/internal/user/server/user.go
+++ b/internal/user/server/user.go
@@ -2,18 +2,16 @@ package server
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
 	"regexp"
 	"strings"
 
 	"github.com/mimecast/dtail/internal/config"
 	"github.com/mimecast/dtail/internal/io/dlog"
+	"github.com/mimecast/dtail/internal/io/fs"
 	"github.com/mimecast/dtail/internal/io/fs/permissions"
 )
 
-const maxLinkDepth int = 100
-
 // User represents an end-user which connected to the server via the DTail client.
 type User struct {
 	// The user name.
@@ -52,27 +50,26 @@ func (u *User) String() string {
 }
 
 // HasFilePermission is used to determine whether user is allowed to read a file.
-func (u *User) HasFilePermission(filePath, permissionType string) (hasPermission bool) {
-	dlog.Server.Debug(u, filePath, permissionType, "Checking config permissions")
-	if u.Name == config.ScheduleUser || u.Name == config.ContinuousUser {
-		// Background user has same permissions as dtail process itself.
-		return true
-	}
+func (u *User) HasFilePermission(filePath, permissionType string) bool {
+	_, hasPermission := u.ValidateReadTarget(filePath, permissionType)
+	return hasPermission
+}
 
+// ValidateReadTarget resolves and authorizes a file path for server-side reads.
+func (u *User) ValidateReadTarget(filePath, permissionType string) (fs.ValidatedReadTarget, bool) {
+	dlog.Server.Debug(u, filePath, permissionType, "Checking config permissions")
 	cleanPath, err := filepath.EvalSymlinks(filePath)
 	if err != nil {
 		dlog.Server.Error(u, filePath, permissionType,
 			"Unable to evaluate symlinks", err)
-		hasPermission = false
-		return
+		return fs.ValidatedReadTarget{}, false
 	}
 
 	cleanPath, err = filepath.Abs(cleanPath)
 	if err != nil {
 		dlog.Server.Error(u, cleanPath, permissionType,
 			"Unable to make file path absolute", err)
-		hasPermission = false
-		return
+		return fs.ValidatedReadTarget{}, false
 	}
 
 	if cleanPath != filePath {
@@ -80,11 +77,23 @@ func (u *User) HasFilePermission(filePath, permissionType string) (hasPermission
 			"Calculated new clean path from original file path (possibly symlink)")
 	}
 
-	hasPermission, err = u.hasFilePermission(cleanPath, permissionType)
+	if u.Name != config.ScheduleUser && u.Name != config.ContinuousUser {
+		hasPermission, permissionErr := u.hasFilePermission(cleanPath, permissionType)
+		if permissionErr != nil {
+			dlog.Server.Warn(u, cleanPath, permissionErr)
+		}
+		if !hasPermission {
+			return fs.ValidatedReadTarget{}, false
+		}
+	}
+
+	target, err := fs.NewValidatedReadTarget(cleanPath)
 	if err != nil {
-		dlog.Server.Warn(u, cleanPath, err)
+		dlog.Server.Warn(u, cleanPath, permissionType, "Unable to validate read target", err)
+		return fs.ValidatedReadTarget{}, false
 	}
-	return
+
+	return target, true
 }
 
 func (u *User) hasFilePermission(cleanPath, permissionType string) (bool, error) {
@@ -95,14 +104,6 @@ func (u *User) hasFilePermission(cleanPath, permissionType string) (bool, error)
 	dlog.Server.Info(u, cleanPath, permissionType,
 		"User with OS file system permissions to path")
 
-	// Only allow to follow regular files or symlinks.
-	info, err := os.Lstat(cleanPath)
-	if err != nil {
-		return false, fmt.Errorf("Unable to determine file type: %w", err)
-	}
-	if !info.Mode().IsRegular() {
-		return false, fmt.Errorf("Can only open regular files or follow symlinks")
-	}
 	hasPermission, err := u.iteratePaths(cleanPath, permissionType)
 	if err != nil {
 		return false, err