syscall: StartProcess Chroot and Credential.

syscall: StartProcess Chroot and Credential.

[albert.strasheim, 2011-03-24 12:19:09]

Hello all

Please review this change.

I used the following test with Busybox installed in the chroot with a symlink
from /tmp/bin/id to the busybox binary.

func TestChrootCredentials(t *testing.T) {
  attr := syscall.ProcAttr{Dir: "/", Chroot: "/tmp", Files: []int{-1, 1,2},
Credentials: &syscall.Credentials{501, []uint32{501, 100}}}
  pid, _, errno := syscall.StartProcess("/bin/id", []string{"/bin/id"}, &attr)
  if errno != 0 {
    panic(os.Errno(errno))
  }
  _, err := os.Wait(pid, 0)
  if err != nil {
    panic(err)
  }
}

It seems one has to call both Setgid and Setgroups to get everything sorted out,
as discussed here:

http://www.palecrow.com/chroot-jail-paper.html

I used one Groups list to avoid doing any allocations.

Regards

Albert

[agl1, 2011-03-24 13:56:28]

http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:160: _, _, err1 = RawSyscall(SYS_SETGID,
uintptr(attr.Credentials.Groups[0]), 0, 0)
The question of what to do when no groups were specified is tough. It's
typically an error to setuid to non-root without also dealing with groups. I
would tend to play it safe and set GID = UID and clear all groups.

http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:262: Groups []uint32
I would add a comment expressing the need to set Groups equal to something.

[albert.strasheim, 2011-03-24 14:02:03]

Hello

On Thu, Mar 24, 2011 at 3:56 PM,  <agl@golang.org> wrote:
>
> http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go
> File src/pkg/syscall/exec_unix.go (right):
>
>
http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go#...
> src/pkg/syscall/exec_unix.go:160: _, _, err1 = RawSyscall(SYS_SETGID,
> uintptr(attr.Credentials.Groups[0]), 0, 0)
> The question of what to do when no groups were specified is tough. It's
> typically an error to setuid to non-root without also dealing with
> groups. I would tend to play it safe and set GID = UID and clear all
> groups.

By clear do you mean call setgroups(0, 0)?

Regards

Albert

[agl1, 2011-03-24 14:05:45]

On Thu, Mar 24, 2011 at 10:01 AM, Albert Strasheim <fullung@gmail.com> wrote:
> By clear do you mean call setgroups(0, 0)?

Yep.


AGL

[albert.strasheim, 2011-03-24 15:09:29]

PTAL

On 2011/03/24 13:56:28, agl1 wrote:
> http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go
> File src/pkg/syscall/exec_unix.go (right):
> 
>
http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go#...
> src/pkg/syscall/exec_unix.go:160: _, _, err1 = RawSyscall(SYS_SETGID,
> uintptr(attr.Credentials.Groups[0]), 0, 0)
> The question of what to do when no groups were specified is tough. It's
> typically an error to setuid to non-root without also dealing with groups. I
> would tend to play it safe and set GID = UID and clear all groups.

Done.

>
http://codereview.appspot.com/4280065/diff/1002/src/pkg/syscall/exec_unix.go#...
> src/pkg/syscall/exec_unix.go:262: Groups []uint32
> I would add a comment expressing the need to set Groups equal to something.

I've added a comment. Let me know if the style is okay.

[rsc, 2011-03-24 22:58:08]

s/Credentials/Credential/

i'd prefer a better name but none come to mind.
i think you need setgid and Gid too.
setgroups just sets the supplementary groups,
not the primary one.

[albert.strasheim, 2011-03-25 05:04:00]

Hello

On Fri, Mar 25, 2011 at 12:58 AM, Russ Cox <rsc@golang.org> wrote:
> s/Credentials/Credential/

I feel Credentials might be a better name, especially when dealing
with multiple groups.

Also, the manual page is called credentials.

But I'm happy to change it.

> i'd prefer a better name but none come to mind.
> i think you need setgid and Gid too.
> setgroups just sets the supplementary groups,
> not the primary one.

I currently call Setgid with the first group in the slice of groups.

The convention seems to be to have the "primary" set with Setgid and
included in the Setgroups list.

I could have a separate Gid field, but that makes it more difficult to
construct the argument that has to be passed to Setgroups.

Regards

Albert

[rsc, 2011-03-25 18:38:36]

Please change to Credential.  It is a single identity
even if it has multiple pieces.

If the groups list is empty I'm not comfortable
assuming that it is correct to use the uid as a gid.
They come from separate spaces.  A better choice
would be to use -1 (nobody), perhaps.

Russ

[albert.strasheim, 2011-03-25 20:57:39]

Hello

On Fri, Mar 25, 2011 at 8:38 PM, Russ Cox <rsc@golang.org> wrote:
> Please change to Credential.  It is a single identity
> even if it has multiple pieces.

Okay, understood.

> If the groups list is empty I'm not comfortable
> assuming that it is correct to use the uid as a gid.
> They come from separate spaces.  A better choice
> would be to use -1 (nobody), perhaps.

It seems nobody is 99 on at least Fedora (and therefore also on RHEL),
not 65534. Are we going to implement getpwnam to get this right? I'm
guessing not.

Maybe it should simply be an error to not specify at least one group.
That would suggest that Credential should have Uid, Gid and Groups
fields and that the function should build a new Groups slice that
includes the value of Group before forking.

Regards

Albert

[rsc, 2011-03-25 21:17:25]

> Maybe it should simply be an error to not specify at least one group.
> That would suggest that Credential should have Uid, Gid and Groups
> fields and that the function should build a new Groups slice that
> includes the value of Group before forking.

I think we should do whatever the client asks.
Is it really required to include the gid in the setgroups call?
I don't think it is.

Russ

[albert.strasheim, 2011-03-28 05:33:14]

PTAL.

On 2011/03/25 21:17:25, rsc wrote:
> > Maybe it should simply be an error to not specify at least one group.
> > That would suggest that Credential should have Uid, Gid and Groups
> > fields and that the function should build a new Groups slice that
> > includes the value of Group before forking.
> I think we should do whatever the client asks.
> Is it really required to include the gid in the setgroups call?
> I don't think it is.

Renamed to Credential and added a Gid field. No attempt is made to add Gid to
Groups.

The user now gets exactly what he/she ask for and Setuid, Setgid and Setgroups
are all called, always.

[agl1, 2011-03-29 14:54:30]

LGTM. Will land if nobody else comments today.

[rsc, 2011-03-29 18:09:31]

LGTM

[agl1, 2011-03-29 18:29:30]

*** Submitted as http://code.google.com/p/go/source/detail?r=a25343ee3016 ***

syscall: StartProcess Chroot and Credential.

R=rsc, iant, agl1
CC=golang-dev
http://codereview.appspot.com/4280065

Committer: Adam Langley <agl@golang.org>