Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Al2Klimov opened this issue May 24, 2018 · 23 comments
Open

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

Al2Klimov opened this issue May 24, 2018 · 23 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Milestone

Comments

@Al2Klimov
Copy link

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 tklauser added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 24, 2018
@tklauser tklauser changed the title os.ModeSetgid has no effect while using with os.Mkdir() on Linux os: ModeSetgid has no effect while using with Mkdir() on Linux May 24, 2018
@tklauser
Copy link
Member

/cc @bradfitz @ianlancetaylor @rsc

@ianlancetaylor
Copy link
Contributor

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

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label May 24, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 24, 2018
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone May 24, 2018
@Al2Klimov
Copy link
Author

Al2Klimov commented May 25, 2018

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
Copy link
Member

odeke-em commented Jun 1, 2018

/cc @jessfraz @lizrice too

@jessfraz
Copy link
Contributor

jessfraz commented Jun 1, 2018

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

@dnsmichi
Copy link

dnsmichi commented Jun 1, 2018

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

@Al2Klimov
Copy link
Author

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

I forgot to do this one.

@gopherbot
Copy link

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

@paulzhol
Copy link
Member

paulzhol commented Jun 4, 2018

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
Copy link
Contributor

ianlancetaylor commented Jun 4, 2018

@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
Copy link
Member

paulzhol commented Jun 4, 2018

@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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Member

paulzhol commented Jun 5, 2018

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
Copy link
Member

paulzhol commented Jun 5, 2018

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Member

paulzhol commented Jun 6, 2018

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
Copy link

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
Copy link
Contributor

I guess this stalled out.

@G2G2G2G
Copy link

G2G2G2G commented Feb 15, 2021

PHP has the same issue for like 20 years, you have to set 02777 or the like separately. At least PHP's chmod equivalent works. In Go nothing works and you have to call the system's chmod to set a setgid bit lmao. Both seem to be issues from their copying of glibc or something.

niemeyer added a commit to canonical/chisel that referenced this issue Jun 1, 2022
Underlying issue is a can of worms:

golang/go#25539
@gshamov
Copy link

gshamov commented Aug 11, 2022

In go version go1.17.7 linux/amd64 it still does not work in os.Mkdir to have masks like 02755 .

Why on earth Golang choose not respect standard Linux behaviour, is hard to understand. Documentation of os.Mkdir or os.Chown is silent about the issue as well, so it is a surprise to discover.

One would naively wish a module called "os" to support ALL of each OS functionality its platform, rather than a minimal common subset of functionalities that can be found between each and every OS that there is.

@G2G2G2G
Copy link

G2G2G2G commented Aug 12, 2022

@gshamov update your Go

This isn't in many languages because it requires 2 things, first making the folder then setting the permissions... it isn't possible to do both at once. which to most of us doesn't matter to 99% of us, apparently it does to them.
They should just do both of those things or not even have the option for permissions on their mkdir so nobody expects anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Projects
None yet
Development

Successfully merging a pull request may close this issue.