Permalink
Browse files

syscall: only call setgroups if we need to

If the caller set ups a Credential in os/exec.Command,
os/exec.Command.Start will end up calling setgroups(2), even if no
supplementary groups were given.

Only root can call setgroups(2) on BSD kernels, which causes Start to
fail for non-root users when they try to set uid and gid for the new
process.

We fix by introducing a new field to syscall.Credential named
NoSetGroups, and setgroups(2) is only called if it is false.
We make this field with inverted logic to preserve backward
compatibility.

RELNOTES=yes

Change-Id: I3cff1f21c117a1430834f640ef21fd4e87e06804
Reviewed-on: https://go-review.googlesource.com/36697
Reviewed-by: Ian Lance Taylor <iant@golang.org>
  • Loading branch information...
walac authored and ianlancetaylor committed Feb 10, 2017
1 parent 708ba22 commit 79f6a5c7bd684f2e6007ee505b522440beb86bf0
Showing with 60 additions and 13 deletions.
  1. +45 −0 src/os/exec/exec_posix_test.go
  2. +5 −3 src/syscall/exec_bsd.go
  3. +1 −4 src/syscall/exec_linux.go
  4. +5 −3 src/syscall/exec_solaris.go
  5. +4 −3 src/syscall/exec_unix.go
@@ -0,0 +1,45 @@
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build darwin dragonfly freebsd linux netbsd openbsd solaris
+
+package exec_test
+
+import (
+ "os/user"
+ "strconv"
+ "syscall"
+ "testing"
+)
+
+func TestCredentialNoSetGroups(t *testing.T) {
+ u, err := user.Current()
+ if err != nil {
+ t.Fatalf("error getting current user: %v", err)
+ }
+
+ uid, err := strconv.Atoi(u.Uid)
+ if err != nil {
+ t.Fatalf("error converting Uid=%s to integer: %v", u.Uid, err)
+ }
+
+ gid, err := strconv.Atoi(u.Gid)
+ if err != nil {
+ t.Fatalf("error converting Gid=%s to integer: %v", u.Gid, err)
+ }
+
+ // If NoSetGroups is true, setgroups isn't called and cmd.Run should succeed
+ cmd := helperCommand(t, "echo", "foo")
+ cmd.SysProcAttr = &syscall.SysProcAttr{
+ Credential: &syscall.Credential{
+ Uid: uint32(uid),
+ Gid: uint32(gid),
+ NoSetGroups: true,
+ },
+ }
+
+ if err = cmd.Run(); err != nil {
+ t.Errorf("Failed to run command: %v", err)
+ }
+}
View
@@ -146,9 +146,11 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr
if ngroups > 0 {
groups = uintptr(unsafe.Pointer(&cred.Groups[0]))
}
- _, _, err1 = RawSyscall(SYS_SETGROUPS, ngroups, groups, 0)
- if err1 != 0 {
- goto childerror
+ if !cred.NoSetGroups {
+ _, _, err1 = RawSyscall(SYS_SETGROUPS, ngroups, groups, 0)
+ if err1 != 0 {
+ goto childerror
+ }
}
_, _, err1 = RawSyscall(SYS_SETGID, uintptr(cred.Gid), 0, 0)
if err1 != 0 {
@@ -210,10 +210,7 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr
if ngroups > 0 {
groups = uintptr(unsafe.Pointer(&cred.Groups[0]))
}
- // Don't call setgroups in case of user namespace, gid mappings
- // and disabled setgroups, because otherwise unprivileged user namespace
- // will fail with any non-empty SysProcAttr.Credential.
- if !(sys.GidMappings != nil && !sys.GidMappingsEnableSetgroups && ngroups == 0) {
+ if !(sys.GidMappings != nil && !sys.GidMappingsEnableSetgroups && ngroups == 0) && !cred.NoSetGroups {
_, _, err1 = RawSyscall(_SYS_setgroups, ngroups, groups, 0)
if err1 != 0 {
goto childerror
@@ -143,9 +143,11 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr
if ngroups > 0 {
groups = uintptr(unsafe.Pointer(&cred.Groups[0]))
}
- err1 = setgroups1(ngroups, groups)
- if err1 != 0 {
- goto childerror
+ if !cred.NoSetGroups {
+ err1 = setgroups1(ngroups, groups)
+ if err1 != 0 {
+ goto childerror
+ }
}
err1 = setgid(uintptr(cred.Gid))
if err1 != 0 {
View
@@ -112,9 +112,10 @@ func SetNonblock(fd int, nonblocking bool) (err error) {
// Credential holds user and group identities to be assumed
// by a child process started by StartProcess.
type Credential struct {
- Uid uint32 // User ID.
- Gid uint32 // Group ID.
- Groups []uint32 // Supplementary group IDs.
+ Uid uint32 // User ID.
+ Gid uint32 // Group ID.
+ Groups []uint32 // Supplementary group IDs.
+ NoSetGroups bool // If true, don't set supplementary groups
}
// ProcAttr holds attributes that will be applied to a new process started

0 comments on commit 79f6a5c

Please sign in to comment.