summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
Diffstat
-rw-r--r--pkg/sentry/fsimpl/ext/filesystem.go5
-rw-r--r--pkg/sentry/fsimpl/kernfs/filesystem.go5
-rw-r--r--pkg/sentry/fsimpl/tmpfs/filesystem.go3
-rw-r--r--pkg/sentry/socket/unix/unix.go5
-rw-r--r--pkg/sentry/socket/unix/unix_vfs2.go12
-rw-r--r--pkg/sentry/vfs/anonfs.go3
-rw-r--r--pkg/sentry/vfs/filesystem.go8
-rw-r--r--test/syscalls/linux/BUILD1
-rw-r--r--test/syscalls/linux/socket.cc61
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 =
generated by cgit v1.2.3 (git 2.39.1) at 2024-11-02 10:31:53 +0000