Conversation
This was referenced Sep 23, 2026
Member
Originally posted by @AkihiroSuda in lima#5529 |
The builtin driver used sftp.NewServer, which serves the whole host file system to the guest. It now uses sftp.NewRequestServer with handlers rooted at LocalPath: reads go through os.Root, and writes open each parent directory without following any symlink (O_NOFOLLOW on Linux and macOS, OBJ_DONT_REPARSE on Windows). A path outside the root is denied. Setting the mode fails on a symlink, on macOS as on Linux. On Linux kernels without fchmodat2 (< 6.6), it goes through /proc/self/fd of an O_PATH fd, as glibc does, so it needs no read permission. ReadonlyNames makes a path read-only when any of its components matches one of these names, compared case-insensitively and ignoring the code points that HFS+ ignores, as git does. For example, [".git"] keeps the working tree writable while hooks and config stay read-only. Setting ReadonlyNames selects the builtin driver in auto mode, and fails with the OpenSSH driver. On Windows, a name that passes this check may still refer to a read-only entry through its 8.3 short name (CVE-2019-1353), so every handle that a write goes through is also checked by its normalized name, in which the file system has expanded short names. Writes to names with a colon (NTFS streams, CVE-2019-1352) or a backslash are denied. Creating symlinks is not supported on Windows: it needs a privilege or the developer mode, and CreateSymbolicLink takes a path. Setting the times of a read-only name to its current mtime is a no-op that succeeds, because this is how the Lima guest agent triggers IN_ATTRIB for host changes (mountInotify). Addresses lima-vm#6. Assisted-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Sylvain Zimmer <sylvain@sylvainzimmer.com>
sylvinus
force-pushed
the
readonly-names
branch
from
September 30, 2026 08:34
9bceda1 to
7234d04
Compare
Author
|
I pushed a new verison of this with experimental Windows support! |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Open-handle semantics, Unicode protection bypasses, rename races, and resource exhaustion risks remain unresolved.
Review effort: Balanced
Findings: 2
Open (6)
What changed in this PR
Introduces a rooted built-in SFTP server for safer reverse SSHFS mounts and adds component-based read-only paths for Lima.
Changes:
- Adds cross-platform rooted request handlers with symlink-safe writes.
- Adds
ReadonlyNames, read-only timestamp handling, and driver validation. - Adds unit and opt-in SSHFS end-to-end coverage.
| File | Description |
|---|---|
go.mod |
Promotes x/sys to a direct dependency. |
pkg/reversesshfs/reversesshfs.go |
Integrates rooted serving and ReadonlyNames. |
pkg/reversesshfs/rooted.go |
Implements shared rooted SFTP handlers. |
pkg/reversesshfs/rooted_unix.go |
Implements Unix filesystem operations. |
pkg/reversesshfs/rooted_linux.go |
Adds Linux chmod fallback and statfs conversion. |
pkg/reversesshfs/rooted_darwin.go |
Adds macOS chmod and statfs support. |
pkg/reversesshfs/rooted_windows.go |
Implements Windows handle-based operations. |
pkg/reversesshfs/rooted_others.go |
Rejects unsupported platforms. |
pkg/reversesshfs/rooted_test.go |
Tests shared behavior and protections. |
pkg/reversesshfs/rooted_linux_test.go |
Tests Linux chmod fallback. |
pkg/reversesshfs/rooted_darwin_test.go |
Supplies macOS timestamp support. |
pkg/reversesshfs/rooted_windows_test.go |
Tests Windows aliases and reparse points. |
pkg/reversesshfs/rooted_e2e_test.go |
Exercises a real SSHFS mount. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| rootedSys: sys, | ||
| } | ||
| handlers := sftp.Handlers{FileGet: h, FilePut: h, FileCmd: h, FileList: h} | ||
| srv := sftp.NewRequestServer(rwc, handlers, sftp.WithStartDirectory(startDirectory(h.rootPath))) |
| } | ||
| return r | ||
| }, s) | ||
| return strings.EqualFold(s, name) |
| // | ||
| // TODO: use sftp.NewRequestServer with custom handlers to mitigate potential vulnerabilities. | ||
| builtinSftpServer, err = sftp.NewServer(stdio, sftpOpts...) | ||
| builtinSftpServer, rooted, err = newRootedServer(stdio, rsf.LocalPath, rsf.Readonly, rsf.ReadonlyNames) |
Comment on lines
+198
to
+202
| fis, err := f.Readdir(-1) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return listerAt(fis), nil |
Comment on lines
+174
to
+180
| if noReplace { | ||
| var st unix.Stat_t | ||
| if err := unix.Fstatat(newfd, newBase, &st, unix.AT_SYMLINK_NOFOLLOW); err == nil { | ||
| return os.ErrExist | ||
| } | ||
| } | ||
| return unix.Renameat(oldfd, oldBase, newfd, newBase) |
Comment on lines
+126
to
+128
| if fi, err := os.Stat(filepath.Join(root, ".git", "config")); err != nil || fi.ModTime().Unix() == 1 { | ||
| t.Errorf(".git/config times were changed: %v, %v", fi.ModTime(), err) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #6.
Needed for lima-vm/lima#5529
ReadonlyNamesfield: paths with a matching component are read-only (case-insensitive, HFS+ ignorable code points handled as in git).Tested:
Assisted-by: Claude Opus 5.5 (1M context)