|
|
Created:
13 years, 1 month ago by albert.strasheim Modified:
13 years ago Reviewers:
CC:
rsc, iant, agl1, golang-dev Visibility:
Public. |
Descriptionsyscall: StartProcess Chroot and Credential.
Patch Set 1 #Patch Set 2 : diff -r 44e733716d02 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 3 : diff -r 44e733716d02 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 3807b738e9ff https://go.googlecode.com/hg/ #MessagesTotal messages: 14
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
> 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
Sign in to reply to this message.
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.
Sign in to reply to this message.
LGTM. Will land if nobody else comments today.
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|