x/sys/unix: Faccessat has unused flags parameter on Linux #25845
Comments
|
Hmmm, the C function |
ianlancetaylor
added
the
NeedsFix
|
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)
} |
tklauser
changed the title
|
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 additionalflagsargument://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_NOFOLLOWcan be used to prevent following symlinks this omission might have a security impact.The text was updated successfully, but these errors were encountered: