osutil/sys, client: add sys.RunAsUidGid, use it for auth.json #4983

chipaca · 2018-04-04T16:11:14Z

If /home is on NFS, trying to read, write or remove the user's
auth.json as root fails.

This PR adds code to drop privileges to the owner of the file before
attempting operating on it.

If /home is on NFS, trying to read, write or remove the user's auth.json as root fails. This PR adds code to drop privileges to the owner of the file before attempting operating on it.

chipaca · 2018-04-04T17:34:24Z

Wellp, that didn't work. back to the drawing board.

bboozzoo · 2018-04-06T08:02:17Z

osutil/sys/syscall.go

+
+		ch <- f()
+
+		// From Go 1.10, if we exit without unlocking, the thread is killed \o/


Have you tried to restore the uid/gid and then runtime.UnlockOSThread() instead of exiting?

bboozzoo · 2018-04-06T08:12:32Z

osutil/sys/syscall.go

+// RunAsUidGid starts a goroutine, pins it to the OS thread, calls setuid and
+// setgid, and runs the function.
+func RunAsUidGid(uid UserID, gid GroupID, f func() error) error {
+	ch := make(chan error)


Oh, and a buffered channel instead of unbufferred one.

chipaca · 2018-04-06T10:24:05Z

we're back, baby!

codecov-io · 2018-04-06T12:22:20Z

Codecov Report

Merging #4983 into master will increase coverage by 0.05%.
The diff coverage is 67.85%.

@@            Coverage Diff             @@
##           master    #4983      +/-   ##
==========================================
+ Coverage   79.12%   79.18%   +0.05%     
==========================================
  Files         478      485       +7     
  Lines       35230    35950     +720     
==========================================
+ Hits        27876    28467     +591     
- Misses       5157     5234      +77     
- Partials     2197     2249      +52

Impacted Files	Coverage Δ
client/login.go	`64.1% <67.85%> (-3.12%)`	⬇️
cmd/snap/cmd_userd.go	`66.66% <0%> (-19.05%)`	⬇️
testutil/lowlevel.go	`88.78% <0%> (-3.9%)`	⬇️
interfaces/builtin/all.go	`80.3% <0%> (-3.83%)`	⬇️
overlord/snapstate/storehelpers.go	`80.76% <0%> (-1.74%)`	⬇️
overlord/ifacestate/handlers.go	`63.25% <0%> (-1.48%)`	⬇️
store/errors.go	`71.18% <0%> (-0.95%)`	⬇️
client/snap_op.go	`80.88% <0%> (-0.79%)`	⬇️
cmd/snap-update-ns/utils.go	`93.93% <0%> (-0.33%)`	⬇️
cmd/snap/cmd_run.go	`65.14% <0%> (-0.23%)`	⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2091aae...ef022e8. Read the comment docs.

bboozzoo

yay!

zyga · 2018-04-24T10:33:58Z

client/login.go

-		return nil
-	}
+	return sys.RunAsUidGid(uid, gid, func() error {
+		if err := os.MkdirAll(filepath.Dir(targetFile), 0700); err != nil {


Hmm, just return the error directly? :-)

Ah, the diff formatting confused me, never mind :)

zyga · 2018-04-24T10:51:57Z

osutil/sys/syscall.go

+	return r1, r2, err
+}
+
+type UnrecoverableError struct {


Please document this type.

zyga

Some comments inline.

zyga · 2018-04-24T10:52:11Z

osutil/sys/syscall.go

+	Err  error
+}
+
+func (e UnrecoverableError) Error() string {


Please document this error.

Missing docs :)

zyga · 2018-04-24T10:56:46Z

osutil/sys/syscall.go

