Enforce chroot-like security in fdo.upload owner FSIM using os.Root

fsim/fs_other.go

@@ -0,0 +1,8 @@
+//go:build aix || dragonfly || illumos || js || plan9 || solaris
+// +build aix dragonfly illumos js plan9 solaris
+
+package fsim
+
+func sameFilesystem(_, _ string) (bool, error) {
+	return false, nil
+}

fsim/fs_unix.go

@@ -0,0 +1,20 @@
+//go:build darwin || freebsd || linux || netbsd || openbsd || android || darwin || freebsd || ios || linux || netbsd || openbsd || wasip1
+// +build darwin freebsd linux netbsd openbsd android darwin freebsd ios linux netbsd openbsd wasip1
+
+package fsim
+
+import "syscall"
+
+// sameFilesystem checks if two paths are on the same filesystem by comparing device IDs
+func sameFilesystem(path1, path2 string) (bool, error) {
+	var stat1, stat2 syscall.Stat_t
+
+	if err := syscall.Stat(path1, &stat1); err != nil {
+		return false, err
+	}
+	if err := syscall.Stat(path2, &stat2); err != nil {
+		return false, err
+	}
+
+	return stat1.Dev == stat2.Dev, nil
+}

fsim/fs_windows.go

@@ -0,0 +1,11 @@
+//go:build windows
+
+package fsim
+
+import "path/filepath"
+
+func sameFilesystem(path1, path2 string) (bool, error) {
+	vol1 := filepath.VolumeName(path1)
+	vol2 := filepath.VolumeName(path2)
+	return vol1 != "" && vol1 == vol2, nil
+}

fsim/fsim_test.go

