From d45cd925747b350fa75b42288b4d496225cac812 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 5 Oct 2022 23:37:36 +0000 Subject: [PATCH] Merged PR 26443: Restrict named mutex files permissions Restrict named mutex files permissions This change restricts the permission for files created for named mutexes that are not machine wide. Until now, all named mutexes had underlying files with access to all users. With this change, mutexes that are session local have access for the current user only. In addition to that, sticky bit is set for the `/tmp/.dotnet` and `/tmp/.dotnet/shm` directories to ensure that only creator of a subdirectory in these directories can delete the respective file / subdirectory. Here is an overview of the permissions before and after my change. The differences are shown in **bold**. The `somemutexfile` is a file with a name equal to the named mutex name. This file contains data of the mutex. Before: /tmp RWX for all users, sticky bit set /tmp/.dotnet - RWX for all users /tmp/.dotnet/shm - RWX for all users /tmp/.dotnet/shm/global - RWX for all users /tmp/.dotnet/shm/sessionXXXX - RWX for all users /tmp/.dotnet/shm/global/`somemutexfile`- RW for all users /tmp/.dotnet/shm/sessionXXXX/`somemutexfile` - RW for all users After: /tmp - RWX for all users, sticky bit set /tmp/.dotnet - RWX for all users /tmp/.dotnet/shm - RWX for all users, **sticky bit set** /tmp/.dotnet/shm/global - RWX for all users /tmp/.dotnet/shm/sessionXXXX - RWX for **current user only** /tmp/.dotnet/shm/global/`somemutexfile`- RW for all users /tmp/.dotnet/shm/sessionXXXX/`somemutexfile` - RW for **current user only** --- .../pal/src/include/pal/sharedmemory.h | 6 ++- .../pal/src/sharedmemory/sharedmemory.cpp | 48 ++++++++++++++----- src/coreclr/pal/src/synchobj/mutex.cpp | 6 +-- 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/coreclr/pal/src/include/pal/sharedmemory.h b/src/coreclr/pal/src/include/pal/sharedmemory.h index c6e5abe97c3a7..0cf5a61a5a1d6 100644 --- a/src/coreclr/pal/src/include/pal/sharedmemory.h +++ b/src/coreclr/pal/src/include/pal/sharedmemory.h @@ -91,9 +91,11 @@ class SharedMemoryException class SharedMemoryHelpers { private: + static const mode_t PermissionsMask_CurrentUser_ReadWrite; static const mode_t PermissionsMask_CurrentUser_ReadWriteExecute; static const mode_t PermissionsMask_AllUsers_ReadWrite; static const mode_t PermissionsMask_AllUsers_ReadWriteExecute; + public: static const UINT32 InvalidProcessId; static const SIZE_T InvalidThreadId; @@ -109,12 +111,12 @@ class SharedMemoryHelpers static void BuildSharedFilesPath(PathCharString& destination, const char *suffix, int suffixByteCount); static bool AppendUInt32String(PathCharString& destination, UINT32 value); - static bool EnsureDirectoryExists(const char *path, bool isGlobalLockAcquired, bool createIfNotExist = true, bool isSystemDirectory = false); + static bool EnsureDirectoryExists(const char *path, bool isGlobalLockAcquired, bool hasCurrentUserAccessOnly, bool setStickyFlag, bool createIfNotExist = true, bool isSystemDirectory = false); private: static int Open(LPCSTR path, int flags, mode_t mode = static_cast(0)); public: static int OpenDirectory(LPCSTR path); - static int CreateOrOpenFile(LPCSTR path, bool createIfNotExist = true, bool *createdRef = nullptr); + static int CreateOrOpenFile(LPCSTR path, bool createIfNotExist = true, bool isSessionScope = true, bool *createdRef = nullptr); static void CloseFile(int fileDescriptor); static SIZE_T GetFileSize(int fileDescriptor); diff --git a/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp b/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp index b1d7b3b683045..84c10384b233d 100644 --- a/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp +++ b/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp @@ -62,6 +62,7 @@ DWORD SharedMemoryException::GetErrorCode() const //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // SharedMemoryHelpers +const mode_t SharedMemoryHelpers::PermissionsMask_CurrentUser_ReadWrite = S_IRUSR | S_IWUSR; const mode_t SharedMemoryHelpers::PermissionsMask_CurrentUser_ReadWriteExecute = S_IRUSR | S_IWUSR | S_IXUSR; const mode_t SharedMemoryHelpers::PermissionsMask_AllUsers_ReadWrite = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; @@ -96,6 +97,8 @@ SIZE_T SharedMemoryHelpers::AlignUp(SIZE_T value, SIZE_T alignment) bool SharedMemoryHelpers::EnsureDirectoryExists( const char *path, bool isGlobalLockAcquired, + bool hasCurrentUserAccessOnly, + bool setStickyFlag, bool createIfNotExist, bool isSystemDirectory) { @@ -103,6 +106,13 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( _ASSERTE(!(isSystemDirectory && createIfNotExist)); // should not create or change permissions on system directories _ASSERTE(SharedMemoryManager::IsCreationDeletionProcessLockAcquired()); _ASSERTE(!isGlobalLockAcquired || SharedMemoryManager::IsCreationDeletionFileLockAcquired()); + _ASSERTE(!(setStickyFlag && hasCurrentUserAccessOnly)); // Sticky bit doesn't make sense with current user access only + + mode_t mode = hasCurrentUserAccessOnly ? PermissionsMask_CurrentUser_ReadWriteExecute : PermissionsMask_AllUsers_ReadWriteExecute; + if (setStickyFlag) + { + mode |= S_ISVTX; + } // Check if the path already exists struct stat statInfo; @@ -123,11 +133,11 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( if (isGlobalLockAcquired) { - if (mkdir(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) + if (mkdir(path, mode) != 0) { throw SharedMemoryException(static_cast(SharedMemoryError::IO)); } - if (chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) + if (chmod(path, mode) != 0) { rmdir(path); throw SharedMemoryException(static_cast(SharedMemoryError::IO)); @@ -142,7 +152,7 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( { throw SharedMemoryException(static_cast(SharedMemoryError::IO)); } - if (chmod(tempPath, PermissionsMask_AllUsers_ReadWriteExecute) != 0) + if (chmod(tempPath, mode) != 0) { rmdir(tempPath); throw SharedMemoryException(static_cast(SharedMemoryError::IO)); @@ -182,13 +192,18 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( // For non-system directories (such as gSharedFilesPath/SHARED_MEMORY_RUNTIME_TEMP_DIRECTORY_NAME), // require sufficient permissions for all users and try to update them if requested to create the directory, so that // shared memory files may be shared by all processes on the system. - if ((statInfo.st_mode & PermissionsMask_AllUsers_ReadWriteExecute) == PermissionsMask_AllUsers_ReadWriteExecute) + if ((statInfo.st_mode & mode) == mode) { return true; } - if (!createIfNotExist || chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) + if (!createIfNotExist || chmod(path, mode) != 0) { - throw SharedMemoryException(static_cast(SharedMemoryError::IO)); + // We were not asked to create the path or we weren't able to set the new permissions. + // As a last resort, check that at least the current user has full access. + if ((statInfo.st_mode & PermissionsMask_CurrentUser_ReadWriteExecute) != PermissionsMask_CurrentUser_ReadWriteExecute) + { + throw SharedMemoryException(static_cast(SharedMemoryError::IO)); + } } return true; } @@ -238,7 +253,7 @@ int SharedMemoryHelpers::OpenDirectory(LPCSTR path) return fileDescriptor; } -int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bool *createdRef) +int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bool isSessionScope, bool *createdRef) { _ASSERTE(path != nullptr); _ASSERTE(path[0] != '\0'); @@ -268,12 +283,13 @@ int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bo // File does not exist, create the file openFlags |= O_CREAT | O_EXCL; - fileDescriptor = Open(path, openFlags, PermissionsMask_AllUsers_ReadWrite); + mode_t mode = isSessionScope ? PermissionsMask_CurrentUser_ReadWrite : PermissionsMask_AllUsers_ReadWrite; + fileDescriptor = Open(path, openFlags, mode); _ASSERTE(fileDescriptor != -1); // The permissions mask passed to open() is filtered by the process' permissions umask, so open() may not set all of // the requested permissions. Use chmod() to set the proper permissions. - if (chmod(path, PermissionsMask_AllUsers_ReadWrite) != 0) + if (chmod(path, mode) != 0) { CloseFile(fileDescriptor); unlink(path); @@ -659,7 +675,7 @@ SharedMemoryProcessDataHeader *SharedMemoryProcessDataHeader::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(SharedMemoryManager::CopySharedMemoryBasePath(filePath)); SharedMemoryHelpers::VerifyStringOperation(filePath.Append('/')); SharedMemoryHelpers::VerifyStringOperation(id.AppendSessionDirectoryName(filePath)); - if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, true /* isGlobalLockAcquired */, createIfNotExist)) + if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, true /* isGlobalLockAcquired */, id.IsSessionScope(), false /* setStickyFlag */, createIfNotExist)) { _ASSERTE(!createIfNotExist); return nullptr; @@ -672,7 +688,7 @@ SharedMemoryProcessDataHeader *SharedMemoryProcessDataHeader::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(filePath.Append(id.GetName(), id.GetNameCharCount())); bool createdFile; - int fileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(filePath, createIfNotExist, &createdFile); + int fileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(filePath, createIfNotExist, id.IsSessionScope(), &createdFile); if (fileDescriptor == -1) { _ASSERTE(!createIfNotExist); @@ -1147,6 +1163,8 @@ void SharedMemoryManager::AcquireCreationDeletionFileLock() if (!SharedMemoryHelpers::EnsureDirectoryExists( *gSharedFilesPath, false /* isGlobalLockAcquired */, + false /* hasCurrentUserAccessOnly */, + true /* setStickyFlag */, false /* createIfNotExist */, true /* isSystemDirectory */)) { @@ -1154,10 +1172,14 @@ void SharedMemoryManager::AcquireCreationDeletionFileLock() } SharedMemoryHelpers::EnsureDirectoryExists( *s_runtimeTempDirectoryPath, - false /* isGlobalLockAcquired */); + false /* isGlobalLockAcquired */, + false /* hasCurrentUserAccessOnly */, + true /* setStickyFlag */); SharedMemoryHelpers::EnsureDirectoryExists( *s_sharedMemoryDirectoryPath, - false /* isGlobalLockAcquired */); + false /* isGlobalLockAcquired */, + false /* hasCurrentUserAccessOnly */, + true /* setStickyFlag */); s_creationDeletionLockFileDescriptor = SharedMemoryHelpers::OpenDirectory(*s_sharedMemoryDirectoryPath); if (s_creationDeletionLockFileDescriptor == -1) { diff --git a/src/coreclr/pal/src/synchobj/mutex.cpp b/src/coreclr/pal/src/synchobj/mutex.cpp index 85bf3da1e79a6..d2499c7c2ae27 100644 --- a/src/coreclr/pal/src/synchobj/mutex.cpp +++ b/src/coreclr/pal/src/synchobj/mutex.cpp @@ -1121,7 +1121,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen( SharedMemoryHelpers::BuildSharedFilesPath(lockFilePath, SHARED_MEMORY_LOCK_FILES_DIRECTORY_NAME); if (created) { - SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */); + SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */, false /* hasCurrentUserAccessOnly */, true /* setStickyFlag */); } // Create the session directory @@ -1130,7 +1130,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(id->AppendSessionDirectoryName(lockFilePath)); if (created) { - SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */); + SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */, id->IsSessionScope(), false /* setStickyFlag */); autoCleanup.m_lockFilePath = &lockFilePath; autoCleanup.m_sessionDirectoryPathCharCount = lockFilePath.GetCount(); } @@ -1138,7 +1138,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen( // Create or open the lock file SharedMemoryHelpers::VerifyStringOperation(lockFilePath.Append('/')); SharedMemoryHelpers::VerifyStringOperation(lockFilePath.Append(id->GetName(), id->GetNameCharCount())); - int lockFileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(lockFilePath, created); + int lockFileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(lockFilePath, created, id->IsSessionScope()); if (lockFileDescriptor == -1) { _ASSERTE(!created);