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

syscall: Faccessat checks wrong group #52313

Closed
neild opened this issue Apr 12, 2022 · 10 comments
Closed

syscall: Faccessat checks wrong group #52313

neild opened this issue Apr 12, 2022 · 10 comments
Labels
NeedsFix release-blocker Security
Milestone

Comments

@neild
Copy link
Contributor

@neild neild commented Apr 12, 2022

The syscall.Faccessat function checks whether the calling process can access a file.

Faccessat contains a bug where it checks a file's group permission bits if the process's user is a member of the process's group rather than a member of the file's group.

if uint32(gid) == st.Gid || isGroupMember(gid) {

	var fmode uint32
	if uint32(uid) == st.Uid {
		fmode = (st.Mode >> 6) & 7
	} else {
		var gid int
		if flags&_AT_EACCESS != 0 {
			gid = Getegid()
		} else {
			gid = Getgid()
		}

		if uint32(gid) == st.Gid || isGroupMember(gid) { // <-- this should be isGroupMember(st.Gid), not gid
			fmode = (st.Mode >> 3) & 7
		} else {
			fmode = st.Mode & 7
		}
	}

Since a process's user is usually a member of the process's group, this causes Faccessat to usually check a file's group permissions even if the process's user is not a member of the file's group.

Thanks to @256dpi for reporting this.

@ianlancetaylor ianlancetaylor self-assigned this Apr 12, 2022
@neild neild assigned neild and unassigned ianlancetaylor Apr 12, 2022
@neild
Copy link
Contributor Author

@neild neild commented Apr 12, 2022

This bug only occurs on Linux systems, and when syscall.Faccessat is called with a non-zero flags parameter.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2022

Change https://go.dev/cl/399539 mentions this issue: syscall: check correct group in Faccessat

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2022

Change https://go.dev/cl/400074 mentions this issue: unix: check correct group in Faccessat

@neild
Copy link
Contributor Author

@neild neild commented Apr 12, 2022

"golang.org/x/sys/unix".Faccessat suffers from the same problem, but only on Linux kernels < 5.8.

gopherbot pushed a commit to golang/sys that referenced this issue Apr 12, 2022
The Faccessat call checks the user, group, or other permission bits of a
file to see if the calling process can access it. The test to see if the
group permissions should be used was made with the wrong group id, using
the process's group id rather than the file's group id. Fix this to use
the correct group id.

This change only affects Linux versions prior to 5.8. Linux 5.8 added
the faccessat2 system call, which we use in preference to the internal
implementation.

No test since we cannot easily change file permissions when not running
as root and the test is meaningless if running as root.

For golang/go#52313

Change-Id: I6fa64379a50c9380207eab9d095ef7fbd05a2d59
Reviewed-on: https://go-review.googlesource.com/c/sys/+/400074
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
gopherbot pushed a commit that referenced this issue Apr 12, 2022
The Faccessat call checks the user, group, or other permission bits of a
file to see if the calling process can access it. The test to see if the
group permissions should be used was made with the wrong group id, using
the process's group id rather than the file's group id. Fix this to use
the correct group id.

No test since we cannot easily change file permissions when not running
as root and the test is meaningless if running as root.

For #52313

Change-Id: I4e2c84754b0af7830b40fd15dedcbc58374d75ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/399539
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@seankhliao seankhliao added the NeedsFix label Apr 14, 2022
@neild
Copy link
Contributor Author

@neild neild commented Apr 19, 2022

@gopherbot please open backport issues.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2022

Backport issue(s) opened: #52439 (for 1.17), #52440 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2022

Change https://go.dev/cl/401078 mentions this issue: [release-branch.go1.17] syscall: check correct group in Faccessat

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2022

Change https://go.dev/cl/401079 mentions this issue: [release-branch.go1.18] syscall: check correct group in Faccessat

@neild neild added the Security label May 4, 2022
@dmitshur dmitshur added this to the Go1.19 milestone May 4, 2022
gopherbot pushed a commit that referenced this issue May 9, 2022
The Faccessat call checks the user, group, or other permission bits of a
file to see if the calling process can access it. The test to see if the
group permissions should be used was made with the wrong group id, using
the process's group id rather than the file's group id. Fix this to use
the correct group id.

No test since we cannot easily change file permissions when not running
as root and the test is meaningless if running as root.

For #52313
Fixes #52440

Change-Id: I4e2c84754b0af7830b40fd15dedcbc58374d75ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/399539
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit f66925e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/401079
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
Auto-Submit: Tatiana Bradley <tatiana@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
gopherbot pushed a commit that referenced this issue May 9, 2022
The Faccessat call checks the user, group, or other permission bits of a
file to see if the calling process can access it. The test to see if the
group permissions should be used was made with the wrong group id, using
the process's group id rather than the file's group id. Fix this to use
the correct group id.

No test since we cannot easily change file permissions when not running
as root and the test is meaningless if running as root.

For #52313
Fixes #52439

Change-Id: I4e2c84754b0af7830b40fd15dedcbc58374d75ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/399539
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit f66925e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/401078
Auto-Submit: Tatiana Bradley <tatiana@golang.org>
Run-TryBot: Tatiana Bradley <tatiana@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
@heschi
Copy link
Contributor

@heschi heschi commented May 11, 2022

This shipped in yesterday's minor releases.

@heschi heschi closed this as completed May 11, 2022
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 11, 2022

Fixed for Go 1.19 in CL 399539. (This didn't get closed because its commit message had "For" rather than "Fixes".)

@rsc rsc unassigned neild Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix release-blocker Security
Projects
None yet
Development

No branches or pull requests

7 participants