code review 106170043: syscall: disable Setuid/Setgid on linux

syscall: disable Setuid/Setgid on linux

Update issue 1435

This proposal disables Setuid and Setgid on all linux platforms.

Issue 1435 has been open for a long time, and it is unlikely to be addressed soon so an argument was made by a commenter

https://code.google.com/p/go/issues/detail?id=1435#c45

That these functions should made to fail rather than succeed in their broken state.

[dfc, 2014-06-23 00:03:00]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[ruiu, 2014-06-23 00:11:47]

https://codereview.appspot.com/106170043/diff/40001/src/pkg/syscall/syscall_l...
File src/pkg/syscall/syscall_linux.go (right):

https://codereview.appspot.com/106170043/diff/40001/src/pkg/syscall/syscall_l...
src/pkg/syscall/syscall_linux.go:849: // that the call succeeded.
That wording is slightly misleading as Linux kernel's syscalls behave reliably
but uid/gid change takes effect per-thread basis. And that semantics does not
match with what we expect.

[dfc, 2014-06-23 00:19:52]

Hello iant@golang.org, ruiu@google.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[dfc, 2014-06-23 00:20:15]

Hello iant@golang.org, ruiu@google.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[ruiu, 2014-06-23 00:26:21]

LGTM but please wait for Ian.

[dfc, 2014-06-23 00:27:49]

Of course, this one might be contentious.

On Mon, Jun 23, 2014 at 10:26 AM,  <ruiu@google.com> wrote:
> LGTM but please wait for Ian.
>
> https://codereview.appspot.com/106170043/

[iant, 2014-06-23 03:40:53]

I think we may as well pull Setgid from syscall_linux_xx.go into
syscall_linux.go.  If we ever do a real implementation, it should be in
syscall_linux.go anyhow.

https://codereview.appspot.com/106170043/diff/80001/src/pkg/syscall/syscall_l...
File src/pkg/syscall/syscall_linux_arm.go (left):

https://codereview.appspot.com/106170043/diff/80001/src/pkg/syscall/syscall_l...
src/pkg/syscall/syscall_linux_arm.go:62: //sysnb	Getgid() (gid int) =
SYS_GETGID32
You seem to have replaced Getgid here.

[dfc, 2014-06-23 03:44:22]

Sorry, that was my mistake, copying and pasting into three different files.

I'll follow your advice and move both stubs into syscall_linux.go

On Mon, Jun 23, 2014 at 1:40 PM,  <iant@golang.org> wrote:
> I think we may as well pull Setgid from syscall_linux_xx.go into
> syscall_linux.go.  If we ever do a real implementation, it should be in
> syscall_linux.go anyhow.
>
>
>
https://codereview.appspot.com/106170043/diff/80001/src/pkg/syscall/syscall_l...
> File src/pkg/syscall/syscall_linux_arm.go (left):
>
>
https://codereview.appspot.com/106170043/diff/80001/src/pkg/syscall/syscall_l...
> src/pkg/syscall/syscall_linux_arm.go:62: //sysnb        Getgid() (gid int) =
> SYS_GETGID32
> You seem to have replaced Getgid here.
>
> https://codereview.appspot.com/106170043/

[dfc, 2014-06-23 03:49:56]

PTAL.

[iant, 2014-06-23 21:43:30]

https://codereview.appspot.com/106170043/diff/80002/src/pkg/syscall/syscall_l...
File src/pkg/syscall/syscall_linux.go (right):

https://codereview.appspot.com/106170043/diff/80002/src/pkg/syscall/syscall_l...
src/pkg/syscall/syscall_linux.go:858: //sysnb	Setregid(rgid int, egid int) (err
error)
Looks like Setregid came through also, which looks wrong--it's still in the arch
variants.

[dfc, 2014-06-23 21:49:42]

PTAL. I hope this is the last time.

https://codereview.appspot.com/106170043/diff/80002/src/pkg/syscall/syscall_l...
File src/pkg/syscall/syscall_linux.go (right):

