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.

Sign up for GitHub

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

Jump to bottom

syscall: TOKEN_ALL_ACCESS inconsistency #25775

Closed
limard opened this issue Jun 7, 2018 · 7 comments
Closed

syscall: TOKEN_ALL_ACCESS inconsistency #25775

limard opened this issue Jun 7, 2018 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone
Go1.12

Comments

@limard
Copy link

limard commented Jun 7, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.2 windows/amd64

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

Windows 10
Windows SDK v7.0A
_WIN32_WINNT>0x400

What did you do?

I'm using Golang to rewrite a C++ Program in Windows

syscall.TOKEN_ALL_ACCESS in golang is 0xF00FF,
this value is the same as TOKEN_ALL_ACCESS_P in WinNT.h.

But TOKEN_ALL_ACCESS in WinNT.h is 0xF01FF (TOKEN_ALL_ACCESS_P|TOKEN_ADJUST_SESSIONID) when _WIN32_WINNT>0x400

The same name but different value, it is easily to make mistake.

What did you expect to see?

The value of syscall.TOKEN_ALL_ACCESS is the same as TOKEN_ALL_ACCESS in WinNT.h

What did you see instead?

syscall.TOKEN_ALL_ACCESS != TOKEN_ALL_ACCESS in WinNT.h

Thanks!
@agnivade agnivade changed the title src/syscall.TOKEN_ALL_ACCESS definition syscall: TOKEN_ALL_ACCESS inconsistency Jun 7, 2018
@agnivade agnivade added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 7, 2018
@agnivade agnivade added this to the Go1.12 milestone Jun 7, 2018
@agnivade
Copy link
Contributor

agnivade commented Jun 7, 2018

/cc @alexbrainman

@alexbrainman
Copy link
Member

Yes current syscall.TOKEN_ALL_ACCESS is wrong. Current syscall.TOKEN_ALL_ACCESS was correct some long time ago, but was changed. We should change it. I am not sure if we are allowed to change syscall const value.

@bradfitz or @ianlancetaylor

Can we?

This const is security related, so it makes it kind of important.

Thank you.

Alex

@ianlancetaylor
Copy link
Member

We've decided that it's OK to change values in the syscall package if they are wrong (https://golang.org/doc/go1compat#operating_systems).

Does golang.org/x/sys/windows also need to be fixed?

@wingyplus
Copy link
Contributor

wingyplus commented Jun 9, 2018 •
edited
Loading

I checked by writing tests

package syscall_test

import (
	"syscall"
	"testing"

	"golang.org/x/sys/windows"
)

func TestTOKEN_ALL_ACCESS(t *testing.T) {
	const expected = 0xF01FF
	
	t.Run("syscall", func(t *testing.T) {
		if syscall.TOKEN_ALL_ACCESS != expected {
			t.Errorf("syscall.TOKEN_ALL_ACCESS = %x, want %x", syscall.TOKEN_ALL_ACCESS, expected)
		}
	})

	t.Run("golang.org/x/sys/windows", func(t *testing.T) {
		if windows.TOKEN_ALL_ACCESS != expected {
			t.Errorf("golang.org/x/sys/windows.TOKEN_ALL_ACCESS = %x, want %x", windows.TOKEN_ALL_ACCESS, expected)
		}
	})
}

The result is

$ go test -v
=== RUN   TestTOKEN_ALL_ACCESS
=== RUN   TestTOKEN_ALL_ACCESS/syscall
=== RUN   TestTOKEN_ALL_ACCESS/golang.org/x/sys/windows
--- FAIL: TestTOKEN_ALL_ACCESS (0.00s)
    --- FAIL: TestTOKEN_ALL_ACCESS/syscall (0.00s)
        syscall_std_test.go:13: syscall.TOKEN_ALL_ACCESS = f00ff, want f01ff
    --- FAIL: TestTOKEN_ALL_ACCESS/golang.org/x/sys/windows (0.00s)
        syscall_std_test.go:19: golang.org/x/sys/windows.TOKEN_ALL_ACCESS = f00ff, want f01ff
FAIL
exit status 1
FAIL    github.com/wingyplus/syscall_test       0.058s

I see implementation in WinNT.h is:

#define TOKEN_ALL_ACCESS_P (STANDARD_RIGHTS_REQUIRED  |\
                          TOKEN_ASSIGN_PRIMARY      |\
                          TOKEN_DUPLICATE           |\
                          TOKEN_IMPERSONATE         |\
                          TOKEN_QUERY               |\
                          TOKEN_QUERY_SOURCE        |\
                          TOKEN_ADJUST_PRIVILEGES   |\
                          TOKEN_ADJUST_GROUPS       |\
                          TOKEN_ADJUST_DEFAULT )

#if ((defined(_WIN32_WINNT) && (_WIN32_WINNT > 0x0400)) || (!defined(_WIN32_WINNT)))
#define TOKEN_ALL_ACCESS  (TOKEN_ALL_ACCESS_P |\
                          TOKEN_ADJUST_SESSIONID )
#else
#define TOKEN_ALL_ACCESS  (TOKEN_ALL_ACCESS_P)
#endif

It needs to add TOKEN_ADJUST_SESSIONID constant to both packages and TOKEN_ALL_ACCESS needs to or (|) with this constant.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/117635 mentions this issue: syscall: update TOKEN_ALL_ACCESS according to WinNT.h

@alexbrainman
Copy link
Member

Does golang.org/x/sys/windows also need to be fixed?

Indeed.

Alex

gopherbot pushed a commit that referenced this issue Jun 11, 2018
@wingyplus @alexbrainman
syscall: update TOKEN_ALL_ACCESS according to WinNT.h
b74f732 
TOKEN_ALL_ACCESS was changed at some stage by Microsoft.

Updates #25775

Change-Id: I3e18914207a0020b2ebfb99f4e57aa55f9de813b
Reviewed-on: https://go-review.googlesource.com/117635
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/117815 mentions this issue: windows: update TOKEN_ALL_ACCESS according to WinNT.h

@golang golang locked and limited conversation to collaborators Jun 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
6 participants
@wingyplus @agnivade @ianlancetaylor @gopherbot @alexbrainman @limard