Add a new RPC ConnectWithCreds to allow gofer to connect to a unix domain socket with application's credentials

[xianzhe-databricks]

Dear gvisor developers,

Thank you very much for maintaining / developing gvisor!

Motivation

We had a use case (which I believe is a wide use case) that the sandboxes send requests over a unix domain socket on host, which is mapped to the container's file system and listened to by a server on the local host.

The sandboxed application is started with a prescribed uid. To authenticate the request, the server verifies the request's uid.

However, as the gofer process (which usually runs as root) executes connect(unix_domain_socket) call on behalf of the sandbox, the server always sees a uid 0. Hence the server cannot authenticate the UDS requests coming from the sandbox.

Proposal

I propose to Add a new RPC ConnectWithCreds to allow gofer to connect to a unix domain socket with application's credentials. On that gofer server thread, the euid/egid are temporarily changed to application's uid/gid and restored after the connect(2) call.

Questions

What do you think of this change? Is there any security/ functionality concern? Thank you so much for your feedback!

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[xianzhe-databricks]

I will prepare some sample code to reproduce this issue. Databricks should already have the CLA with Google. I will ask our PoC to include me in the group of contributors.

[ayushr2]

Is the server using SO_PEERCRED? We had a similar issue in gVisor where our control server was using SO_PEERCRED to authenticate requests and due to the different user namespace mappings the sandbox process was running under, the check always seemed to pass. We got rid of the SO_PEERCRED check and started only relying on filesystem permission check; see 36b9b19 and d81768d.

I understand that your situation is different, but could you use the same logic in your case? To connect(2), the caller needs write permissions on the UDS. If the UDS that you share with the sandbox is owned by UID=10 and the app is running with UID=8, then the connect(2) attempt from the sandbox should fail.

cc @avagin about implications of running the gofer with euid=spec.Process.User.UID.

[xianzhe-databricks]

Is the server using SO_PEERCRED? We had a similar issue in gVisor where our control server was using SO_PEERCRED to authenticate requests and due to the different user namespace mappings the sandbox process was running under, the check always seemed to pass. We got rid of the SO_PEERCRED check and started only relying on filesystem permission check; see 36b9b19 and d81768d.

I understand that your situation is different, but could you use the same logic in your case? To connect(2), the caller needs write permissions on the UDS. If the UDS that you share with the sandbox is owned by UID=10 and the app is running with UID=8, then the connect(2) attempt from the sandbox should fail.

cc @avagin about implications of running the gofer with euid=spec.Process.User.UID.

Thanks for the fast reply! Yes the server uses SO_PEERCRED.

We cannot use file permission because, there is only one unix domain socket file on the host, which the server listens to. This unix domain socket file is mapped to every sandbox' filesystem, i.e. every sandbox should have the capability to talk to the server over this unix domain socket. The server utilizes the uid to prevent the case that one sandbox impersonates another sandbox when talking to the server.

[EtiennePerot]

Would this problem be solved by running sandboxes in --rootless mode, each with a distinct user?

[xianzhe-databricks]

Would this problem be solved by running sandboxes in --rootless mode, each with a distinct user?

Yeah we considered this. However this comes with the cost of much weakened security guarantees, for example the sandbox's isolated network stack cannot be used. Directfs can also not be used.

I think in general the sandbox's host-filesystem-access shouldn't be performed with a root entity. This is already a security concern. What do you think?

[ayushr2]

I think in general the sandbox's host-filesystem-access shouldn't be performed with a root entity. This is already a security concern. What do you think?

The gofer is running in a pivot_root(2) which only has the container filesystem (the rootfs + any bind mounts) mounted in it. But yes it can create the SO_PEERCRED kind of issues you have seen. Note that the application in the container may not always be running with the UID from spec.Process.User.UID. The application can change UID/GID and we need to gofer to replicate that behavior. That is why we pass UID/GID as part of RPC messages that create files. For example, see Mkdir() implementation:

gvisor/runsc/fsgofer/lisafs.go

Lines 617 to 620 in bd0cbf8

	if err := unix.Fchownat(childDirFd, "", int(uid), int(gid), unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW); err != nil {
	unix.Close(childDirFd)
	return nil, linux.Statx{}, err
	}

So the gofer runs as root and changes UID/GID for created files based on the application thread's UID/GID. I think running as root is also needed for the special capabilities that the gofer needs to run with:

gvisor/runsc/cmd/gofer.go

Lines 47 to 62 in bd0cbf8

	var caps = []string{
	"CAP_CHOWN",
	"CAP_DAC_OVERRIDE",
	"CAP_DAC_READ_SEARCH",
	"CAP_FOWNER",
	"CAP_FSETID",
	"CAP_SYS_CHROOT",
	}
	// goferCaps is the minimal set of capabilities needed by the Gofer to operate
	// on files.
	var goferCaps = &specs.LinuxCapabilities{
	Bounding: caps,
	Effective: caps,
	Permitted: caps,
	}

I think we can make an exception for connect(2) implementation and temporarily set euid to the caller's euid while making the connect(2) host syscall. But I think we will need to add another lisafs RPC because the current Connect implementation doesn't have information about user UID/GID:

gvisor/pkg/lisafs/message.go

Lines 1296 to 1306 in bd0cbf8

	// ConnectReq is used to make a Connect request.
	//
	// +marshal boundCheck
	type ConnectReq struct {
	FD FDID
	// SockType is used to specify the socket type to connect to. As a special
	// case, SockType = 0 means that the socket type does not matter and the
	// requester will accept any socket type.
	SockType uint32
	_ uint32 // Need to make struct packed.
	}