https://codereview.appspot.com/106170043/diff/80002/src/pkg/syscall/syscall_l...
src/pkg/syscall/syscall_linux.go:858: //sysnb	Setregid(rgid int, egid int) (err
error)
On 2014/06/23 21:43:30, iant wrote:
> Looks like Setregid came through also, which looks wrong--it's still in the
arch
> variants.

sigh. I'm not sure why this simple task is causing me so much difficulty.

[iant, 2014-06-23 22:52:00]

LGTM

Thanks.

It's always the simple ones that get you.

[dfc, 2014-06-23 23:06:17]

Thanks Ian.

Should I wait for anyone else or are you happy to see this land ?

On Tue, Jun 24, 2014 at 8:51 AM,  <iant@golang.org> wrote:
> LGTM
>
> Thanks.
>
> It's always the simple ones that get you.
>
>
> https://codereview.appspot.com/106170043/

[iant, 2014-06-23 23:15:06]

You can commit.

Ian

On Mon, Jun 23, 2014 at 4:06 PM, Dave Cheney <dave@cheney.net> wrote:
> Thanks Ian.
>
> Should I wait for anyone else or are you happy to see this land ?
>
> On Tue, Jun 24, 2014 at 8:51 AM,  <iant@golang.org> wrote:
>> LGTM
>>
>> Thanks.
>>
>> It's always the simple ones that get you.
>>
>>
>> https://codereview.appspot.com/106170043/

[dfc, 2014-06-23 23:16:39]

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

syscall: disable Setuid/Setgid on linux

Update issue 1435

This proposal disables Setuid and Setgid on all linux platforms.

Issue 1435 has been open for a long time, and it is unlikely to be addressed
soon so an argument was made by a commenter

https://code.google.com/p/go/issues/detail?id=1435#c45

That these functions should made to fail rather than succeed in their broken
state.

LGTM=ruiu, iant
R=iant, ruiu
CC=golang-codereviews
https://codereview.appspot.com/106170043

[gobot, 2014-06-23 23:31:22]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/01167d7336a18b40ef7760ca3271f6b203f16b6c

[dfc, 2014-06-23 23:42:22]

A falsehood!

# GOMAXPROCS=2 runtime -cpu=1,2,4
--- FAIL: TestFinalizerType-4 (5.63s)
mfinal_test.go:61: finalizer for type func(*int) didn't run
FAIL
FAIL runtime 201.017s
Build complete, duration 14m4.66624244s. Result: error: exit status 1

On Tue, Jun 24, 2014 at 9:31 AM,  <gobot@golang.org> wrote:
> This CL appears to have broken the netbsd-amd64-bsiegert builder.
> See http://build.golang.org/log/01167d7336a18b40ef7760ca3271f6b203f16b6c
>
> https://codereview.appspot.com/106170043/

[rsc, 2014-06-26 15:07:01]

Should these be removed instead?

[iant, 2014-06-26 16:30:18]

On Thu, Jun 26, 2014 at 8:07 AM,  <rsc@golang.org> wrote:
> Should these be removed instead?

I guess it's a question of which we think will break the API more.
Right now you can at least compile code that uses syscall.Setuid, and
it will work on most systems even if it manages to fail on GNU/Linux.

Ian

[bradfitz, 2014-06-26 16:31:26]

Yeah I say keep them. Fewer api exceptions. And maybe it'll be fixed in the
future.
 On Jun 26, 2014 9:30 AM, "Ian Lance Taylor" <iant@golang.org> wrote:

> On Thu, Jun 26, 2014 at 8:07 AM,  <rsc@golang.org> wrote:
> > Should these be removed instead?
>
> I guess it's a question of which we think will break the API more.
> Right now you can at least compile code that uses syscall.Setuid, and
> it will work on most systems even if it manages to fail on GNU/Linux.
>
> Ian
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>