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

syscall: Add to Token to Windows syscall.SysProcAttr #21105

Closed
pquerna opened this issue Jul 20, 2017 · 7 comments
Closed

syscall: Add to Token to Windows syscall.SysProcAttr #21105

pquerna opened this issue Jul 20, 2017 · 7 comments

Comments

@pquerna
Copy link

@pquerna pquerna commented Jul 20, 2017

What version of Go are you using (go version)?

go1.9beta2

What did you do?

syscall.SysProcAttr on Windows is currently very limited. On Linux/Unixes, it has been extended to include many features like UID Mapping, chroot, etc.

Adding Token syscall.Handle field to SysProcAttr and using it as a parameter to CreateProcessAsUser would enable many use cases on Windows, including:

Besides the new struct field, syscall.StartProcess would need to call CreateProcessAsUser instead of CreateProcess

The equivalents of these are already possible on Linux.

We have maintained a partial internal fork of os/exec since go1.4 where we pass in a Token, and use CreateProcessAsUser, but some of our tests broke in go1.9beta2 (our fault), but we would love to see this go into upstream at some point.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Aug 8, 2017

I think it is good idea. And will not be too complicated to implement.
@pquerna, please, ping us if you won't get decision soon. Thank you.

Alex

@pquerna
Copy link
Author

@pquerna pquerna commented Oct 31, 2017

@alexbrainman decision on this feature? Thanks!

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 1, 2017

@alexbrainman decision on this feature?

Like I said before, this sounds good to me. But I don't do deciding here, @rsc makes desisions. What do you think, Russ?

If we want this implemented for go1.10, we need to do it quickly - freeze for go1/10 is about to start https://groups.google.com/d/topic/golang-dev/csSLMsd7SF4/discussion

If we decide to do it, the change would have to include some test that demonstrates / verifies new feature. @pquerna do you have any suggestion for the test? Do you want to implement whole thing. if it is approved? Thank you.

Alex

pquerna pushed a commit to pquerna/go that referenced this issue Nov 1, 2017
Fixes golang#21105

Change-Id: Ia2dea9b82a356795f581ce75616198b46e97abb6
@pquerna
Copy link
Author

@pquerna pquerna commented Nov 1, 2017

The actual changes are small:

master...pquerna:windows_add_token_to_sys_proc_attr

Making a test case that launches a process at a lower integrity level is hard to do without a few hundred lines of new code (needs new bindings to CreateWellKnownSid, DuplicateTokenEx, SetTokenInformation, along with a few new structs/enums). Most of these could go into x/sys/windows, but that another whole set of CRs for a test case. With that dance, your test would look something like this: https://gist.github.com/pquerna/04f1933e5f6ced7f66f1f181d1c21ebf

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 1, 2017

Seems reasonable to me.

But the freeze for new features was today. (I just got back today)

Do you want to send a Gerrit change? If you can do it within this week and it looks safe, it might still make it to Go 1.10. Otherwise it'll likely be a Go 1.11 thing.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 1, 2017

Change https://golang.org/cl/75253 mentions this issue: syscall: Add Token to Windows SysProcAttr

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 2, 2017

Making a test case that launches a process at a lower integrity level is hard to do without a few hundred lines of new code (needs new bindings to CreateWellKnownSid, DuplicateTokenEx, SetTokenInformation, along with a few new structs/enums). Most of these could go into x/sys/windows, but that another whole set of CRs for a test case. With that dance, your test would look something like this: https://gist.github.com/pquerna/04f1933e5f6ced7f66f1f181d1c21ebf

Yes I had something like that in mind. And we need to test your new code, so I would like to see that test before we discard it as "too many lines". We cannot put all that code into x/sys/window, because it needs to be part of main repo, so internal/syscall/windows is the place. Maybe we can use some of our existing code from syscall/security_windows.go. If you want to to another CR (or CRs) for the test that is fine too. Thank you.

Alex

pquerna pushed a commit to pquerna/go that referenced this issue Nov 2, 2017
Fixes golang#21105

Change-Id: Ia2dea9b82a356795f581ce75616198b46e97abb6
pquerna pushed a commit to pquerna/go that referenced this issue Nov 2, 2017
Fixes golang#21105

Change-Id: Ia2dea9b82a356795f581ce75616198b46e97abb6
pquerna pushed a commit to pquerna/go that referenced this issue Nov 2, 2017
Fixes golang#21105

Change-Id: Ia2dea9b82a356795f581ce75616198b46e97abb6
@gopherbot gopherbot closed this in bb98331 Nov 6, 2017
@golang golang locked and limited conversation to collaborators Nov 6, 2018
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
4 participants
You can’t perform that action at this time.