CC @golang/windows

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

@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.

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

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
}

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

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
}

@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.

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.

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

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.

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

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.

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?

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

@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

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.

@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