Fix Unix socket permissions.

Enforce write permission checks in BoundEndpointAt, which corresponds to the
permission checks in Linux (net/unix/af_unix.c:unix_find_other).
Also, create bound socket files with the correct permissions in VFS2.

Fixes #2324.

PiperOrigin-RevId: 308949084

9 files changed, 94 insertions, 9 deletions

diff --git a/pkg/sentry/fsimpl/ext/filesystem.go b/pkg/sentry/fsimpl/ext/filesystem.go
index 6c882d0b6..77b644275 100644
--- a/pkg/sentry/fsimpl/ext/filesystem.go
+++ b/pkg/sentry/fsimpl/ext/filesystem.go
@@ -486,10 +486,13 @@ func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error
 
 // BoundEndpointAt implements FilesystemImpl.BoundEndpointAt.
 func (fs *filesystem) BoundEndpointAt(ctx context.Context, rp *vfs.ResolvingPath, opts vfs.BoundEndpointOptions) (transport.BoundEndpoint, error) {
-	_, _, err := fs.walk(rp, false)
+	_, inode, err := fs.walk(rp, false)
 	if err != nil {
 		return nil, err
 	}
+	if err := inode.checkPermissions(rp.Credentials(), vfs.MayWrite); err != nil {
+		return nil, err
+	}
 
 	// TODO(b/134676337): Support sockets.
 	return nil, syserror.ECONNREFUSED
diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go
index 40c1b2104..4a12ae245 100644
--- a/pkg/sentry/fsimpl/kernfs/filesystem.go
+++ b/pkg/sentry/fsimpl/kernfs/filesystem.go
@@ -768,12 +768,15 @@ func (fs *Filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error
 // BoundEndpointAt implements FilesystemImpl.BoundEndpointAt.
 func (fs *Filesystem) BoundEndpointAt(ctx context.Context, rp *vfs.ResolvingPath, opts vfs.BoundEndpointOptions) (transport.BoundEndpoint, error) {
 	fs.mu.RLock()
-	_, _, err := fs.walkExistingLocked(ctx, rp)
+	_, inode, err := fs.walkExistingLocked(ctx, rp)
 	fs.mu.RUnlock()
 	fs.processDeferredDecRefs()
 	if err != nil {
 		return nil, err
 	}
+	if err := inode.CheckPermissions(ctx, rp.Credentials(), vfs.MayWrite); err != nil {
+		return nil, err
+	}
 	return nil, syserror.ECONNREFUSED
 }
 
diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go
index 388b98bef..36ffcb592 100644
--- a/pkg/sentry/fsimpl/tmpfs/filesystem.go
+++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go
@@ -704,6 +704,9 @@ func (fs *filesystem) BoundEndpointAt(ctx context.Context, rp *vfs.ResolvingPath
 	if err != nil {
 		return nil, err
 	}
+	if err := d.inode.checkPermissions(rp.Credentials(), vfs.MayWrite); err != nil {
+		return nil, err
+	}
 	switch impl := d.inode.impl.(type) {
 	case *socketFile:
 		return impl.ep, nil
diff --git a/pkg/sentry/socket/unix/unix.go b/pkg/sentry/socket/unix/unix.go
index ddd0eda4b..5b29e9d7f 100644
--- a/pkg/sentry/socket/unix/unix.go
+++ b/pkg/sentry/socket/unix/unix.go
@@ -323,7 +323,10 @@ func (s *SocketOperations) Bind(t *kernel.Task, sockaddr []byte) *syserr.Error {
 
 			// Create the socket.
 			//
-			// TODO(gvisor.dev/issue/2324): Correctly set file permissions.
+			// Note that the file permissions here are not set correctly (see
+			// gvisor.dev/issue/2324). There is no convenient way to get permissions
+			// on the socket referred to by s, so we will leave this discrepancy
+			// unresolved until VFS2 replaces this code.
 			childDir, err := d.Bind(t, t.FSContext().RootDirectory(), name, bep, fs.FilePermissions{User: fs.PermMask{Read: true}})
 			if err != nil {
 				return syserr.ErrPortInUse
diff --git a/pkg/sentry/socket/unix/unix_vfs2.go b/pkg/sentry/socket/unix/unix_vfs2.go
index 433cde9cb..23db93f33 100644
--- a/pkg/sentry/socket/unix/unix_vfs2.go
+++ b/pkg/sentry/socket/unix/unix_vfs2.go
@@ -197,11 +197,13 @@ func (s *SocketVFS2) Bind(t *kernel.Task, sockaddr []byte) *syserr.Error {
 				Start: start,
 				Path:  path,
 			}
-			err := t.Kernel().VFS().MknodAt(t, t.Credentials(), &pop, &vfs.MknodOptions{
-				// TODO(gvisor.dev/issue/2324): The file permissions should be taken
-				// from s and t.FSContext().Umask() (see net/unix/af_unix.c:unix_bind),
-				// but VFS1 just always uses 0400. Resolve this inconsistency.
-				Mode:     linux.S_IFSOCK | 0400,
+			stat, err := s.vfsfd.Stat(t, vfs.StatOptions{Mask: linux.STATX_MODE})
+			if err != nil {
+				return syserr.FromError(err)
+			}
+			err = t.Kernel().VFS().MknodAt(t, t.Credentials(), &pop, &vfs.MknodOptions{
+				// File permissions correspond to net/unix/af_unix.c:unix_bind.
+				Mode:     linux.FileMode(linux.S_IFSOCK | uint(stat.Mode)&^t.FSContext().Umask()),
 				Endpoint: bep,
 			})
 			if err == syserror.EEXIST {
diff --git a/pkg/sentry/vfs/anonfs.go b/pkg/sentry/vfs/anonfs.go
index b1a998590..981bd8caa 100644
--- a/pkg/sentry/vfs/anonfs.go
+++ b/pkg/sentry/vfs/anonfs.go
@@ -241,6 +241,9 @@ func (fs *anonFilesystem) BoundEndpointAt(ctx context.Context, rp *ResolvingPath
 	if !rp.Final() {
 		return nil, syserror.ENOTDIR
 	}
+	if err := GenericCheckPermissions(rp.Credentials(), MayWrite, anonFileMode, anonFileUID, anonFileGID); err != nil {
+		return nil, err
+	}
 	return nil, syserror.ECONNREFUSED
 }
 
diff --git a/pkg/sentry/vfs/filesystem.go b/pkg/sentry/vfs/filesystem.go
index 70385a21f..1edd584c9 100644
--- a/pkg/sentry/vfs/filesystem.go
+++ b/pkg/sentry/vfs/filesystem.go
@@ -494,7 +494,13 @@ type FilesystemImpl interface {
 
 	// BoundEndpointAt returns the Unix socket endpoint bound at the path rp.
 	//
-	// - If a non-socket file exists at rp, then BoundEndpointAt returns ECONNREFUSED.
+	// Errors:
+	//
+	// - If the file does not have write permissions, then BoundEndpointAt
+	// returns EACCES.
+	//
+	// - If a non-socket file exists at rp, then BoundEndpointAt returns
+	// ECONNREFUSED.
 	BoundEndpointAt(ctx context.Context, rp *ResolvingPath, opts BoundEndpointOptions) (transport.BoundEndpoint, error)
 
 	// PrependPath prepends a path from vd to vd.Mount().Root() to b.
diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index d9095c95f..837e56042 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -365,6 +365,7 @@ cc_binary(
         ":socket_test_util",
         "//test/util:file_descriptor",
         gtest,
+        "//test/util:temp_umask",
         "//test/util:test_main",
         "//test/util:test_util",
     ],
diff --git a/test/syscalls/linux/socket.cc b/test/syscalls/linux/socket.cc
index 3a07ac8d2..703d594a2 100644
--- a/test/syscalls/linux/socket.cc
+++ b/test/syscalls/linux/socket.cc
@@ -13,11 +13,14 @@
 // limitations under the License.
 
 #include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 #include <unistd.h>
 
 #include "gtest/gtest.h"
 #include "test/syscalls/linux/socket_test_util.h"
 #include "test/util/file_descriptor.h"
+#include "test/util/temp_umask.h"
 #include "test/util/test_util.h"
 
 namespace gvisor {
@@ -58,11 +61,69 @@ TEST(SocketTest, ProtocolInet) {
   }
 }
 
+TEST(SocketTest, UnixSocketFileMode) {
+  // TODO(gvisor.dev/issue/1624): Re-enable this test once VFS1 is deleted. It
+  // should pass in VFS2.
+  SKIP_IF(IsRunningOnGvisor());
+
+  FileDescriptor bound =
+      ASSERT_NO_ERRNO_AND_VALUE(Socket(AF_UNIX, SOCK_STREAM, PF_UNIX));
+
+  // The permissions of the file created with bind(2) should be defined by the
+  // permissions of the bound socket and the umask.
+  mode_t sock_perm = 0765, mask = 0123;
+  ASSERT_THAT(fchmod(bound.get(), sock_perm), SyscallSucceeds());
+  TempUmask m(mask);
+
+  struct sockaddr_un addr =
+      ASSERT_NO_ERRNO_AND_VALUE(UniqueUnixAddr(/*abstract=*/false, AF_UNIX));
+  ASSERT_THAT(bind(bound.get(), reinterpret_cast<struct sockaddr*>(&addr),
+                   sizeof(addr)),
+              SyscallSucceeds());
+
+  struct stat statbuf = {};
+  ASSERT_THAT(stat(addr.sun_path, &statbuf), SyscallSucceeds());
+  EXPECT_EQ(statbuf.st_mode, S_IFSOCK | sock_perm & ~mask);
+}
+
+TEST(SocketTest, UnixConnectNeedsWritePerm) {
+  // TODO(gvisor.dev/issue/1624): Re-enable this test once VFS1 is deleted. It
+  // should succeed in VFS2.
+  SKIP_IF(IsRunningOnGvisor());
+
+  FileDescriptor bound =
+      ASSERT_NO_ERRNO_AND_VALUE(Socket(AF_UNIX, SOCK_STREAM, PF_UNIX));
+
+  struct sockaddr_un addr =
+      ASSERT_NO_ERRNO_AND_VALUE(UniqueUnixAddr(/*abstract=*/false, AF_UNIX));
+  ASSERT_THAT(bind(bound.get(), reinterpret_cast<struct sockaddr*>(&addr),
+                   sizeof(addr)),
+              SyscallSucceeds());
+  ASSERT_THAT(listen(bound.get(), 1), SyscallSucceeds());
+
+  // Connect should fail without write perms.
+  ASSERT_THAT(chmod(addr.sun_path, 0500), SyscallSucceeds());
+  FileDescriptor client =
+      ASSERT_NO_ERRNO_AND_VALUE(Socket(AF_UNIX, SOCK_STREAM, PF_UNIX));
+  EXPECT_THAT(connect(client.get(), reinterpret_cast<struct sockaddr*>(&addr),
+                      sizeof(addr)),
+              SyscallFailsWithErrno(EACCES));
+
+  // Connect should succeed with write perms.
+  ASSERT_THAT(chmod(addr.sun_path, 0200), SyscallSucceeds());
+  EXPECT_THAT(connect(client.get(), reinterpret_cast<struct sockaddr*>(&addr),
+                      sizeof(addr)),
+              SyscallSucceeds());
+}
+
 using SocketOpenTest = ::testing::TestWithParam<int>;
 
 // UDS cannot be opened.
 TEST_P(SocketOpenTest, Unix) {
   // FIXME(b/142001530): Open incorrectly succeeds on gVisor.
+  //
+  // TODO(gvisor.dev/issue/1624): Re-enable this test once VFS1 is deleted. It
+  // should succeed in VFS2.
   SKIP_IF(IsRunningOnGvisor());
 
   FileDescriptor bound =