Code review - Issue 4280065: syscall: StartProcess Chroot and Credential.https://codereview.appspot.com/2011-03-29T18:29:30+00:00rietveld
Message from unknown
2011-03-24T12:13:39+00:00albert.strasheimurn:md5:c65a58c0070b6b171fe9b28cb9ccafe1
Message from unknown
2011-03-24T12:13:49+00:00albert.strasheimurn:md5:669ede782c1b71d9a41acf91ea8a0168
Message from fullung@gmail.com
2011-03-24T12:19:09+00:00albert.strasheimurn:md5:86b55a3da9fd7bfbc81bc57bbfaad692
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
Message from agl@golang.org
2011-03-24T13:56:28+00:00agl1urn:md5:84d7bac8f053c5a9c6621792004e5cad
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#newcode160
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#newcode262
src/pkg/syscall/exec_unix.go:262: Groups []uint32
I would add a comment expressing the need to set Groups equal to something.
Message from fullung@gmail.com
2011-03-24T14:02:03+00:00albert.strasheimurn:md5:c6f7223988cb86f2895c506696915e08
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#newcode160
> 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
Message from agl@golang.org
2011-03-24T14:05:45+00:00agl1urn:md5:39ef0230c24766dabe6689edd679d63c
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
Message from unknown
2011-03-24T15:06:11+00:00albert.strasheimurn:md5:344130ddb11d8251ea556db7f54d0738
Message from fullung@gmail.com
2011-03-24T15:09:29+00:00albert.strasheimurn:md5:b3f2c80a32356f2f168553c2ed7711d9
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#newcode160
> 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#newcode262
> 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.
Message from rsc@golang.org
2011-03-24T22:58:08+00:00rscurn:md5:62ebd288e74383c14d2721b7c15db10f
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.
Message from fullung@gmail.com
2011-03-25T05:04:00+00:00albert.strasheimurn:md5:ae545a3487df39eb99161a085e6d8ddc
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
Message from rsc@golang.org
2011-03-25T18:38:36+00:00rscurn:md5:0f0e1248d7f746f2571cdbe2bf463324
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
Message from fullung@gmail.com
2011-03-25T20:57:39+00:00albert.strasheimurn:md5:9a5b7841350599de9c2cb3624b49ebc0
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
Message from rsc@golang.org
2011-03-25T21:17:25+00:00rscurn:md5:fe0d23b3522807d35ba22ded4a4ae0d0
> 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
Message from unknown
2011-03-28T05:30:34+00:00albert.strasheimurn:md5:88c558a04aec76b8f6087fda33237846
Message from fullung@gmail.com
2011-03-28T05:33:14+00:00albert.strasheimurn:md5:0e60ba84ee667c1e4705f60f40e8e6a1
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.
Message from agl@golang.org
2011-03-29T14:54:30+00:00agl1urn:md5:348d0a74ef362d2e0f415b450d046c7d
LGTM. Will land if nobody else comments today.
Message from rsc@golang.org
2011-03-29T18:09:31+00:00rscurn:md5:5128b748734541625890a11769e9ab81
LGTM
Message from agl@golang.org
2011-03-29T18:29:30+00:00agl1urn:md5:af9655f76b8975a04bc787fd57a76566
*** 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>