Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

os: RemoveAll susceptible to symlink race #52745

Open
rolandshoemaker opened this issue May 6, 2022 · 6 comments
Open

os: RemoveAll susceptible to symlink race #52745

rolandshoemaker opened this issue May 6, 2022 · 6 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@rolandshoemaker
Copy link
Member

Both the at (systems that implement openat, unlinkat etc) and the noat implementations of os.RemoveAll are susceptible to a TOCTOU symlink race, where a directory can be replaced with a symlink between being stat'd and open'd. This can be used to 'trick' the program into deleting things it does not expect to delete. This is a minor security issue, but has relatively limited impact because it requires a multi-user system where an attacker is able to create symlinks, a program which will call os.RemoveAll on an attacker writable tree.

This is due to O_NOFOLLOW not being passed to openat/open on Unix systems, and FILE_FLAG_OPEN_REPARSE_POINT not being passed to CreateFileW on Windows. On Unix systems the fix is extremely simple, but on Windows it requires some changes to the Windows syscalls, since the flags passed to CreateFileW are fixed and cannot be altered by the caller currently.
@rolandshoemaker rolandshoemaker added OS-Windows NeedsFix The path to resolution is known, but the work has not been done. labels May 6, 2022
@rolandshoemaker rolandshoemaker added this to the Backlog milestone May 6, 2022
@neild neild removed the OS-Windows label Jul 13, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/588495 mentions this issue: os: RemoveAll: don't stat before open dir

gopherbot pushed a commit that referenced this issue May 29, 2024
@kolyshkin @gopherbot
os: RemoveAll: fix symlink race for unix
a3a584e 
Since all the platforms now support O_DIRECTORY flag for open, it can be
used to (together with O_NOFOLLOW) to ensure we open a directory, thus
eliminating the need to call stat before open. This fixes the symlink race,
when a directory is replaced by a symlink in between stat and open calls.

While at it, rename openFdAt to openDirAt, because this function is (and was)
meant for directories only.

NOTE Solaris supports O_DIRECTORY since before Solaris 11 (which is the
only version Go supports since supported version now), and Illumos
always had it. The only missing piece was O_DIRECTORY flag value, which
is taken from golang.org/x/sys/unix.

Updates #52745.

Change-Id: Ic1111d688eebc8804a87d39d3261c2a6eb33f176
Reviewed-on: https://go-review.googlesource.com/c/go/+/588495
Reviewed-by: Ian Lance Taylor <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Aleksa Sarai <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
@rolandshoemaker
Copy link
Member Author

@gopherbot please open backport issues for this, it's a security hardening issue which fixes a long running TOCTOU race.

This was referenced May 29, 2024
Closed
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #67695 (for 1.21), #67696 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@rolandshoemaker
Copy link
Member Author

The Unix-likes (everything wtih openat) is basically fixed now, but we still need to do the same thing for Windows. We'll try to get that done such that we can backport it into the 1.24 release once that happens so that all implementations are aligned.

@cyphar cyphar mentioned this issue Oct 4, 2024
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/629698 mentions this issue: os: add Root.RemoveAll, avoid symlink race in RemoveAll on Windows

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/630337 mentions this issue: os: avoid symlink race in RemoveAll on Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@neild @rolandshoemaker @gopherbot