Set unix socket peer credentials only when a connection is made.

Address review feedback on SO_PEERGROUPS:

* A failed connect(2) (ECONNREFUSED, EISCONN, EAGAIN, ...) no longer leaves
  the caller's own credentials as the socket's peer credentials. Linux only
  sets them once the connection is established, so SO_PEERGROUPS must keep
  failing with ENODATA, and an already connected socket must keep reporting
  its real peer. Socket.Connect used to store the caller's credentials in the
  client endpoint before connecting and never undid it. The credentials are
  now passed to Endpoint.Connect and BoundEndpoint.BidirectionalConnect, which
  assign them to both endpoints only in the commit branch, with both endpoint
  locks held. This also avoids a race between concurrent connect(2) calls on
  the same socket that a save and restore in Socket.Connect would have.

* SOCK_SEQPACKET connections reported reversed credentials: the accepted
  socket got the listener's credentials and the client got its own. The swap
  of the two sets was limited to SOCK_STREAM. The accepted endpoint now gets
  the client's credentials and the client gets the listener's for every
  connection-oriented type, as unix_stream_connect() does in Linux for both.

The same two bugs affected SO_PEERCRED, so it is fixed as well.

Connecting to a socket served by the gofer (a host socket) no longer reports
the caller's own credentials as the peer; they stay unset like for any other
peer that is not a sentry endpoint.

Add tests for the seqpacket case, ECONNREFUSED and EISCONN.

Assisted-by: Claude
Refs #14202

Author: anzal1

pkg/sentry/fsimpl/gofer/socket.go

@@ -54,7 +54,7 @@ type endpoint struct {
 }
 
 // BidirectionalConnect implements BoundEndpoint.BidirectionalConnect.
