Skip to content

fix: add O_NOFOLLOW to copyFile#2534

Open
valoq wants to merge 1 commit intogokcehan:masterfrom
valoq:copyfile
Open

fix: add O_NOFOLLOW to copyFile#2534
valoq wants to merge 1 commit intogokcehan:masterfrom
valoq:copyfile

Conversation

@valoq
Copy link
Copy Markdown
Contributor

@valoq valoq commented Apr 17, 2026

copyFile currently opens the destination with O_CREATE|O_TRUNC
on shared storage, other users can place a symlink at the destination and cause the write operation to be redirected..
adding O_NOFOLLOW avoids this.

Note that O_NOFOLLOW does not exist on windows.

@valoq valoq marked this pull request as draft April 22, 2026 16:22
@valoq valoq marked this pull request as ready for review April 22, 2026 16:22
@valoq
Copy link
Copy Markdown
Contributor Author

valoq commented Apr 22, 2026

I update this PR to address one remaining case of the same issue

Copy link
Copy Markdown
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O_NOFOLLOW only works on the last component of a path.

Theoretically this means your patch will not address cases where other parts of the destination path contains a symbolic link. For example when copying foo/bar/baz/test.txt to /home/user/dest, the destination path will be /home/user/dest/foo/bar/baz/test.txt, and while O_NOFOLLOW will protect against test.txt being a symbolic link, it will not cover cases where foo, bar or baz is a symbolic link.

@valoq
Copy link
Copy Markdown
Contributor Author

valoq commented Apr 24, 2026

I changed the patch to use os.Root instead which is Go's standard library solution for safely writing files into a directory (added in go 1.24, but we already require 1.25 anyway)

os.Root methods reject file modes with non-permission bits, so info.Mode().Perm() is needed where the original code passed info.Mode() directly.

Comment thread copy.go
Comment thread copy.go
Comment thread os.go Outdated
@valoq
Copy link
Copy Markdown
Contributor Author

valoq commented Apr 25, 2026

Thanks, that should all be addressed now.

@valoq valoq requested a review from joelim-work April 25, 2026 15:14
@joelim-work
Copy link
Copy Markdown
Collaborator

Sorry I just thought of one edge case - if using os.Root, it's still possible for someone to create a symbolic link during the copy which points to a location inside the destination root, which results in the file at the target location being overwritten. So it is not completely foolproof. Using O_NOFOLLOW would also not fully address the issue either as the symbolic link might not be at the last component in the path. An example would be to copy src/ which contains src/subdir/test.txt and then create a symbolic link at subdir which points somewhere else.

Hi @CatsDeservePets, I was wondering if you had any thoughts on this PR.

@valoq
Copy link
Copy Markdown
Contributor Author

valoq commented Apr 27, 2026

Uh, this is becoming a bit too complicated for what I had in mind. The intention was to ensure reliable copy operations even on shared directories. And to mimic the same behavior as binutils cp enforces.

@valoq
Copy link
Copy Markdown
Contributor Author

valoq commented Apr 27, 2026

After looking into this a bit more, it turns out that completely preventing interference is not possible, and even coreutils 'cp' does not address this. Actually, the current behavior of lf is already equivalent to the threat model applied by cp. We could drop this entire PR if thats the objective.

The only reason to keep it is to address the cases where the improvement is easy and with low cost in terms of complexity. The focus should be on accidental writes that were not intended when symlinks are encountered.

I will look a bit more into this, but I would only merge this as a minimal change that adds useful protection against either a trivial and common attack on shared drives or if it prevents accidents.

At the moment, I tend towards dropping this PR, since using 'cp' behavior as baseline seems reasonable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants