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.

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

os: Symlink creation should work on Windows without elevation #22874

Closed
calmh opened this issue Nov 25, 2017 · 11 comments
Closed

os: Symlink creation should work on Windows without elevation #22874

calmh opened this issue Nov 25, 2017 · 11 comments

Comments

@calmh
Copy link
Contributor

@calmh calmh commented Nov 25, 2017

On Windows 10 Creators Update, with Developer Mode enabled, symlinks can be created by regular users without elevation. This can be tested by opening a command prompt and running mklink a b for example. However, creating links using os.Symlink in Go 1.9.2 windows/amd64 fails with A required privilege is not held by the client.

The cause seems to be that the application needs to pass the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag to CreateSymbolicLink in order to be able to create symbolic links without elevation. I can't exactly follow the reasoning behind introducing and requiring this flag, but it seems like we should probably set it in the os.Symlink implementation for Windows.

https://msdn.microsoft.com/en-us/library/windows/desktop/aa363866(v=vs.85).aspx

Given that this flag exists at all we should probably expect further bizarre stuff like the call failing if we are privileged and passing that flag, or if we are privileged and on a Windows version where the flag doesn't exist yet, or whatever. So it probably requires more careful testing than just slapping on the flag and seeing it succeed...

@calmh

This comment has been minimized.

Copy link
Contributor Author

@calmh calmh commented Nov 25, 2017

For the record, this does in fact make the call succeed on my systems;

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index 93b6c135c7..8baf7d2c60 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -391,6 +391,7 @@ func Symlink(oldname, newname string) error {
        if isdir {
                flags |= syscall.SYMBOLIC_LINK_FLAG_DIRECTORY
        }
+       flags |= 0x02 // SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
        err = syscall.CreateSymbolicLink(n, o, flags)
        if err != nil {
                return &LinkError{"symlink", oldname, newname, err}
@as

This comment has been minimized.

Copy link
Contributor

@as as commented Nov 25, 2017

If they transparently allowed users to create symlinks without client side code changes, that means existing broken code would suddenly start working. Instead, the Windows 10 Creators Update introduces honor system privilege escalation.

FWIW the change works on my Windows 10 system after the patch too

@bradfitz bradfitz added the OS-Windows label Nov 25, 2017
@bradfitz bradfitz added this to the Go1.11 milestone Nov 25, 2017
@bradfitz bradfitz added the NeedsFix label Nov 25, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 25, 2017

Thanks for the info!

We can look into this for Go 1.11, including writing a test which will make sure we don't accidentally break Windows XP etc by including this flag. Hopefully we don't need to conditionally set the flag after detecting the OS version at runtime. But we might.

/cc @alexbrainman

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 28, 2017

@calmh do you propose to just add SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag to syscall.CreateSymbolicLink call? I am bit concerned about doing this by default (what @as said) - your program fails at this moment, but it will succeed after your change. And given that this functionality is a "security concern" according to Microsoft, that makes me even more worried. Reading https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/ does not make it clear in my mind what Microsoft recommends - new functionality will only work in "developer" mode, so they are still concerned about security - why do they leave final decision on us developers (SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag)?

Alex

@calmh

This comment has been minimized.

Copy link
Contributor Author

@calmh calmh commented Nov 28, 2017

Go tries to take the cross platform approach, erring to the side of Unixness. Compare for example os.Remove which takes care to remove read only files on Windows too, because that's how it works on most other Go support systems - as opposed to the Windows native API. On other systems regular users can create symlinks, so I don't think most Go programs will be surprised if this also succeeds on Windows (with the appropriate mode enabled, etc.). The developer mode in Windows 10 is one thing, it's not clear to me that symlinks for regular users will not be enabled by default in Windows 11 or whatever.

So yes, I think we should set the flag and make os.Symlink succeed when it can, even on Windows. Especially since there is no other way to do it on Windows (using the standard library without hacks).

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 28, 2017

so I don't think most Go programs will be surprised if this also succeeds on Windows

My concern is that we are changing existing behavior. And a security related behavior at that. I am not security expert myself, so I would not know what the implication of this change is.

it's not clear to me that symlinks for regular users will not be enabled by default in Windows 11 or whatever.

That does not bother me, because it is out of our control.

Especially since there is no other way to do it on Windows (using the standard library without hacks).

Yes, I don't see a way to allow new functionality in Go standard library. Maybe we should just wait for Windows 11 :-)

Alex

@calmh

This comment has been minimized.

Copy link
Contributor Author

@calmh calmh commented Nov 28, 2017

I don't know how to argue against an unspecified security straw man, but whatever Go program depends on this behavior seems broken to me. I can see Microsoft having to retain 100% API compatibility forever (that's their thing for good and bad), but we do not to the same extent and there is prior art - for example in #9606. The impact of this change would be insignificant in comparison to that change, imho.

@as

This comment has been minimized.

Copy link
Contributor

@as as commented Nov 28, 2017

Go tries to take the cross platform approach, erring to the side of Unixness.

Symbolic links are UNIX like Sockets and X11 are UNIX: regrettably so. There is probably a reason MS used an opt-in for this feature, whether they want free quality assurance through their developer program or otherwise aren't sure of what this change will do across different Windows versions: it's odd.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 7, 2017

The impact of this change would be insignificant in comparison to that change, imho.

Fair enough. @calmh if you send a fix for this, I will be happy to test it on Windows XP. But this would have to wait until after go1.10 is released. Thank you.

Alex

PrototypeNM1 added a commit to PrototypeNM1/syncthing that referenced this issue Jan 14, 2018
…g as SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE is not yet implemented in Go's implementation for Window's symlinks as of 1.9.2 - golang/go#22874

Signed-off-by: Nicholas Rishel <PrototypeNM1@users.noreply.github.com>
PrototypeNM1 added a commit to PrototypeNM1/syncthing that referenced this issue Jan 15, 2018
…g as SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE is not yet implemented in Go's implementation for Window's symlinks as of 1.9.2 - golang/go#22874

Signed-off-by: Nicholas Rishel <PrototypeNM1@users.noreply.github.com>
PrototypeNM1 added a commit to PrototypeNM1/syncthing that referenced this issue Jan 23, 2018
…g as SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE is not yet implemented in Go's implementation for Window's symlinks as of 1.9.2 - golang/go#22874

Signed-off-by: Nicholas Rishel <PrototypeNM1@users.noreply.github.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 8, 2018

Change https://golang.org/cl/99337 mentions this issue: os: enable symlink creation on Windows 10

@djdv

This comment has been minimized.

Copy link

@djdv djdv commented May 11, 2018

@calmh
I'm not sure where to report this, but os.Symlink returns A required privilege is not held by the client. if the link already exists, even if they have creation privilege. I find this very misleading.
On FreeBSD, the same error is file exists.

err := os.Symlink("old", "new")
if err != nil {
        fmt.Printf("%s\n", err)
}
err = os.Symlink("old", "new")
if err != nil {
        fmt.Printf("%s\n", err)
}

Unfortunately, I beleive the fault seems to be in the WINAPI itself, CreateSymbolicLink returning that error code instead of ERROR_FILE_EXISTS.
Maybe someone at MS should be informed of this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.