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: exec_linux.go not using cross architecture safe SYS_SETGROUPS #17092

Closed
celledge opened this issue Sep 13, 2016 · 7 comments
Closed

syscall: exec_linux.go not using cross architecture safe SYS_SETGROUPS #17092

celledge opened this issue Sep 13, 2016 · 7 comments
Assignees
Milestone

Comments

@celledge
Copy link

@celledge celledge commented Sep 13, 2016

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

go 1.7.1

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

GOOS=linux
GOARCH=arm

What did you do?

groups = []uint32{20, 21, 22}
cmd.SysProcAttr.Credential = &syscall.Credential{Groups: groups}
cmd.Run()

Run strace on the process:
strace -e trace=setgroups -e trace=setgroups32 -f {executable}

What did you expect to see?

setgroups32(3, [20, 21, 22])

What did you see instead?

setgroups(3, [20, 0, 21])

Source of problem and possible fix

It appears that syscall/linux_exec.go line 217, the RawSyscall is using SYS_SETGROUPS which on linux/arm is the 16bit GID system call. Since golang always uses 32bit GIDs, this fails. I switched this statement to SYS_SETGROUPS32, and it worked fine on my linux/arm system.

Perhaps this RawSyscall should be replaced with syscall.setgroups so that the correct architecture dependent system call is used.

Please note that this is also true for the SYS_SETUID and SYS_SETGID syscalls just below this line as well. Those statements will operate correctly, unless a UID or GID >= 2^16 is used. In which case it will truncate the integer and use the wrong ID instead of giving an error. However, I do not see an architecture dependent function already implemented to replace those.

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Sep 13, 2016
@ianlancetaylor ianlancetaylor changed the title exec_linux.go not using cross architecture safe SYS_SETGROUPS syscall: exec_linux.go not using cross architecture safe SYS_SETGROUPS Sep 13, 2016
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 13, 2016

CC @LK4D4

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Sep 13, 2016

@ianlancetaylor This code was there before me :/ Looks pretty bad and I don't about policy in forkAndExecInChild - if it's possible to call functions instead of RawSyscall there. I hope there won't hack as with s390x SYS_CLONE :)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 13, 2016

@LK4D4 Oh, sorry, I just assumed it was your code.

The child process in forkAndExecInChild can call any function, as long as it is nosplit and does not allocate any memory.

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Sep 13, 2016

Just to be sure, here is function for arm:
https://github.com/golang/go/blob/master/src/syscall/zsyscall_linux_arm.go#L1263
Also, there are no functions for setuid and setgid. They need to be created in something like exec_linux_arm.go :(

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 19, 2016

CL https://golang.org/cl/31458 mentions this issue.

@gopherbot gopherbot closed this in 6c295a9 Oct 19, 2016
@celledge
Copy link
Author

@celledge celledge commented Nov 8, 2016

I went over the patch at https://golang.org/cl/31458
It does fix the SYS_SETGROUPS issue, but it doesn't address the SYS_SETUID and SYS_SETGID 16bit vs 32bit potential issue that I mentioned.

@bradfitz bradfitz reopened this Nov 9, 2016
@bradfitz bradfitz self-assigned this Nov 9, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 9, 2016

CL https://golang.org/cl/33011 mentions this issue.

@gopherbot gopherbot closed this in 22c70f2 Nov 9, 2016
@golang golang locked and limited conversation to collaborators Nov 9, 2017
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.