From d81768d4c8d29d2a2e0880f615d04e1dd79c88c7 Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Fri, 15 Sep 2023 19:40:57 -0700
Subject: [PATCH] Remove peer credential check from control server.

The peer credential check worked by querying the peer's UID using SO_PEERCRED.
However, when sandbox is configured with custom UID/GID mappings, which it is
in the following situations:
1. When not using directfs, sandbox has a mapping of only `65534:65534:1`. So
   all other non-65534 UIDs are unmmaped.
2. When using directfs with userns-remap. The sandbox has the same UID/GID
   mapping as the container.

In these cases, when the root user tries to connect, the host root user is not
mapped. Unmapped IDs are translated to overflowuid/gid, which happens to
default to 65534. This leads to unexpected results:
- Without directfs, this check allows any connection, because sandbox is also
  running as nobody/65534.
- In the directfs+userns-remap case, all connections are denied.

This check doesn't really enforce anything. Hence remove it.

The real enforcement mechanism is the file mode of the unix domain socket over
which the connection happens. The socket file's mode depends on umask, which
defaults to 022. Hence only the user which creates the sandbox via
`runsc create` has write permission on this socket. So only the sandbox creator
or root can connect to the sandbox, as intended.

Fixes #9356

PiperOrigin-RevId: 565836818
---
 pkg/control/server/BUILD     |  1 -
 pkg/control/server/server.go | 16 ----------------
 pkg/unet/unet.go             | 11 -----------
 pkg/unet/unet_test.go        | 28 ----------------------------
 4 files changed, 56 deletions(-)

diff --git a/pkg/control/server/BUILD b/pkg/control/server/BUILD
index 9f4592f73..4b8de9666 100644
--- a/pkg/control/server/BUILD
+++ b/pkg/control/server/BUILD
@@ -11,7 +11,6 @@ go_library(
     visibility = ["//:sandbox"],
     deps = [
         "//pkg/abi/linux",
-        "//pkg/log",
         "//pkg/sync",
         "//pkg/unet",
         "//pkg/urpc",
diff --git a/pkg/control/server/server.go b/pkg/control/server/server.go
index 906725a09..a6950009b 100644
--- a/pkg/control/server/server.go
+++ b/pkg/control/server/server.go
@@ -28,7 +28,6 @@ import (
 
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/abi/linux"
-	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/sync"
 	"gvisor.dev/gvisor/pkg/unet"
 	"gvisor.dev/gvisor/pkg/urpc"
@@ -107,21 +106,6 @@ func (s *Server) serve() {
 			return
 		}
 
-		ucred, err := conn.GetPeerCred()
-		if err != nil {
-			log.Warningf("Control couldn't get credentials: %s", err.Error())
-			conn.Close()
-			continue
-		}
-
-		// Only allow this user and root.
-		if int(ucred.Uid) != curUID && ucred.Uid != 0 {
-			// Authentication failed.
-			log.Warningf("Control auth failure: other UID = %d, current UID = %d", ucred.Uid, curUID)
-			conn.Close()
-			continue
-		}
-
 		// Handle the connection non-blockingly.
 		s.server.StartHandling(conn)
 	}
diff --git a/pkg/unet/unet.go b/pkg/unet/unet.go
index 9510a07ea..2911ffa72 100644
--- a/pkg/unet/unet.go
+++ b/pkg/unet/unet.go
@@ -395,17 +395,6 @@ func (s *Socket) GetPeerName() ([]byte, error) {
 	}
 }
 
-// GetPeerCred returns the peer's unix credentials.
-func (s *Socket) GetPeerCred() (*unix.Ucred, error) {
-	fd, ok := s.enterFD()
-	if !ok {
-		return nil, unix.EBADF
-	}
-	defer s.gate.Leave()
-
-	return unix.GetsockoptUcred(fd, unix.SOL_SOCKET, unix.SO_PEERCRED)
-}
-
 // SocketReader wraps an individual receive operation.
 //
 // This may be used for doing vectorized reads and/or sending additional
diff --git a/pkg/unet/unet_test.go b/pkg/unet/unet_test.go
index 9875a3cda..a39cef99d 100644
--- a/pkg/unet/unet_test.go
+++ b/pkg/unet/unet_test.go
@@ -556,22 +556,6 @@ func TestFDsReceiveSizeZero(t *testing.T) {
 	recvFDs(t, client, 0, []int{})
 }
 
-func TestGetPeerCred(t *testing.T) {
-	server, client := socketPair(t, false)
-	defer server.Close()
-	defer client.Close()
-
-	want := &unix.Ucred{
-		Pid: int32(os.Getpid()),
-		Uid: uint32(os.Getuid()),
-		Gid: uint32(os.Getgid()),
-	}
-
-	if got, err := client.GetPeerCred(); err != nil || !reflect.DeepEqual(got, want) {
-		t.Errorf("GetPeerCred() = %v, %v, want = %+v, %+v", got, err, want, nil)
-	}
-}
-
 func newClosedSocket() (*Socket, error) {
 	fd, err := unix.Socket(unix.AF_UNIX, unix.SOCK_STREAM, 0)
 	if err != nil {
@@ -587,18 +571,6 @@ func newClosedSocket() (*Socket, error) {
 	return s, s.Close()
 }
 
-func TestGetPeerCredFailure(t *testing.T) {
-	s, err := newClosedSocket()
-	if err != nil {
-		t.Fatalf("newClosedSocket got error %v want nil", err)
-	}
-
-	want := "bad file descriptor"
-	if _, err := s.GetPeerCred(); err == nil || err.Error() != want {
-		t.Errorf("s.GetPeerCred() = %v, want = %s", err, want)
-	}
-}
-
 func TestAcceptClosed(t *testing.T) {
 	name, err := randomFilename()
 	if err != nil {