-func (e *endpoint) BidirectionalConnect(ctx context.Context, ce transport.ConnectingEndpoint, returnConnect func(transport.Receiver, transport.Sender)) *syserr.Error {
+func (e *endpoint) BidirectionalConnect(ctx context.Context, ce transport.ConnectingEndpoint, clientCreds transport.CredentialsControlMessage, returnConnect func(transport.Receiver, transport.Sender)) *syserr.Error {
 	// No lock ordering required as only the ConnectingEndpoint has a mutex.
 	ce.Lock()
 

pkg/sentry/socket/unix/transport/connectioned.go

@@ -107,11 +107,12 @@ type connectionedEndpoint struct {
 	stype linux.SockType
 
 	// peerCreds is used to store the peer credentials.
-	// This will store the socket's own credentials until the connection is
-	// established with connect(2). Once the connection is established, this
-	// will store the peer's credentials. The use of this option is possible
-	// only for connected `AF_UNIX` stream sockets and for `AF_UNIX` stream and
-	// datagram socket pairs created using socketpair(2)
+	// It is nil until the connection is established with connect(2), so a
+	// connect(2) that fails leaves it untouched. Once the connection is
+	// established, this stores the peer's credentials. The use of this option
+	// is possible only for connected `AF_UNIX` stream and seqpacket sockets and
+	// for `AF_UNIX` stream, seqpacket and datagram socket pairs created using
+	// socketpair(2).
 	peerCreds CredentialsControlMessage
 
 	// acceptedChan is per the TCP endpoint implementation. Note that the
@@ -297,24 +298,20 @@ func (e *connectionedEndpoint) Close(ctx context.Context) {
 	}
 }
 
-func (e *connectionedEndpoint) swapPeerCredsLocked(ctx context.Context, cend ConnectingEndpoint, ne *connectionedEndpoint) *syserr.Error {
-	ce, ok := cend.(*connectionedEndpoint)
-	if !ok {
-		return syserr.ErrInvalidEndpointState
-	}
-	// Swap peer credentials between the two endpoints.
-	ne.peerCreds, ce.peerCreds = ce.peerCreds, ne.peerCreds
-	return nil
-}
-
 // BidirectionalConnect implements BoundEndpoint.BidirectionalConnect.
-func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce ConnectingEndpoint, returnConnect func(Receiver, Sender)) *syserr.Error {
+func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce ConnectingEndpoint, clientCreds CredentialsControlMessage, returnConnect func(Receiver, Sender)) *syserr.Error {
 	if ce.Type() != e.stype {
 		return syserr.ErrWrongProtocolForSocket
 	}
 
+	// The peer credentials of the connecting endpoint are set on commit.
+	cce, ok := ce.(*connectionedEndpoint)
+	if !ok {
+		return syserr.ErrInvalidEndpointState
+	}
+
 	// Check if ce is e to avoid a deadlock.
-	if ce, ok := ce.(*connectionedEndpoint); ok && ce == e {
+	if cce == e {
 		return syserr.ErrInvalidEndpointState
 	}
 
@@ -360,7 +357,9 @@ func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce Conn
 	ne.ops.SetSendBufferSize(defaultBufferSize, false /* notify */)
 	ne.ops.SetReceiveBufferSize(defaultBufferSize, false /* notify */)
 	ne.SocketOptions().SetPassCred(e.SocketOptions().GetPassCred())
-	ne.peerCreds = e.peerCreds
+	// The accepted endpoint reports the connecting task as its peer, like
+	// Linux does for both SOCK_STREAM and SOCK_SEQPACKET.
+	ne.peerCreds = clientCreds
 
 	readQueue := &queue{readerWaiters: ce.WaiterQueue(), writerWaiters: ne.WaitQueue, limit: defaultBufferSize}
 	readQueue.InitRefs()
@@ -387,10 +386,11 @@ func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce Conn
 			writeQueue: writeQueue,
 		}
 		readQueue.IncRef()
+		// The connecting endpoint reports the listener as its peer. This is
+		// done only here, with both endpoints locked, so that a failed or
+		// concurrent connect(2) never changes the peer credentials.
+		cce.peerCreds = e.peerCreds
 		if e.stype == linux.SOCK_STREAM {
-			if err := e.swapPeerCredsLocked(ctx, ce, ne); err != nil {
-				return err
-			}
 			returnConnect(&streamQueueReceiver{queueReceiver: queueReceiver{readQueue: readQueue}}, peer)
 		} else {
 			returnConnect(&queueReceiver{readQueue: readQueue}, peer)
@@ -421,7 +421,7 @@ func (e *connectionedEndpoint) UnidirectionalConnect(ctx context.Context) (Sende
 
 // Connect attempts to directly connect to another Endpoint.
 // Implements Endpoint.Connect.
-func (e *connectionedEndpoint) Connect(ctx context.Context, server BoundEndpoint) *syserr.Error {
+func (e *connectionedEndpoint) Connect(ctx context.Context, server BoundEndpoint, clientCreds CredentialsControlMessage) *syserr.Error {
 	returnConnect := func(r Receiver, ce Sender) {
 		e.receiver = r
 		e.peer = ce
@@ -433,7 +433,7 @@ func (e *connectionedEndpoint) Connect(ctx context.Context, server BoundEndpoint
 		}
 	}
 
-	return server.BidirectionalConnect(ctx, e, returnConnect)
+	return server.BidirectionalConnect(ctx, e, clientCreds, returnConnect)
 }
 
 // Listen starts listening on the connection.

pkg/sentry/socket/unix/transport/connectionless.go

@@ -79,7 +79,7 @@ func (e *connectionlessEndpoint) Close(ctx context.Context) {
 }
 
 // BidirectionalConnect implements BoundEndpoint.BidirectionalConnect.
-func (e *connectionlessEndpoint) BidirectionalConnect(ctx context.Context, ce ConnectingEndpoint, returnConnect func(Receiver, Sender)) *syserr.Error {
+func (e *connectionlessEndpoint) BidirectionalConnect(ctx context.Context, ce ConnectingEndpoint, clientCreds CredentialsControlMessage, returnConnect func(Receiver, Sender)) *syserr.Error {
 	return syserr.ErrConnectionRefused
 }
 
@@ -136,7 +136,7 @@ func (e *connectionlessEndpoint) Type() linux.SockType {
 }
 
 // Connect attempts to connect directly to server.
-func (e *connectionlessEndpoint) Connect(ctx context.Context, server BoundEndpoint) *syserr.Error {
+func (e *connectionlessEndpoint) Connect(ctx context.Context, server BoundEndpoint, clientCreds CredentialsControlMessage) *syserr.Error {
 	peer, err := server.UnidirectionalConnect(ctx)
 	if err != nil {
 		return err

pkg/sentry/socket/unix/transport/unix.go

@@ -215,8 +215,12 @@ type Endpoint interface {
 	// This should be called on the client endpoint, and the (bound)
 	// endpoint passed in as a parameter.
 	//
+	// clientCreds are the credentials of the connecting task. They become the
+	// peer credentials of the accepted endpoint only if the connection is
+	// established; a failed Connect doesn't change any peer credentials.
+	//
 	// The error codes are the same as Connect.
-	Connect(ctx context.Context, server BoundEndpoint) *syserr.Error
+	Connect(ctx context.Context, server BoundEndpoint, clientCreds CredentialsControlMessage) *syserr.Error
 
 	// Shutdown closes the read and/or write end of the endpoint connection
 	// to its peer.
@@ -307,9 +311,14 @@ type BoundEndpoint interface {
 	// be unconnected and not listening and the BoundEndpoint whose
 	// BidirectionalConnect method is being called must be listening.
 	//
+	// clientCreds are the credentials of the connecting task. On a successful
+	// connect, the accepted endpoint gets them as its peer credentials, and
+	// the ConnectingEndpoint gets the peer credentials of this endpoint. They
+	// must not change if the connect fails.
+	//
 	// This method will return syserr.ErrConnectionRefused on endpoints with a
 	// type that isn't SockStream or SockSeqpacket.
-	BidirectionalConnect(ctx context.Context, ep ConnectingEndpoint, returnConnect func(Receiver, Sender)) *syserr.Error
+	BidirectionalConnect(ctx context.Context, ep ConnectingEndpoint, clientCreds CredentialsControlMessage, returnConnect func(Receiver, Sender)) *syserr.Error
 
 	// UnidirectionalConnect establishes a write-only connection to a unix
 	// endpoint.

pkg/sentry/socket/unix/unix.go

@@ -640,11 +640,9 @@ func (s *Socket) Connect(t *kernel.Task, sockaddr []byte, blocking bool) *syserr
 	}
 	defer ep.Release(t)
 
-	// Initialize the peer credentials of client endpoint.
-	s.ep.SetPeerCreds(control.MakeCreds(t))
-
-	// Connect the server endpoint.
-	err = s.ep.Connect(t, ep)
+	// Connect the server endpoint. The peer credentials of both endpoints are
+	// set by the transport once the connection is established.
+	err = s.ep.Connect(t, ep, control.MakeCreds(t))
 
 	if err == syserr.ErrWrongProtocolForSocket {
 		// Linux for abstract sockets returns ErrConnectionRefused

test/syscalls/linux/socket_unix_peercred.cc

@@ -20,6 +20,7 @@
 #include <linux/capability.h>
 #include <sched.h>
 #include <signal.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/mman.h>
@@ -33,6 +34,7 @@
 #include <array>
 #include <atomic>
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "gmock/gmock.h"
@@ -661,6 +663,122 @@ TEST(UnixSocketPeerGroupsTest, NotUnixSocket) {
               SyscallFailsWithErrno(ENODATA));
 }
 
+// GroupsString formats groups for failure messages.
+std::string GroupsString(const std::vector<gid_t>& groups) {
+  std::string s = "{";
+  for (size_t i = 0; i < groups.size(); i++) {
+    if (i > 0) {
+      s += ",";
+    }
+    s += std::to_string(groups[i]);
+  }
+  return s + "}";
+}
+
+// connect(2) on a SOCK_SEQPACKET socket sets the peer credentials like it does
+// for SOCK_STREAM: the accepted socket reports the client's groups and the
+// client reports the groups the listener had at listen(2).
+TEST(UnixSocketPeerGroupsTest, SeqpacketConnectReportsPeerGroups) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SETGID)));
+
+  const std::vector<gid_t> listener_groups = {3001};
+  const std::vector<gid_t> client_groups = {3002, 3003};
+  const auto fn = [&] {
+    TEST_PCHECK(setgroups(listener_groups.size(), listener_groups.data()) ==
+                0);
+    struct sockaddr_un addr =
+        TEST_CHECK_NO_ERRNO_AND_VALUE(UniqueUnixAddr(true, AF_UNIX));
+    int srv = socket(AF_UNIX, SOCK_SEQPACKET, 0);
+    TEST_PCHECK(srv >= 0);
+    TEST_PCHECK(bind(srv, AsSockAddr(&addr), sizeof(addr)) == 0);
+    TEST_PCHECK(listen(srv, 5) == 0);
+
+    // Connect with different groups than the listener had at listen(2).
+    TEST_PCHECK(setgroups(client_groups.size(), client_groups.data()) == 0);
+    int cli = socket(AF_UNIX, SOCK_SEQPACKET, 0);
+    TEST_PCHECK(cli >= 0);
+    TEST_PCHECK(connect(cli, AsSockAddr(&addr), sizeof(addr)) == 0);
+    int acc = accept(srv, nullptr, nullptr);
+    TEST_PCHECK(acc >= 0);
+
+    const std::vector<gid_t> acc_groups =
+        TEST_CHECK_NO_ERRNO_AND_VALUE(GetPeerGroups(acc));
+    const std::vector<gid_t> cli_groups =
+        TEST_CHECK_NO_ERRNO_AND_VALUE(GetPeerGroups(cli));
+    fprintf(stderr,
+            "SOCK_SEQPACKET: accepted socket reports %s (want %s), client "
+            "reports %s (want %s)\n",
+            GroupsString(acc_groups).c_str(),
+            GroupsString(client_groups).c_str(),
+            GroupsString(cli_groups).c_str(),
+            GroupsString(listener_groups).c_str());
+    TEST_CHECK_MSG(acc_groups == client_groups,
+                   "accepted socket must report the client's groups");
+    TEST_CHECK_MSG(cli_groups == listener_groups,
+                   "client must report the listener's groups");
+  };
+  EXPECT_THAT(InForkedProcess(fn), IsPosixErrorOkAndHolds(0));
+}
+
+// A connect(2) that fails leaves the socket without peer credentials, so
+// SO_PEERGROUPS still fails with ENODATA.
+TEST(UnixSocketPeerGroupsTest, FailedConnectLeavesNoPeerGroups) {
+  auto addr = ASSERT_NO_ERRNO_AND_VALUE(UniqueUnixAddr(true, AF_UNIX));
+  // Bound but not listening, so connect(2) fails with ECONNREFUSED.
+  auto bound = ASSERT_NO_ERRNO_AND_VALUE(Socket(AF_UNIX, SOCK_STREAM, 0));
+  ASSERT_THAT(bind(bound.get(), AsSockAddr(&addr), sizeof(addr)),
+              SyscallSucceeds());
+
+  auto s = ASSERT_NO_ERRNO_AND_VALUE(Socket(AF_UNIX, SOCK_STREAM, 0));
+  ASSERT_THAT(connect(s.get(), AsSockAddr(&addr), sizeof(addr)),
+              SyscallFailsWithErrno(ECONNREFUSED));
+
+  std::vector<gid_t> buf(NGROUPS_MAX);
+  socklen_t len = buf.size() * sizeof(gid_t);
+  EXPECT_THAT(getsockopt(s.get(), SOL_SOCKET, SO_PEERGROUPS, buf.data(), &len),
+              SyscallFailsWithErrno(ENODATA));
+}
+
+// connect(2) on a connected socket fails with EISCONN and doesn't change the
+// groups reported for the peer.
+TEST(UnixSocketPeerGroupsTest, ConnectEISCONNKeepsPeerGroups) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SETGID)));
+
+  const std::vector<gid_t> listener_groups = {4001};
+  const std::vector<gid_t> later_groups = {4002};
+  const auto fn = [&] {
+    TEST_PCHECK(setgroups(listener_groups.size(), listener_groups.data()) ==
+                0);
+    struct sockaddr_un addr =
+        TEST_CHECK_NO_ERRNO_AND_VALUE(UniqueUnixAddr(true, AF_UNIX));
+    int srv = socket(AF_UNIX, SOCK_STREAM, 0);
+    TEST_PCHECK(srv >= 0);
+    TEST_PCHECK(bind(srv, AsSockAddr(&addr), sizeof(addr)) == 0);
+    TEST_PCHECK(listen(srv, 5) == 0);
+    int cli = socket(AF_UNIX, SOCK_STREAM, 0);
+    TEST_PCHECK(cli >= 0);
+    TEST_PCHECK(connect(cli, AsSockAddr(&addr), sizeof(addr)) == 0);
+    TEST_CHECK(TEST_CHECK_NO_ERRNO_AND_VALUE(GetPeerGroups(cli)) ==
+               listener_groups);
+
+    // Connect again with different groups. This fails and must not replace
+    // the peer credentials.
+    TEST_PCHECK(setgroups(later_groups.size(), later_groups.data()) == 0);
+    TEST_CHECK_MSG(connect(cli, AsSockAddr(&addr), sizeof(addr)) == -1 &&
+                       errno == EISCONN,
+                   "second connect must fail with EISCONN");
+
+    const std::vector<gid_t> cli_groups =
+        TEST_CHECK_NO_ERRNO_AND_VALUE(GetPeerGroups(cli));
+    fprintf(stderr, "after EISCONN: client reports %s (want %s)\n",
+            GroupsString(cli_groups).c_str(),
+            GroupsString(listener_groups).c_str());
+    TEST_CHECK_MSG(cli_groups == listener_groups,
+                   "a failed connect must not change the peer groups");
+  };
+  EXPECT_THAT(InForkedProcess(fn), IsPosixErrorOkAndHolds(0));
+}
+
 }  // namespace
 
 }  // namespace testing