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: supplementary groups are not cleared #15865

Closed
aronatkins opened this issue May 27, 2016 · 13 comments
Closed

syscall: supplementary groups are not cleared #15865

aronatkins opened this issue May 27, 2016 · 13 comments
Milestone

Comments

@aronatkins
Copy link

@aronatkins aronatkins commented May 27, 2016

With Go 1.6, a call to setgroups happens under fewer conditions, meaning supplementary groups may not be cleared.

  1. What version of Go are you using (go version)?
    go version go1.6.2 linux/amd64
  2. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH=""
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"

Further system/kernel information:

$ uname -a
Linux vagrant-ubuntu-trusty-64 3.13.0-86-generic #131-Ubuntu SMP Thu May 12 23:33:13 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
  1. What did you do?
package main

import (
    "log"
    "os"
    "os/exec"
    "os/user"
    "strconv"
    "syscall"
)

func main() {
    username := os.Args[1]

    u, err := user.Lookup(username)
    if err != nil {
        log.Fatal(err)
    }

    uid, err := strconv.Atoi(u.Uid)
    gid, err := strconv.Atoi(u.Gid)

    cmd := exec.Command(os.Args[2], os.Args[3:]...)
    cmd.Stdout = os.Stdout
    cmd.Stderr = os.Stderr
    cmd.Stdin = os.Stdin
    cmd.SysProcAttr = &syscall.SysProcAttr{
        Pdeathsig: syscall.SIGKILL,
    }
    cmd.SysProcAttr.Credential = &syscall.Credential{
        Uid: uint32(uid),
        Gid: uint32(gid),
        // Groups: []uint32{uint32(gid)},
    }
    if err := cmd.Run(); err != nil {
        log.Fatal(err)
    }
}
  1. What did you expect to see?

Running with Go 1.4.3 (as root):

#./foo username /bin/bash
$ id
uid=999(username) gid=999(username) groups=999(username)
  1. What did you see instead?

Running with Go 1.6.2 (as root):

# ./foo username /bin/bash
$ id
uid=999(username) gid=999(username) groups=999(username),0(root)

With Go 1.4, exec.Cmd ends up calling setgroups even with an empty syscall.Credential.Groups.

Workaround: Specify a non-empty syscall.Credential.Groups where the single element matches the specified syscall.Credential.Gid.

This behavior was altered with the change:
https://go-review.googlesource.com/#/c/13938/

@LK4D4

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented May 27, 2016

Yup, it's confusing. I don't know what we should do by default though. Should we always clear them or only if requested? For example if syscall.Credential.Groups != nil.

@ianlancetaylor ianlancetaylor changed the title Supplementary groups are not cleared syscall: supplementary groups are not cleared May 27, 2016
@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone May 27, 2016
@aronatkins
Copy link
Author

@aronatkins aronatkins commented May 27, 2016

After more digging: This exact matter was discussed in https://codereview.appspot.com/4280065 -- the call to setgroups to clear supplementary groups was intentional.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 27, 2016

Hmmm, it looks like exec_linux.go and exec_bsd.go behave differently. exec_bsd.go always calls setgroups if Credential is not nil, but exec_linux.go only calls setgroups if Credential.Groups has at least one element. That does not seem right.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 27, 2016

I see, it changed for exec_linux.go only in https://golang.org/cl/13938 because passing 0 groups to the setgroups system call doesn't change anything.

Should we close this issue or is there something to do?

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented May 27, 2016

@ianlancetaylor looks like it changes something after all. Maybe we should just revert that patch.
I'm sorry, I'm misread man setgroups. That patch is wrong, it should be reverted.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 27, 2016

Would it work to simply revert 13938? Don't we need to fiddle with writeUidGidMappings in some way so that the test continues to work?

@aronatkins
Copy link
Author

@aronatkins aronatkins commented May 27, 2016

https://go-review.googlesource.com/#/c/10670/ is the uid/gid mapping change if that helps...

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented May 27, 2016

@ianlancetaylor let me try, also I think test is needed also

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented May 27, 2016

@ianlancetaylor I think it's better to revert it and think about solution for mappings later :/
EDIT: I'll try to make patch now

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 27, 2016

The change was already in Go 1.6. We have apparently broken programs that worked in Go 1.4, and we want to fix those. But we don't want to thereby break programs that are working today in Go 1.6.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.7Maybe, Go1.8 May 27, 2016
@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented May 27, 2016

@ianlancetaylor yeah, I think it's possible

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented May 27, 2016

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 9, 2016

@golang golang locked and limited conversation to collaborators Jun 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
4 participants
You can’t perform that action at this time.