diff --git a/README.md b/README.md index 9b91748..bb18bcc 100644 --- a/README.md +++ b/README.md @@ -201,7 +201,10 @@ mode = "0755" # optional; default 0600 for files Runs at `vm create` time. Each entry copies `host` → `guest` onto the VM's work disk (mounted at `/root` in the guest). Guest paths must live under `~/` or `/root/...`. Default is no entries — add the -ones you want. +ones you want. Symlinks encountered while recursing into a synced +directory are skipped with a warning — they'd otherwise leak files +from outside the named tree (e.g. a symlink inside `~/.aws` pointing +to an unrelated credential dir). ## Advanced diff --git a/internal/daemon/vm_authsync.go b/internal/daemon/vm_authsync.go index 45488e0..af24a3e 100644 --- a/internal/daemon/vm_authsync.go +++ b/internal/daemon/vm_authsync.go @@ -220,7 +220,7 @@ func (s *WorkspaceService) runFileSync(ctx context.Context, vm *model.VMRecord) } if info.IsDir() { - if err := copyHostDir(ctx, runner, hostPath, target); err != nil { + if err := s.copyHostDir(ctx, *vm, runner, hostPath, target); err != nil { return fmt.Errorf("file_sync: copy directory %s → %s: %w", hostPath, target, err) } continue @@ -240,10 +240,14 @@ func (s *WorkspaceService) runFileSync(ctx context.Context, vm *model.VMRecord) // copyHostDir recursively copies hostDir into guestTarget using only // `mkdir` (for subdirs) and `install` (for files). Each file's source // permissions are preserved; ownership is forced to root:root via -// `install -o 0 -g 0`. Symlinks are followed (target content is -// copied as a regular file). Other special types (devices, FIFOs) -// are skipped silently. -func copyHostDir(ctx context.Context, runner system.CommandRunner, hostDir, guestTarget string) error { +// `install -o 0 -g 0`. Symlinks encountered during recursion are +// SKIPPED with a warning — `os.Lstat` tells us the entry itself is a +// link without resolving it, so a symlink inside ~/.aws that points +// at ~/secrets can't leak out of the tree the user named. Other +// special types (devices, FIFOs) are skipped silently. Top-level +// host paths go through `os.Stat` back in runFileSync and still +// follow, since the user explicitly named that path. +func (s *WorkspaceService) copyHostDir(ctx context.Context, vm model.VMRecord, runner system.CommandRunner, hostDir, guestTarget string) error { if _, err := runner.RunSudo(ctx, "mkdir", "-p", guestTarget); err != nil { return err } @@ -255,13 +259,15 @@ func copyHostDir(ctx context.Context, runner system.CommandRunner, hostDir, gues hostChild := filepath.Join(hostDir, entry.Name()) guestChild := filepath.Join(guestTarget, entry.Name()) - info, err := os.Stat(hostChild) + info, err := os.Lstat(hostChild) if err != nil { return err } switch { + case info.Mode()&os.ModeSymlink != 0: + s.warnFileSyncSymlinkSkipped(vm, hostChild) case info.IsDir(): - if err := copyHostDir(ctx, runner, hostChild, guestChild); err != nil { + if err := s.copyHostDir(ctx, vm, runner, hostChild, guestChild); err != nil { return err } case info.Mode().IsRegular(): @@ -372,6 +378,17 @@ func (s *WorkspaceService) warnFileSyncSkipped(vm model.VMRecord, hostPath strin s.logger.Warn("file_sync skipped", append(vmLogAttrs(vm), "host_path", hostPath, "error", err.Error())...) } +// warnFileSyncSymlinkSkipped surfaces a skipped nested symlink to the +// user through the daemon log. Skipping is deliberate — see +// copyHostDir's docstring — but invisible skips would hide a +// "why did my file not show up in the guest?" debugging trail. +func (s *WorkspaceService) warnFileSyncSymlinkSkipped(vm model.VMRecord, hostPath string) { + if s.logger == nil { + return + } + s.logger.Warn("file_sync skipped symlink (would escape the requested tree)", append(vmLogAttrs(vm), "host_path", hostPath)...) +} + func (s *WorkspaceService) warnGitIdentitySyncSkipped(vm model.VMRecord, source string, err error) { if s.logger == nil || err == nil { return diff --git a/internal/daemon/vm_test.go b/internal/daemon/vm_test.go index e131e94..05b4713 100644 --- a/internal/daemon/vm_test.go +++ b/internal/daemon/vm_test.go @@ -1177,6 +1177,105 @@ func TestRunFileSyncCopiesDirectoryRecursively(t *testing.T) { } } +// TestRunFileSyncSkipsNestedSymlinks pins the anti-sprawl contract: +// a symlink INSIDE a synced directory is not followed, even if the +// target holds real files. Without this, a user syncing ~/.aws with +// a ~/.aws/session -> ~/other-creds symlink would copy the unrelated +// creds into the guest. Top-level entries (the path the user +// literally named) still follow, because they explicitly asked for +// that path. +func TestRunFileSyncSkipsNestedSymlinks(t *testing.T) { + homeDir := t.TempDir() + t.Setenv("HOME", homeDir) + + // Target the user DID NOT name — lives outside the synced tree. + outsideDir := filepath.Join(homeDir, "other-creds") + if err := os.MkdirAll(outsideDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(outsideDir, "leaked.txt"), []byte("must-not-escape"), 0o600); err != nil { + t.Fatal(err) + } + + // The synced directory. + srcDir := filepath.Join(homeDir, ".aws") + if err := os.MkdirAll(srcDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(srcDir, "credentials"), []byte("access"), 0o600); err != nil { + t.Fatal(err) + } + // File symlink inside .aws pointing OUT of the tree. + if err := os.Symlink(filepath.Join(outsideDir, "leaked.txt"), filepath.Join(srcDir, "session")); err != nil { + t.Skipf("symlink unsupported on this filesystem: %v", err) + } + // Directory symlink inside .aws pointing OUT of the tree — must + // not be recursed into. + if err := os.Symlink(outsideDir, filepath.Join(srcDir, "linked-dir")); err != nil { + t.Fatal(err) + } + + var buf bytes.Buffer + logger, _, err := newDaemonLogger(&buf, "info") + if err != nil { + t.Fatal(err) + } + + workDisk := t.TempDir() + d := &Daemon{ + runner: &filesystemRunner{t: t}, + logger: logger, + config: model.DaemonConfig{ + FileSync: []model.FileSyncEntry{ + {Host: "~/.aws", Guest: "~/.aws"}, + }, + }, + } + wireServices(d) + vm := testVM("sync-symlink", "image", "172.16.0.76") + vm.Runtime.WorkDiskPath = workDisk + if err := d.ws.runFileSync(context.Background(), &vm); err != nil { + t.Fatalf("runFileSync: %v", err) + } + + // The real file inside the tree must copy. + creds, err := os.ReadFile(filepath.Join(workDisk, ".aws", "credentials")) + if err != nil { + t.Fatalf("credentials not copied: %v", err) + } + if string(creds) != "access" { + t.Fatalf("credentials = %q, want access", creds) + } + + // Neither the file symlink nor anything reached through the + // directory symlink should have been materialised in the guest + // path. + for _, shouldNotExist := range []string{ + filepath.Join(workDisk, ".aws", "session"), + filepath.Join(workDisk, ".aws", "linked-dir"), + filepath.Join(workDisk, ".aws", "linked-dir", "leaked.txt"), + } { + if _, err := os.Stat(shouldNotExist); !os.IsNotExist(err) { + t.Fatalf("symlinked path %s was materialised in guest tree (stat err = %v); secret leakage path open", shouldNotExist, err) + } + } + + // Each skipped symlink must be warned. + entries := parseLogEntries(t, buf.Bytes()) + for _, want := range []string{ + filepath.Join(srcDir, "session"), + filepath.Join(srcDir, "linked-dir"), + } { + if !hasLogEntry(entries, map[string]string{ + "msg": "file_sync skipped symlink (would escape the requested tree)", + "vm_name": vm.Name, + "host_path": want, + }) { + t.Fatalf("expected warn log for skipped symlink %s; got %v", want, entries) + } + } +} + func TestCreateVMRejectsNonPositiveCPUAndMemory(t *testing.T) { d := &Daemon{} wireServices(d)