@@ -35,10 +35,10 @@ import (
 )
 
 func TestClientWithDataModules(t *testing.T) {
-	if err := os.MkdirAll("testdata/downloads", 0755); err != nil {
+	if err := os.MkdirAll("testdata/downloads", 0o755); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.MkdirAll("testdata/uploads", 0755); err != nil {
+	if err := os.MkdirAll("testdata/uploads", 0o755); err != nil {
 		t.Fatal(err)
 	}
 
@@ -75,7 +75,7 @@ func TestClientWithDataModules(t *testing.T) {
 			"fdo.upload": &fsim.Upload{FS: fstest.MapFS{
 				"bigfile.test": &fstest.MapFile{
 					Data: data,
-					Mode: 0777,
+					Mode: 0o777,
 				},
 			}},
 			"fdo.wget": &fsim.Wget{
@@ -99,8 +99,9 @@ func TestClientWithDataModules(t *testing.T) {
 				}
 
 				if !yield("fdo.upload", &fsim.UploadRequest{
-					Dir:  "testdata/uploads",
-					Name: "bigfile.test",
+					Dir:       "testdata/uploads",
+					Name:      "bigfile.test",
+					Overwrite: true,
 					CreateTemp: func() (*os.File, error) {
 						return os.CreateTemp("testdata", "fdo.upload_*")
 					},
@@ -171,25 +172,30 @@ func TestClientWithMockDownloadOwner(t *testing.T) {
 					return false, false, err
 				}
 				if err := producer.WriteChunk("sha-384",
-					[]byte{0x58, 0x30, 0x9c, 0xa3, 0x46, 0xe2, 0xd3,
+					[]byte{
+						0x58, 0x30, 0x9c, 0xa3, 0x46, 0xe2, 0xd3,
 						0x47, 0x94, 0x3f, 0xa6, 0xfe, 0x18, 0xb5, 0x33, 0x23, 0x76,
 						0xa8, 0x28, 0x1a, 0xae, 0x7f, 0x92, 0x3c, 0x82, 0x37, 0xd3,
 						0x83, 0x70, 0xcb, 0x78, 0xdf, 0x41, 0x7d, 0x41, 0x4e, 0x0f,
 						0x38, 0x04, 0xfb, 0x89, 0x97, 0x00, 0x0e, 0x79, 0xcb, 0xd5,
-						0xb7, 0xbe, 0xb4}); err != nil {
+						0xb7, 0xbe, 0xb4,
+					}); err != nil {
 					return false, false, err
 				}
 				if err := producer.WriteChunk("length",
 					[]byte{0x18, 0x1c}); err != nil {
 					return false, false, err
 				}
 				if err := producer.WriteChunk("name",
-					[]byte{0x67, 0x6e, 0x65, 0x77, 0x66, 0x69, 0x6c,
-						0x65}); err != nil {
+					[]byte{
+						0x67, 0x6e, 0x65, 0x77, 0x66, 0x69, 0x6c,
+						0x65,
+					}); err != nil {
 					return false, false, err
 				}
 				if err := producer.WriteChunk("data",
-					[]byte{0x58, 0x1c, 0x54, 0x68, 0x69, 0x73, 0x20, 0x69,
+					[]byte{
+						0x58, 0x1c, 0x54, 0x68, 0x69, 0x73, 0x20, 0x69,
 						0x73, 0x20, 0x61, 0x20, 0x6e, 0x65, 0x77, 0x20, 0x66, 0x69, 0x6c,
 						0x65, 0x2c, 0x20, 0x66, 0x6f, 0x72, 0x20, 0x53, 0x56, 0x49, 0x0a,
 					}); err != nil {

fsim/upload_owner.go

@@ -11,8 +11,10 @@ import (
 	"fmt"
 	"hash"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	"github.com/fido-device-onboard/go-fdo/cbor"
@@ -30,13 +32,17 @@ type UploadRequest struct {
 	// Name to use in upload request
 	Name string
 
-	// Optional name to use on local filesystem
-	Rename string
+	// Rename func optionally overrides the behavior of how the module saves
+	// the device file to the local filesystem
+	Rename func(string) string
 
 	// CreateTemp optionally overrides the behavior of how the module creates a
 	// temporary file to download to.
 	CreateTemp func() (*os.File, error)
 
+	// Ovewrite makes the module to ovewrite the local file if it already exist
+	Overwrite bool
+
 	// internal state
 	requested bool
 	length    int64
@@ -157,13 +163,84 @@ func (u *UploadRequest) finalize() (blockPeer, moduleDone bool, _ error) {
 	if err := u.temp.Close(); err != nil {
 		return false, false, fmt.Errorf("error closing temp file for upload %q: %w", u.Name, err)
 	}
-	// TODO: Enforce chroot-like security
-	if u.Rename == "" {
-		u.Rename = filepath.Base(u.Name)
+
+	// Enforce chroot-like security using os.OpenRoot
+	// Create a rooted filesystem to prevent path traversal attacks
+	rootDir, err := os.OpenRoot(u.Dir)
+	if err != nil {
+		return false, false, fmt.Errorf("error creating '%q' as root dir for uploads: %w", u.Dir, err)
+	}
+	defer rootDir.Close()
+
+	rootDirName := rootDir.Name()
+	tempFileName := u.temp.Name()
+
+	defer os.Remove(tempFileName)
+
+	// Check if temp file and destination directory are on the same filesystem
+	samefs, err := sameFilesystem(tempFileName, rootDirName)
+	if err != nil {
+		return false, false, fmt.Errorf("error checking filesystem for temp file and destination: %w", err)
 	}
-	oldpath, newpath := u.temp.Name(), filepath.Join(u.Dir, u.Rename)
-	if err := os.Rename(oldpath, newpath); err != nil {
-		return false, false, fmt.Errorf("error renaming temp file %q to %q: %w", oldpath, newpath, err)
+
+	if u.Rename == nil {
+		u.Rename = func(name string) string {
+			return strings.TrimLeft(name, "/")
+		}
+	}
+
+	// Construct the actual destination filename (relative to u.Dir)
+	// Clean the path to prevent traversal attacks with ../
+	dstFileName := filepath.Clean(u.Rename(u.Name))
+
+	// Use rootDir to validate the destination path doesn't escape
+	var dstFileInfo fs.FileInfo
+	dstFileInfo, err = rootDir.Stat(dstFileName)
+	// The file exists and overwrite is false
+	if err == nil && !u.Overwrite {
+		return false, false, fmt.Errorf("'%s' already exists", dstFileInfo.Name())
+	}
+	// The was an unexpected error
+	if err != nil && !errors.Is(err, fs.ErrNotExist) {
+		return false, false, err
+	}
+
+	// Create parent directories if needed
+	if err = rootDir.MkdirAll(filepath.Dir(dstFileName), 0o755); err != nil {
+		return false, false, fmt.Errorf("error creating parent directories for %q: %w", dstFileName, err)
+	}
+
+	// Construct the full destination path for operations outside rootDir
+	dstFilePath := filepath.Join(u.Dir, dstFileName)
+
+	// Same filesystem - we can use rename for efficiency
+	// Use os.Rename since temp file is outside the rooted directory
+	if samefs {
+		if err = os.Rename(tempFileName, dstFilePath); err != nil {
+			return false, false, fmt.Errorf("error renaming temp file %q to %q: %w", tempFileName, dstFilePath, err)
+		}
+		return false, true, nil
+	}
+
+	// Different filesystems - fall back to copy
+	// Open the temp file for reading
+	src, err := os.Open(tempFileName)
+	if err != nil {
+		return false, false, fmt.Errorf("error opening temp file %q: %w", tempFileName, err)
+	}
+	defer src.Close()
+
+	// Create the destination file within the rooted filesystem
+	// This ensures u.Rename cannot escape u.Dir even with path traversal
+	dst, err := rootDir.Create(dstFileName)
+	if err != nil {
+		return false, false, fmt.Errorf("error creating destination file %q: %w", dstFileName, err)
+	}
+	defer dst.Close()
+
+	// Copy the data
+	if _, err = io.Copy(dst, src); err != nil {
+		return false, false, fmt.Errorf("error copying to destination file %q: %w", dstFileName, err)
 	}
 	return false, true, nil
 }