+func retryRawSyscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err syscall.Errno) {
+	ms := &syscall.Timespec{Nsec: 1000}
+	for i := 0; i < 30; i++ {
+		r1, r2, err = syscall.RawSyscall(trap, a1, a2, a3)


Is there a need to use RawSyscall vs Syscall? The difference, AFAIK, is that raw syscall blocks directly and Syscall will ping back to the runtime to say its about to enter and has just left a system call. If there's a need then please document why we use RawSyscall here.

I'm trusting the go standard library on the distinction here (syscall.Setreuid et al use RawSyscall). The difference is not documented and I don't know enough of the internals to have a good opinion.

zyga · 2018-04-24T10:57:12Z

osutil/sys/syscall.go

@@ -107,3 +109,79 @@ func FcntlGetFl(fd int) (int, error) {
 	}
 	return int(flags), nil
 }
+
+func retryRawSyscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err syscall.Errno) {


I think this function warrants a comment

zyga · 2018-04-24T10:59:41Z

osutil/sys/syscall.go

+		ruid := Getuid()
+		rgid := Getgid()
+
+		// do the setregid first =)


Can you document if the order is significant and if so, why, please?

zyga · 2018-04-24T11:05:14Z

osutil/sys/syscall.go

+		// carefully restore thread state.
+		defer runtime.UnlockOSThread()
+
+		ruid := Getuid()


While reading I read the code for those. Is there any reason we are not using getres{u,g}id()? It uses one less system call.

Drop retryRawSyscall (we don't do this for other syscalls, yet), document UnrecoverableError, and make RunAsUidGid defer-less.

zyga

LGTM with a nitpick.

zyga · 2018-04-26T08:24:48Z

osutil/sys/syscall.go

+		// from the docs:
+		//   until the goroutine exits or calls UnlockOSThread, it will
+		//   always execute in this thread, and no other goroutine can.
+		// that last bit means it's safe to setuid/setgid in here, as no


Nitpick, That

I'm torn between fixing it now, or landing it now and fixing it later :-)

chipaca · 2018-04-27T09:20:39Z

After much poking around, I don't think this is safe to land as is.

It all boils down to golang/go#20676, which is fixed in 1.10 but not before.

Given that this is for use from snap and not snapd, and that contention should be low, we could make it very very unlikely to hit the bug. But with the number of users we have, very unlikely is still quite likely.

The options to fix lp:1761193 seem to me to be:

write something in cgo that calls clone and then does the syscalls, and either calls a function back (via an id, as you can't pass pointers around), or does the file opening and returns the descriptor,
ignore the contentious contention and land this,
move to 1.10, or
do the file i/o via a helper (e.g. runuser+cat).

the first one gives me hives, the second will cause very weird bugs, the third is likely a year away, so ... maybe I do the fourth?

osutil/sys, client: add sys.RunAsUidGid, use it for auth.json

098b7cc

If /home is on NFS, trying to read, write or remove the user's auth.json as root fails. This PR adds code to drop privileges to the owner of the file before attempting operating on it.

chipaca closed this Apr 4, 2018

chipaca mentioned this pull request Apr 5, 2018

many: implement a poor man's privileges drop, use for auth.json #4990

Closed

bboozzoo reviewed Apr 6, 2018

View reviewed changes

chipaca reopened this Apr 6, 2018

osutil/sys, etc: switch to using euids! \o/ (thanks @bboozzoo)

41d93cc

bboozzoo approved these changes Apr 6, 2018

View reviewed changes

chipaca mentioned this pull request Apr 24, 2018

overlord/snapshotstate/backend: introducing the snapshot backend #5066

Merged

zyga reviewed Apr 24, 2018

View reviewed changes

osutil/sys/syscall.go Outdated

return r1, r2, err

}

type UnrecoverableError struct {

Copy link

Collaborator

zyga Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this type.

zyga reviewed Apr 24, 2018

View reviewed changes

osutil/sys: fixes and cleanup

ef022e8

Drop retryRawSyscall (we don't do this for other syscalls, yet), document UnrecoverableError, and make RunAsUidGid defer-less.

zyga approved these changes Apr 26, 2018

View reviewed changes

chipaca closed this May 9, 2018

chipaca deleted the drop-privs-for-auth-file-ops branch June 4, 2019 10:41

Meulengracht mentioned this pull request Jul 12, 2023

client,osutil: add sys.RunAsUidGid, use it for auth.json (revived #4983) #12973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osutil/sys, client: add sys.RunAsUidGid, use it for auth.json #4983

osutil/sys, client: add sys.RunAsUidGid, use it for auth.json #4983

chipaca commented Apr 4, 2018 •

edited

chipaca commented Apr 4, 2018

bboozzoo Apr 6, 2018

bboozzoo Apr 6, 2018

chipaca commented Apr 6, 2018

codecov-io commented Apr 6, 2018 •

edited

bboozzoo left a comment

zyga Apr 24, 2018

zyga Apr 24, 2018

zyga Apr 24, 2018

zyga left a comment

zyga Apr 24, 2018

zyga Apr 26, 2018

zyga Apr 24, 2018

chipaca Apr 24, 2018

zyga Apr 24, 2018

zyga Apr 24, 2018

zyga Apr 24, 2018

zyga left a comment

zyga Apr 26, 2018

chipaca Apr 26, 2018

chipaca commented Apr 27, 2018 •

edited


		ch <- f()

		// From Go 1.10, if we exit without unlocking, the thread is killed \o/

osutil/sys, client: add sys.RunAsUidGid, use it for auth.json #4983

osutil/sys, client: add sys.RunAsUidGid, use it for auth.json #4983

Conversation

chipaca commented Apr 4, 2018 • edited

chipaca commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca commented Apr 6, 2018

codecov-io commented Apr 6, 2018 • edited

Codecov Report

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca commented Apr 27, 2018 • edited

chipaca commented Apr 4, 2018 •

edited

codecov-io commented Apr 6, 2018 •

edited

chipaca commented Apr 27, 2018 •

edited