[xianzhe-databricks]

@ayushr2 Thank you so much for your detailed reply!

The gofer is running in a pivot_root(2) which only has the container filesystem (the rootfs + any bind mounts) mounted in it.

However the bind mounts represent files on host filesystem. The sandbox's file access to bind mounts is performed with root privilege due to gofer. Would this already be a security concern?

Note that the application in the container may not always be running with the UID from spec.Process.User.UID. The application can change UID/GID and we need to gofer to replicate that behavior.

The application usually runs as non-root. How is it possible/ What are the cases for a non-root to change its euid at runtime (besides the corner case of changing to its ruid or saved uid)?

I think we can make an exception for connect(2) implementation and temporarily set euid to the caller's euid while making the connect(2) host syscall.

I have two concerns here:

1. Would this create a race condition? I.e. there might be concurrent RPC call which requires a euid 0, while this temporary change takes place.
2. We cannot rely on the uid/gid information passed from RPC message here, as what we did for the mkdir implementation. The execution context within the sandbox is untrusted and can in theory pass any uid/gid pair to the RPC call as it wishes. It would allow connecting to the unix domain socket with arbitrary credential as the attacker likes.

[xianzhe-databricks]

We somewhat urgently rely on solving this issue. May I be able to expect an answer from you on this? @ayushr2 @avagin Thank you so much!!

[xianzhe-databricks]

Also a follow-up question:

I think running as root is also needed for the special capabilities that the gofer needs to run with

Then what happens with the -rootless mode? My previous assumption was that gofer only needs root privilege during the initialization phase (to set up security guardrails). When the gofer server is started, it could run without root (since -rootless mode anyway runs gofer without root).

Also aren't linux capabilities designed to grant non-root processes the fine-grained root-like privileged actions? I think just because we grant gofer these capabilities, gofer can run without root

gvisor/runsc/cmd/gofer.go

Lines 47 to 62 in bd0cbf8

	var caps = []string{
	"CAP_CHOWN",
	"CAP_DAC_OVERRIDE",
	"CAP_DAC_READ_SEARCH",
	"CAP_FOWNER",
	"CAP_FSETID",
	"CAP_SYS_CHROOT",
	}
	// goferCaps is the minimal set of capabilities needed by the Gofer to operate
	// on files.
	var goferCaps = &specs.LinuxCapabilities{
	Bounding: caps,
	Effective: caps,
	Permitted: caps,
	}

I also verified that the unix.Fchownat succeeds even with Gofer's uid changed to spec.Process.User.UID. I believe it is just because gofer is granted the capabilities CAP_CHOWN and CAP_FOWNER. Note that with directfs enabled, the Mkdir system call is even not routed to gofer.

[ayushr2]

However the bind mounts represent files on host filesystem. The sandbox's file access to bind mounts is performed with root privilege due to gofer. Would this already be a security concern?

Note that the sentry does all necessary filesystem permission checks and only makes authenticated RPCs to the gofer, after which the gofer takes actions as root to fulfill the filesystem request. The attacker needs to compromise the sentry to bypass sentry checks and make arbitrary RPCs to the gofer. Even if that does happen, then our second layer of defense here is the gofer. It limits the attacker to just the filesystem that has been exposed to the container via bind mounts and prevents the attacker from accessing the host filesystem. That being said, if we can actually run the gofer in non-root user, it would be nice.

How is it possible/ What are the cases for a non-root to change its euid at runtime (besides the corner case of changing to its ruid or saved uid)?

I believe it is possible that the user can call runsc exec which can start a new process with another uid/gid.

Would this create a race condition? I.e. there might be concurrent RPC call which requires a euid 0, while this temporary change takes place.

setuid/setgid only changes the users/groups for the current system thread. So we need to use runtime.LockOSThread() and runtime.UnlockOSThread() during the temporary change in UID if we were to do this.

[ayushr2]

Please squash all your commits into 1, because all commits from this branch will be applied directly to master if we were to merge this.

[xianzhe-databricks]

Please squash all your commits into 1, because all commits from this branch will be applied directly to master if we were to merge this.

Thanks for reminding this! I will keep the commit history as-is for the moment, and when we are ready to merge, I squash all the commits and do a force-push.

[xianzhe-databricks]

@ayushr2 could you approve of the github workflows to see how tests are going? Thank you so much!

[xianzhe-databricks]

@ayushr2 Happy new year and thank you so much for the guidance along the way. I did a final check and made sure the unit tests and integration tests passed.

The fix is needed by us urgently. I really appreciate it if we could make the fix included in next week's release. Could you let me know what else is left for the merge (besides squashing the commits)?

[ayushr2]

Just nits, mostly LGTM

[ayushr2]

Its probably time to squash all commits BTW.

[xianzhe-databricks]

Its probably time to squash all commits BTW.

Thanks. The commits were squashed.

[xianzhe-databricks]

Just did another final check. The PR can be merged from our side. Thank you so much!

[xianzhe-databricks]

@ayushr2 shall we create a copybara PR for this one? Will we able to include it in this week's release? Thanks!

[ayushr2]

Its undergoing internal review and needs approval from someone other than me since I approved it here. I don't think we will be able to make it into this week's release.

[xianzhe-databricks]

Its undergoing internal review and needs approval from someone other than me since I approved it here. I don't think we will be able to make it into this week's release.

I see. Thanks for informing me!