x/sys/unix: Faccessat has unused flags parameter on Linux

[rudis]

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 additional flags 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.

[ianlancetaylor]

Hmmm, the C function faccessat takes four arguments. But I see that it seems to be implemented in glibc directly. So I agree that we should change x/sys/unix. Want to send a patch?

[tklauser]

The faccessat syscall on Darwin and FreeBSD (also NetBSD and OpenBSD, but these aren't implemented in x/sys/unix) takes a 4th flag argument and it would be nice to keep the Linux version of Faccessat compatible. Maybe change it to faccessat on Linux and only take 3 arguments and then add Faccessat as a wrapper taking 4 arguments which just ignores the 4th argument, i.e.

//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)
} 

[ianlancetaylor]

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

[rudis]

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.

[tklauser]

@rudis thanks for elaborating. Checking flags like in Fchmodat sounds good to me. Feel free to send a CL.

As for making the Fchmodat change in syscall: The package is frozen as per the comment in https://github.com/golang/go/blob/master/src/syscall/syscall.go#L21-L28, but occasional changes following the Go 1 compatibility promise are sometimes allowed. I'm not sure whether such a change would be acceptable, so l'll let @ianlancetaylor and @bradfitz decide.

[ianlancetaylor]

I'm OK with backporting the Fchmodat flags check to the syscall package.

[gopherbot]

Change https://golang.org/cl/118658 mentions this issue: syscall: check Fchmodat flags parameter on Linux

[gopherbot]

Change https://golang.org/cl/119495 mentions this issue: unix: check faccessat flags parameter on Linux