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

proposal: x/sys/windows: indicate when ERROR_NOT_ALL_ASSIGNED occurs in AdjustTokenPrivileges #64170

Open
sherif-elmetainy opened this issue Nov 15, 2023 · 16 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Proposal
Milestone

Comments

@sherif-elmetainy
Copy link

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

$ go version
go version go1.21.4 windows/amd64

Does this issue reproduce with the latest release?

Yes, it reproduces with the latest release as of 15 Nov 2023.

What operating system and processor architecture are you using (go env)?

Windows 11 Pro Version 22H2 (OS Build 22621.2715)

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\--OMITED--\AppData\Local\go-build
set GOENV=C:\Users\--OMITED--\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=--OMITED--
set GONOPROXY=--OMITED--
set GONOSUMDB=--OMITED--
set GOOS=windows
set GOPATH=D:\packages\go
set GOPRIVATE=--OMITED--
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.21.4
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=

What did you do?

Here is link to Go Playground (but note that this will not run on the Go Playground because it requires Windows environment). I also provided a full listing below.
https://go.dev/play/p/G2J2te4l-4q

The program is attempting to call AdjustTokenPrivileges to enable the SeSecurityPrivilege. When I run the program elevated (as Administrator), the program runs as expected without an issue. The issue occurs when running as a normal user (non elevated privilege) where it also completes without any error, where it was supposed to panic at line 32.

package main

import (
	"golang.org/x/sys/windows"
	"unsafe"
)

func main() {
	var token windows.Token
	err := windows.OpenProcessToken(windows.CurrentProcess(), windows.TOKEN_ADJUST_PRIVILEGES|windows.TOKEN_QUERY, &token)
	if err != nil {
		panic(err)
	}

	var luid windows.LUID

	privilegeName := "SeSecurityPrivilege"
	err = windows.LookupPrivilegeValue(nil, windows.StringToUTF16Ptr(privilegeName), &luid)
	if err != nil {
		panic(err)
	}

	newStateBuffer := make([]byte, 4+unsafe.Sizeof(windows.LUIDAndAttributes{}))
	newState := (*windows.Tokenprivileges)(unsafe.Pointer(&newStateBuffer[0]))
	newState.PrivilegeCount = 1
	newState.Privileges[0].Luid = luid
	newState.Privileges[0].Attributes = windows.SE_PRIVILEGE_ENABLED

	err = windows.AdjustTokenPrivileges(token, false, newState, uint32(len(newStateBuffer)), nil, nil)
	// When elevated this should succeed, but when not elevated ERROR_NOT_ALL_ASSIGNED should be returned, but it succeeds instead.
	if err != nil {
		panic(err)
	}
}

What did you expect to see?

When I run the program as a normal user (not administrator), I expect the windows.AdjustTokenPrivileges call at line 29 to fail with and return error ERROR_NOT_ALL_ASSIGNED error, since the privilege that was requested to be enable was not enabled.

What did you see instead?

windows.AdjustTokenPrivileges returns null instead of returning an error.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 15, 2023
@gopherbot gopherbot added this to the Unreleased milestone Nov 15, 2023
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 20, 2023
@mknyszek
Copy link
Contributor

CC @golang/windows

@alexbrainman
Copy link
Member

When I run the program as a normal user (not administrator), I expect the windows.AdjustTokenPrivileges call at line 29 to fail with and return error ERROR_NOT_ALL_ASSIGNED error, since the privilege that was requested to be enable was not enabled.

I am not an expert in Windows security, but

https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-adjusttokenprivileges

only mentions ERROR_NOT_ALL_ASSIGNED when SE_PRIVILEGE_REMOVED parameter is used. So perhaps your reasoning is wrong.

I suggest you ask your question somewhere else where you can find at least some Windows security experts. I doubt many experts read your question here.

You can try https://github.com/golang/go/wiki/Questions or https://stackoverflow.com

Alex

@sherif-elmetainy
Copy link
Author

@alexbrainman

Thanks for taking the time to check and explain the issue.

only mentions ERROR_NOT_ALL_ASSIGNED when SE_PRIVILEGE_REMOVED parameter is used. So perhaps your reasoning is wrong.

Believe me, it does occur when SE_PRIVILEGE_ENABLED parameter is used, I tested this many times and I am sure. May be the wording of Microsoft's documentation is confusing, but in the same documentation you linked it says under "return value" section that ERROR_NOT_ALL_ASSIGNED occurs when (and I quote):

The token does not have one or more of the privileges specified in the NewState parameter. The function may succeed with this error value even if no privileges were adjusted. The PreviousState parameter indicates the privileges that were adjusted.

As you see the Windows function call can indicate success, even when ERROR_NOT_ALL_ASSIGNED occurs (That's because it could have succeeded with other privileges, but in my case I am enabling just one and it failed, so it did nothing). The Golang's implementation doesn't check for this scenario and just returns no error at all when this condition happens. It completely ignores the error value if the Windows function returns TRUE.

As for a solution to my problem, I didn't feel the need to ask around as I already knew exactly what was happening. I already worked around the problem by calling the Windows API directly using NewLazySystemDLL and NewProc functions and instead of using the AdjustTokenPrivileges function in Go's ``golang.org/x/sys/windowspackage. So basically I created my own version ofAdjustTokenPrivileges` function.

I wanted it fixed so that others don't face the same issue. I thought of actually fixing the code myself and submitting a pull request, but I was discouraged by having to create a Gerrit account then going through several steps, and I also didn't have enough time to setup an environment on my local machine where I can build the source and run tests as it required installing some stuff and changing some git settings that may affect my other work, so I needed to setup a VM specifically for that. In short it was a long process for just updating 2-3 lines of code for something that could be rejected as it is a breaking change and I didn't have time. I also realize that this is a niche scenario, and not a lot of developers are affected by it.

@alexbrainman
Copy link
Member

I wanted it fixed so that others don't face the same issue. I thought of actually fixing the code myself and submitting a pull request, but I was discouraged by having to create a Gerrit account then going through several steps, and I also didn't have enough time ...

It is also fine just to point to the problem in the code, and perhaps others will be willing to implement the fix.

Alex

@sherif-elmetainy
Copy link
Author

sherif-elmetainy commented Dec 26, 2023

It is also fine just to point to the problem in the code, and perhaps others will be willing to implement the fix.

@alexbrainman

You are right.
Here is a link to the code with the problem:
https://cs.opensource.google/go/x/sys/+/master:windows/zsyscall_windows.go;drc=11eadc05e9bfbaaf23ac6ebc28595a9a12f607f4;l=553

original code (for some reason this is different from the code on this repo)

func AdjustTokenPrivileges(token Token, disableAllPrivileges bool, newstate *Tokenprivileges, buflen uint32, prevstate *Tokenprivileges, returnlen *uint32) (err error) {
	var _p0 uint32
	if disableAllPrivileges {
		_p0 = 1
	}
	r1, _, e1 := syscall.Syscall6(procAdjustTokenPrivileges.Addr(), 6, uintptr(token), uintptr(_p0), uintptr(unsafe.Pointer(newstate)), uintptr(buflen), uintptr(unsafe.Pointer(prevstate)), uintptr(unsafe.Pointer(returnlen)))
	if r1 == 0 {
		err = errnoErr(e1)
	}
	return
}

suggested fix: This is what I would do to fix:

func AdjustTokenPrivileges(token Token, disableAllPrivileges bool, newstate *Tokenprivileges, buflen uint32, prevstate *Tokenprivileges, returnlen *uint32) (err error) {
	var _p0 uint32
	if disableAllPrivileges {
		_p0 = 1
	}
	r1, _, e1 := syscall.Syscall6(procAdjustTokenPrivileges.Addr(), 6, uintptr(token), uintptr(_p0), uintptr(unsafe.Pointer(newstate)), uintptr(buflen), uintptr(unsafe.Pointer(prevstate)), uintptr(unsafe.Pointer(returnlen)))
	if r1 == 0 || e1 == ERROR_NOT_ALL_ASSIGNED { // <-- additional check here.
		err = errnoErr(e1)
	}
	return
}

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/566036 mentions this issue: windows: handle ERROR_NOT_ALL_ASSIGNED in AdjustTokenPrivileges

@qmuntal
Copy link
Contributor

qmuntal commented Feb 23, 2024

I've submitted CL 566036 which contains the fix proposed by @sherif-elmetainy. The problem I now see is that this is a breaking change.

For example, the following code from https://github.com/Azure/azure-storage-azcopy would no longer report a useful error, at it is a shame because it is trying to sidestep our AdjustTokenPrivileges issue.

err = windows.AdjustTokenPrivileges(procToken, false, &tokenPrivs, oldPrivsSize, &oldPrivs, &requiredReturnLen)
if err != nil {
     return err
}
if oldPrivs.PrivilegeCount != 1 {
     // Only the successful changes are returned in the old state
     // If there were none, that means it didn't work
     return errors.New("could not activate '" + BackupModeFlagName + "' mode.  Probably the account running AzCopy does not have " +
          privName + " so AzCopy could not activate that privilege. Administrators usually have that privilege, but only when they are in an elevated command prompt. " +
          "Members of the 'Backup Operators' security group also have that privilege. To check which privileges an account has, run this from a command line: whoami /priv")
}

There are many other AdjustTokenPrivileges uses uses that are wrong, see this query. Most of them are just expecting windows.AdjustTokenPrivileges to return an error when ERROR_NOT_ALL_ASSIGNED happens. A few try to call GetLastError right after, not realizing that that function is also broken (see #41220). From https://github.com/harryeetsource:

err = windows.AdjustTokenPrivileges(token, false, &tokenPrivs, 1024, nil, nil)
if err != nil || windows.GetLastError() != nil {
	return false
}

@qmuntal
Copy link
Contributor

qmuntal commented Feb 23, 2024

@bcmills suggested a few solutions to this issue in https://go-review.googlesource.com/c/sys/+/566036/comment/3e705da4_cddd965a/:

  1. Return ERROR_NOT_ALL_ASSIGNED even on success, and document that the Go AdjustTokenPrivileges function may return it.

  2. Add an AdjustTokenPrivileges variant that returns (bool, error) instead of just error, bundling together the results of AdjustTokenPrivileges and GetLastError.

  3. Make no change to the return value, but add an example illustrating how to use the PreviousState parameter to determine whether all requested privileges were assigned.

  4. Fix #41220 by making GetLastError actually work in limited cases (per https://go.dev/issue/41220#issuecomment-1959940374), and add an example for AdjustTokenPrivileges illustrating how to lock and unlock the OS thread in order to check for ERROR_NOT_ALL_ASSIGNED.

Of those, (1) is technically an incompatible change (but may be ok as a bug-fix), (2) is a new API, (3) may be tricky to use, and (4) seems a bit fragile.

@qmuntal
Copy link
Contributor

qmuntal commented Feb 23, 2024

I would lean towards having a new API (option 2 in previous comment):

// AdjustTokenPrivileges2 enables or disables privileges in the specified token
// by calling AdjustTokenPrivileges.
// Note that err can be [ERROR_NOT_ALL_ASSIGNED] even if success is true,
// which means that one or more privileges were not adjusted.
func AdjustTokenPrivileges2(...) (success bool, err error)

I would also deprecate windows.AdjustTokenPrivileges. It would make no sense to use it once AdjustTokenPrivileges2 exists.

@bcmills bcmills modified the milestones: Unreleased, Proposal Feb 23, 2024
@bcmills bcmills changed the title x/sys/windows: AdjustTokenPrivileges does not return error when ERROR_NOT_ALL_ASSIGNED occurs proposal: x/sys/windows: indicate when when ERROR_NOT_ALL_ASSIGNED occurs in AdjustTokenPrivileges Feb 23, 2024
@alexbrainman
Copy link
Member

I would lean towards having a new API (option 2 in previous comment):

// AdjustTokenPrivileges2 enables or disables privileges in the specified token
// by calling AdjustTokenPrivileges.
// Note that err can be [ERROR_NOT_ALL_ASSIGNED] even if success is true,
// which means that one or more privileges were not adjusted.
func AdjustTokenPrivileges2(...) (success bool, err error)

@qmuntal what would your proposed new AdjustTokenPrivileges2 function do? How would it calculate the success return value?

I would go for

  1. Make no change to the return value, but add an example illustrating how to use the PreviousState parameter to determine whether all requested privileges were assigned.

solution instead.

Perhaps I don't understand the problem, but this is how AdjustTokenPrivileges is designed by Microsoft. The only tricky bit Go implementation code adds is converting ERROR_SUCCESS to nil. While ERROR_NOT_ALL_ASSIGNED here can also be success or not. So adding some documentation should be OK in my view.

Alex

@sherif-elmetainy
Copy link
Author

Perhaps I don't understand the problem, but this is how AdjustTokenPrivileges is designed by Microsoft.

The function designed by Microsoft returns a bool in addition to the to the error value returned from GetLastError. So Option 2 with signature func AdjustTokenPrivileges2(...) (success bool, err error) suggested by @qmuntal would match the Microsoft behavior in case the success is true and ERROR_NOT_ALL_ASSIGNED is still returned. The signature of the current API and that suggested as Option 1 has only 1 return value (the error) and no bool return value, so can't possibly match Microsoft's design.

@bcmills
Copy link
Contributor

bcmills commented Feb 26, 2024

The signature of the current API and that suggested as Option 1 has only 1 return value (the error) and no bool return value, so can't possibly match Microsoft's design.

Maybe? But I'm not sure from the documentation whether it is ever possible to get ERROR_NOT_ALL_ASSIGNED in the “failure” case. If it is only possible in the “success” case, then returning only the error value still provides sufficient information, which callers can use like:

	err := windows.AdjustTokenPrivileges(…)
	switch err {
	case nil:
		// Adjusted all specified privileges.
	case windows.ERROR_NOT_ALL_ASSIGNED:
		// Partial success, but “[t]he token does not have one or more of the privileges
		// specified in the NewState parameter.”
	default:
		// Failure.
		return err
	}

That is: maybe the bool return value is just there to save callers the expense of a call to GetLastError in the trivial success case?

@qmuntal
Copy link
Contributor

qmuntal commented Feb 27, 2024

maybe the bool return value is just there to save callers the expense of a call to GetLastError in the trivial success case?

Could be, but this assumption is not written in the AdjustTokenPrivileges docs. I think that it is safer and future-proof to return (sucess bool, err error) so we don't have to create a AdjustTokenPrivileges3 in 5 years.

@qmuntal what would your proposed new AdjustTokenPrivileges2 function do? How would it calculate the success return value?

Like in here: https://go-review.googlesource.com/c/sys/+/566036

@alexbrainman
Copy link
Member

@qmuntal

maybe the bool return value is just there to save callers the expense of a call to GetLastError in the trivial success case?

Could be, but this assumption is not written in the AdjustTokenPrivileges docs. I think that it is safer and future-proof to return (sucess bool, err error) so we don't have to create a AdjustTokenPrivileges3 in 5 years.

Do you have a repro that demonstrates the problem with the current Go implementation of the AdjustTokenPrivileges?

If you do not have, then I would not vote for implementing AdjustTokenPrivileges2 just because you or others read Microsoft AdjustTokenPrivileges documentation in a particular way.

Perhaps Go implementation does not match Microsoft documentation. But as long as users programs are not broken in any way, why change anything?

@qmuntal what would your proposed new AdjustTokenPrivileges2 function do? How would it calculate the success return value?

Like in here: https://go-review.googlesource.com/c/sys/+/566036

Your https://go-review.googlesource.com/c/sys/+/566036 change does not contain AdjustTokenPrivileges2 function. If you propose we add new AdjustTokenPrivileges2 function, then we should see what the function will do. I would also like to see if new AdjustTokenPrivileges2 solves the problem in the repro that you will provide.

Thank you.

Alex

@qmuntal qmuntal changed the title proposal: x/sys/windows: indicate when when ERROR_NOT_ALL_ASSIGNED occurs in AdjustTokenPrivileges proposal: x/sys/windows: indicate when ERROR_NOT_ALL_ASSIGNED occurs in AdjustTokenPrivileges Mar 7, 2024
@qmuntal
Copy link
Contributor

qmuntal commented Mar 7, 2024

Do you have a repro that demonstrates the problem with the current Go implementation of the AdjustTokenPrivileges?

The issue description contains a reproducer. When running as no-admin, windows.AdjustTokenPrivileges returns no error, but the privilege hasn't been granted. It should return ERROR_NOT_ALL_ASSIGNED.

CL 566036 also contains a test case that fails with the current Go implementation.

Perhaps Go implementation does not match Microsoft documentation. But as long as users programs are not broken in any way, why change anything?

The point is that programs that use windows.AdjustTokenPrivileges are probably broken, because they can't detect when a privilege hasn't been granted. This is why some projects that know about this issue don't use that function, but define their own. For example, microsoft/go-winio.

@alexbrainman
Copy link
Member

@qmuntal

CL 566036 also contains a test case that fails with the current Go implementation.

Thanks for the test case. I was able to reproduce the problem.

I agree with your suggestion to add

// AdjustTokenPrivileges2 enables or disables privileges in the specified token
// by calling AdjustTokenPrivileges.
// Note that err can be [ERROR_NOT_ALL_ASSIGNED] even if success is true,
// which means that one or more privileges were not adjusted.
func AdjustTokenPrivileges2(...) (success bool, err error)

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Proposal
Projects
Status: Incoming
Development

No branches or pull requests

6 participants