fix(localfile): reject symlinks in SetFileAttributes - #627
fix(localfile): reject symlinks in SetFileAttributes#627sfc-gh-ikryvanos wants to merge 1 commit into
Conversation
Attribute changes followed symlinks, so chown/chmod/immutable could escape path-based policy confinement. Lstat-reject symlinks, use Lchown, and open immutable updates with O_NOFOLLOW. Co-authored-by: Cursor <cursoragent@cursor.com>
| // directory prefix could create a symlink inside that prefix pointing at an | ||
| // arbitrary target (e.g. /etc/shadow) and then chown/chmod/immutable the | ||
| // target, escaping the confinement. Lstat inspects the link itself. | ||
| if fi, err := os.Lstat(filename); err != nil { |
There was a problem hiding this comment.
Nice fix overall — switching chown to Lchown and opening the immutable path with O_NOFOLLOW makes those two operations safe at the syscall level, independent of any earlier check.
The chmod path is the exception. It still calls unix.Chmod(filename, ...), which follows symlinks, so its only protection is the os.Lstat guard at the top of the function. That leaves a time-of-check/time-of-use gap between the Lstat and this Chmod: a principal who can write into the confined directory can pass the check with a regular file and then swap it for a symlink to an out-of-policy target (e.g. /etc/shadow) before this line runs, and the chmod follows the link. That's the same link-following class the PR is closing, just narrowed to chmod and gated behind a race.
Linux doesn't give us a direct Lchmod, and fchmodat(AT_SYMLINK_NOFOLLOW) returns EOPNOTSUPP for chmod, so the race-free approach is the same one the immutable path already uses — open with O_NOFOLLOW and operate on the descriptor:
fd, err := os.OpenFile(filename, os.O_RDONLY|unix.O_NOFOLLOW, 0)
if err != nil {
return nil, status.Errorf(codes.Internal, "error opening for chmod: %v", err)
}
defer fd.Close()
if err := unix.Fchmod(int(fd.Fd()), mode&modeMask); err != nil {
return nil, status.Errorf(codes.Internal, "error from fchmod: %v", err)
}
2 participants
%23%23 Summary
SetFileAttributeson symlink paths (FailedPrecondition) so attribute changes cannot follow a link to an out-of-policy targetLchowninstead ofChown, and open immutable flag updates withO_NOFOLLOWon Linux as defense in depth%23%23 Test plan
go test ./services/localfile/server/...GOOS=linux GOARCH=amd64 go build ./services/localfile/...