-
Notifications
You must be signed in to change notification settings - Fork 17.3k
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
x/sys/unix: Faccessat has unused flags parameter on Linux #25845
Comments
Hmmm, the C function |
The //sys faccessat(dirfd int, path string, mode uint32) (err error)
func Faccessat(dirfd int, path string, mode uint32, flags int) (err error) {
return faccessat(dirfd, path, mode)
} |
Sure, we could do it that way. The glibc code is at https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/faccessat.c;h=ea42b2303ff4b2d2d6548ea04376fb265f773436;hb=HEAD |
On Tue, Jun 12, 2018 at 02:14:11PM -0700, Tobias Klauser wrote:
`faccessat` on Darwin and FreeBSD takes a 4th `flag` argument and it would be nice to keep the Linux-version compatible. Maybe change the `faccessat` syscall wrapper for Linux to only take 3 arguments and don't export it and then add `Faccessat` taking 4 arguments which just ignores the 4th argument, i.e.
```go
//sys faccessat(dirfd int, path string, mode uint32) (err error)
func Faccessat(dirfd int, path string, mode uint32, flags int) (err error) {
return faccessat(dirfd, path, mode)
}
```
Please don't just ignore `flags` without telling the user! As I
wrote in the original report this can be a security issue and is
even more confusing to the user.
Or maybe do the same thing glibc does with the `flag` argument?
An alternative would be to disallow the flag argument like it's
currently done for `Fchmodat()`, see
src/cmd/vendor/golang.org/x/sys/unix/syscall_linux.go or commit
c23410a ("unix: check Fchmodat
flags parameter on Linux", 2017-06-23) in the golang/sys.git
repository. However it looks like this change wasn't merged into
syscall/syscall_linux.go - should I open a separate bug for this?
```go
//sys fchmodat(dirfd int, path string, mode uint32) (err error)
func Fchmodat(dirfd int, path string, mode uint32, flags int) (err error) {
// Linux fchmodat doesn't support the flags parameter. Mimick glibc's behavior
// and check the flags. Otherwise the mode would be applied to the symlink
// destination which is not what the user expects.
if flags&^AT_SYMLINK_NOFOLLOW != 0 {
return EINVAL
} else if flags&AT_SYMLINK_NOFOLLOW != 0 {
return EOPNOTSUPP
}
return fchmodat(dirfd, path, mode)
}
```
That's simpler and to me the preferred method as I think of
sys/unix as a thin layer over the syscalls of the system and not
a compatibility layer. I can prefer a patch for this you like.
|
@rudis thanks for elaborating. Checking flags like in As for making the |
I'm OK with backporting the |
Change https://golang.org/cl/118658 mentions this issue: |
As mentioned in #25845, port CL 46474 from golang.org/x/sys/unix to the syscall package. Currently Linux' fchmodat(2) syscall implementation doesn't support the flags parameter (though it might in future versions [1]). Fchmodat in the syscall package takes the parameter and (wrongly) passes it on to the syscall which will ignore it. According to the POSIX.1-2008 manual page [2], AT_SYMLINK_NOFOLLOW is the only valid value for the flags parameter and EOPNOTSUPP should be returned in case changing the mode of a symbolic link is not supported by the underlying system. EINVAL should be returned for any other value of the flags parameter. [1] https://patchwork.kernel.org/patch/9596301/ [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html Updates #20130 Updates #25845 Change-Id: I1021dd0e6a4f4cb3557cb1c1b34dd618c378cda6 Reviewed-on: https://go-review.googlesource.com/118658 Reviewed-by: Ian Lance Taylor <iant@golang.org>
As mentioned in golang#25845, port CL 46474 from golang.org/x/sys/unix to the syscall package. Currently Linux' fchmodat(2) syscall implementation doesn't support the flags parameter (though it might in future versions [1]). Fchmodat in the syscall package takes the parameter and (wrongly) passes it on to the syscall which will ignore it. According to the POSIX.1-2008 manual page [2], AT_SYMLINK_NOFOLLOW is the only valid value for the flags parameter and EOPNOTSUPP should be returned in case changing the mode of a symbolic link is not supported by the underlying system. EINVAL should be returned for any other value of the flags parameter. [1] https://patchwork.kernel.org/patch/9596301/ [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html Updates golang#20130 Updates golang#25845 Change-Id: I1021dd0e6a4f4cb3557cb1c1b34dd618c378cda6 Reviewed-on: https://go-review.googlesource.com/118658 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Change https://golang.org/cl/119495 mentions this issue: |
Change https://golang.org/cl/120015 mentions this issue: |
@ianlancetaylor @tklauser Should we need to port CL120015 back to syscall package? |
@wingyplus Thanks, I sent https://golang.org/cl/120015 doing that and let @ianlancetaylor decide whether that changeis acceptable for |
Port CL 119495 from golang.org/x/sys/unix to the syscall package. Currently Linux faccessat(2) syscall implementation doesn't support the flags parameter. As per the discussion in #25845, permit the same flags as glibc [1]. [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/faccessat.c;h=ea42b2303ff4b2d2d6548ea04376fb265f773436;hb=HEAD Updates #25845 Change-Id: I132b33275a9cc72b3a97acea5482806c7f47d7f7 Reviewed-on: https://go-review.googlesource.com/120015 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
@tklauser Thanks |
The
faccessat(2)
syscall on Linux takes three arguments (see https://github.com/torvalds/linux/blob/8efcf34a263965e471e3999904f94d1f6799d42a/fs/open.c#L434), but Go's signature has an additionalflags
argument://sys Faccessat(dirfd int, path string, mode uint32, flags int) (err error)
This is problematic as the extra argument is ignored by the kernel but there's no indication in the source code/during compilation. As the only possible argument
AT_SYMLINK_NOFOLLOW
can be used to prevent following symlinks this omission might have a security impact.The text was updated successfully, but these errors were encountered: