Go: os: ModeSetgid has no effect while using with Mkdir() on Linux

[Al2Klimov]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.9.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root/go"
GORACE=""
GOROOT="/usr/lib/golang"
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build203196509=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

package main

import (
    "os"
)

func main() {
    os.Mkdir("test", 0770 | os.ModeSetgid)
}

What did you expect to see?

# ls -la test
insgesamt 4
drwxr-s---.  2 root root    6 24. Mai 04:09 .
dr-xr-x---. 13 root root 4096 24. Mai 04:09 ..
#

What did you see instead?

# ls -la test
insgesamt 4
drwxr-x---.  2 root root    6 24. Mai 04:09 .
dr-xr-x---. 13 root root 4096 24. Mai 04:09 ..
#

Why did this happen?

According to strace -f ./mkdir the Go stdlib behaves as expected...

[pid  6782] mkdirat(AT_FDCWD, "test", 02770 <unfinished ...>
[pid  6782] <... mkdirat resumed> )     = 0

... but on Linux this is not enough, see mkdirat(2):

The mkdirat() system call operates in exactly the same way as mkdir(2), (...)

... and mkdir(2):

The  argument  mode  specifies the permissions to use.
It is modified (...): the permissions of the created directory are (mode & ~umask & 0777).
Other mode bits of the created directory depend on the operating system.
For Linux, see below.
(...)
That is, under Linux the created directory actually gets mode (mode & ~umask & 01777)

How could this be fixed?

Similar to #8383, via Chmod.

[tklauser]

/cc @bradfitz @ianlancetaylor @rsc

[ianlancetaylor]

Seems that we should fix that along the lines that you point out. Want to send a patch?

[Al2Klimov]

I'm afraid I have to correct myself: It's not just Linux, it seems to be all *nix.

Discovered this by running this on my Mac workstation:

#include <sys/stat.h>

int main() {
    mkdir("test", 02770);
}

vs.

#include <sys/stat.h>

int main() {
    mkdir("test", 02770);
    chmod("test", 02770);
}

[odeke-em]

/cc @jessfraz @lizrice too

[jessfraz]

Dope, if you don't want to send a patch I can make one, just let me know

[dnsmichi]

@jessfraz Thanks. Haven't tested the linked PR yet, I'll talk to @Al2Klimov in terms of signing the CLA on Monday :)

[Al2Klimov]

As discussed w/ @dnsmichi (offline): I've just overseen an elephant:

I forgot to do this one.

[gopherbot]

Change https://golang.org/cl/116075 mentions this issue: os.Mkdir(): respect setuid and setgid bit on *nix

[paulzhol]

I would like to make a motion that this is not a bug, but the expected behavior and therefore should not be fixed.
If you treat the os package as a similar layer of what libc provides on unix systems - a thin wrapper around syscall invocations. Then having os.Mkdir perform an additional chmod operation would be a surprise to quite a few people.
Currently there is special treatment for handling ModeSticky as Linux accepts that bit as part of the mkdir syscall while BSDs don't. Note that on Linux an os.Mkdir operation with ModeSticky is one atomic operation, it either succeeds or it fails. On BSDs the directory can be created but the chmod could fail. There's even a pending CL to test for that and remove the directory.

With the proposed ModeSetuid/ModeSetgid change, as no unix system supports this at the syscall or libc level. It constitutes a new (unexpected in my opinion) behavior.

[ianlancetaylor]

@paulzhol I think the counter argument is that passing these bits in the mode argument to os.Mkdir has a clear and unambiguous meaning. I see two possible surprises here, and we must pick one or the other:

1. People familiar with the libc behavior, who for some reason choose to pass ModeSetuid and/or ModeSetgid to os.Mkdir, and are then surprised that the bits are not ignored.
2. People unfamiliar with the libc behavior, who pass ModeSetuid and/or ModeSetgid to os.Mkdir and are then surprised that the bits are ignored.

I don't see any particular reason to require Go programmers to be familiar with libc behavior. And I don't see any particular reason why people familiar with the libc behavior would choose to pass ModeSetuid and/or ModeSetgid to os.Mkdir. So on balance I think I would would prefer to pick surprise number 1, and go ahead and make this change.

[paulzhol]

@ianlancetaylor I would argue that the current documentation states one should be passing only permission bits:

func Mkdir(name string, perm FileMode) error
Mkdir creates a new directory with the specified name and permission bits (before umask). If there is an error, it will be of type *PathError.

Note the use of perm as the parameter name and the phrase permission bits in the doc comment.
Together with the split definition of FileMode:

type FileMode
A FileMode represents a file's mode and permission bits... Not all bits apply to all systems. The only required bit is ModeDir for directories.

ModePerm FileMode = 0777 // Unix permission bits`

func (m FileMode) Perm() FileMode
Perm returns the Unix permission bits in m.

I suggest the documentation should be updated to state the special handling of ModeSticky and that all other non-permission bits are to be ignored, no familiarity with libc will be required and surprise no.2 avoided.

[Al2Klimov]

LibC? I thought I was programming in Go... 😕

Seriously, I has programmed in C/POSIX and I've expected Go to handle 04755, but (surprise, surprise) it handles 040000755 and only 040000755. 04755 has literally no effect compared to 0755 – whether libc/kernel would handle it or not. 07000 gets taken away and replaced w/ the bits from 070000000.

TL;DR

Go's os package seems not to be bound to any libc constraints – and IMO shouldn't be.
If I can pass FileMode, I can pass setuid, setgid and sticky, too.
And Mkdir() already handles sticky – why not to handle the others, too?

[ianlancetaylor]

@paulzhol I agree that we could document the restriction, but that just changes the surprise from the person writing the code to the person reading the documentation. That is better, but it still seems odd. After all, there isn't any fundamental reason that it shouldn't work.

[paulzhol]

If I can pass FileMode, I can pass setuid, setgid and sticky, too.
And Mkdir() already handles sticky – why not to handle the others, too?

You can pass a FileMode param but you should be setting only the permission bits, i.e

The nine least-significant bits are the standard Unix rwxrwxrwx permissions

(the quote above is from the type FileMode documentation).

Because the os.Mkdir states you should be passing only the permission bits, and these bits are affected by the process' umask.
This means you need to be aware of what a umask is, and how it interacts with said permission bits (umask only affects the lower 9 bits as well) and generally be aware of the Unix OS interface after which package os was modeled.

os.Mkdir handles ModeSticky the way it does because there is one widely popular operation system called Linux that has a different behavior than the other Unix OSs' and the Go developers (I guess) have tried to provide a compatible interface (and I think it should have been explicitly documented how ModeSticky is handled).

To emphasize my point, what about os.OpenFile? it also accepts a FileMode argument (documented that it should contain permissions only before being affected by the umask).
Why not make OpenFile create a character device if I set ModeDevice and ModeCharDevice? Or maybe call mkfifo if ModeNamedPipeis set?

The interface of a method is not just it's signature, but also the accompanied doc comment, you can't ignore one or the other.

[paulzhol]

@ianlancetaylor The restriction is already documented (maybe not as clearly as we'd like).
As to reasons for it not to work is that with ModeSticky, OpenFile will not report the chmod failing even if it did. This is probably OK because ModeSticky doesn't really have any effect on regular files, whileMkdir will now try to delete the created directory.

What happens when you add ModeSetuid to the mix? This is no longer benign as with the ModeSticky. If the user would need to Stat the resulting file and check if the chmod succeeded, he might as well call the Chmod himself.
Maybe OpenFile should try to delete the file, but failing the delete it should inform the caller somehow.
These are all corner cases handled by the kernel in the syscall, emulating them will make everyone unhappy with the outcome.

[ianlancetaylor]

Note that as of earlier today the code does now check whether the chmod succeeded for ModeSticky.

I'm still not really seeing the real problem with handling ModeSetuid. Frankly, none of the problems you've mentioned seem significant to me.

I do now think that we should be checking for mode bits we aren't going to handle and returning an error if we see any.

[Al2Klimov]

os.Mkdir handles ModeSticky the way it does because there is one widely popular operation system called Linux that has a different behavior (...)

There is another (IMO even more) popular OS called MacOS X that has a different behavior than Linux. (mode & 0777 & ~umask)
To me Go's special handling looks more like making things easier for the developers (e.g. on MacOS) w/ the sticky bit.
I see literally no reason why Set{u,g}id shouldn't be also handled the same way as the sticky bit.
Go is kinda OS-agnostic after all, isn't it?

[paulzhol]

I do now think that we should be checking for mode bits we aren't going to handle and returning an error if we see any.

That makes sense.

Note that as of earlier today the code does now check whether the chmod succeeded for ModeSticky.

I'm aware of it (I was a reviewer) and I have been trying to explain there will be an inconsistency if we treat ModeSetuid in the same way as ModeSticky:

1. OpenFile (with O_CREATE) ignores chmod failures
2. Mkdir reports chmod failures and tries to cleanup after itself

Behaviour 1 is probably valid for ModeSticky because the sticky bit has no real effect on a regular file, I don't think it's valid for ModeSetuid.
Behaviour 2 is trying to follow the Linux "syscall semantic" for ModeSticky, which is fine but there is no "syscall semantic" for ModeSetuid, because no system has this behavior (you seem to have no problem with that, to me it feels wrong).

[justinfx]

Was curious if there is any movement on this issue? One reason it would be useful to have this is in combination with os.MkdirAll(), where you don't know how many intermediate directories will be created and then would have to do an extra chmod for each one to apply the setgid bit. If it were part of the Mkdir() then it would just happen as needed when a directory is really created.

[ianlancetaylor]

I guess this stalled out.