From e74977736d426c7ed913067ea5960545820a39a9 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Wed, 9 Aug 2023 16:51:09 -0700 Subject: [PATCH 1/2] Revert "Merged PR 26443: Restrict named mutex files permissions" This reverts commit d45cd925747b350fa75b42288b4d496225cac812. --- .../pal/src/include/pal/sharedmemory.h | 6 +-- .../pal/src/sharedmemory/sharedmemory.cpp | 48 +++++-------------- src/coreclr/pal/src/synchobj/mutex.cpp | 6 +-- 3 files changed, 18 insertions(+), 42 deletions(-) diff --git a/src/coreclr/pal/src/include/pal/sharedmemory.h b/src/coreclr/pal/src/include/pal/sharedmemory.h index 0cf5a61a5a1d6..c6e5abe97c3a7 100644 --- a/src/coreclr/pal/src/include/pal/sharedmemory.h +++ b/src/coreclr/pal/src/include/pal/sharedmemory.h @@ -91,11 +91,9 @@ 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; @@ -111,12 +109,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 hasCurrentUserAccessOnly, bool setStickyFlag, bool createIfNotExist = true, bool isSystemDirectory = false); + static bool EnsureDirectoryExists(const char *path, bool isGlobalLockAcquired, 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 isSessionScope = true, bool *createdRef = nullptr); + static int CreateOrOpenFile(LPCSTR path, bool createIfNotExist = 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 84c10384b233d..b1d7b3b683045 100644 --- a/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp +++ b/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp @@ -62,7 +62,6 @@ 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; @@ -97,8 +96,6 @@ 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) { @@ -106,13 +103,6 @@ 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; @@ -133,11 +123,11 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( if (isGlobalLockAcquired) { - if (mkdir(path, mode) != 0) + if (mkdir(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { throw SharedMemoryException(static_cast(SharedMemoryError::IO)); } - if (chmod(path, mode) != 0) + if (chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { rmdir(path); throw SharedMemoryException(static_cast(SharedMemoryError::IO)); @@ -152,7 +142,7 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( { throw SharedMemoryException(static_cast(SharedMemoryError::IO)); } - if (chmod(tempPath, mode) != 0) + if (chmod(tempPath, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { rmdir(tempPath); throw SharedMemoryException(static_cast(SharedMemoryError::IO)); @@ -192,18 +182,13 @@ 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 & mode) == mode) + if ((statInfo.st_mode & PermissionsMask_AllUsers_ReadWriteExecute) == PermissionsMask_AllUsers_ReadWriteExecute) { return true; } - if (!createIfNotExist || chmod(path, mode) != 0) + if (!createIfNotExist || chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 0) { - // 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)); - } + throw SharedMemoryException(static_cast(SharedMemoryError::IO)); } return true; } @@ -253,7 +238,7 @@ int SharedMemoryHelpers::OpenDirectory(LPCSTR path) return fileDescriptor; } -int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bool isSessionScope, bool *createdRef) +int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bool *createdRef) { _ASSERTE(path != nullptr); _ASSERTE(path[0] != '\0'); @@ -283,13 +268,12 @@ int SharedMemoryHelpers::CreateOrOpenFile(LPCSTR path, bool createIfNotExist, bo // File does not exist, create the file openFlags |= O_CREAT | O_EXCL; - mode_t mode = isSessionScope ? PermissionsMask_CurrentUser_ReadWrite : PermissionsMask_AllUsers_ReadWrite; - fileDescriptor = Open(path, openFlags, mode); + fileDescriptor = Open(path, openFlags, PermissionsMask_AllUsers_ReadWrite); _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, mode) != 0) + if (chmod(path, PermissionsMask_AllUsers_ReadWrite) != 0) { CloseFile(fileDescriptor); unlink(path); @@ -675,7 +659,7 @@ SharedMemoryProcessDataHeader *SharedMemoryProcessDataHeader::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(SharedMemoryManager::CopySharedMemoryBasePath(filePath)); SharedMemoryHelpers::VerifyStringOperation(filePath.Append('/')); SharedMemoryHelpers::VerifyStringOperation(id.AppendSessionDirectoryName(filePath)); - if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, true /* isGlobalLockAcquired */, id.IsSessionScope(), false /* setStickyFlag */, createIfNotExist)) + if (!SharedMemoryHelpers::EnsureDirectoryExists(filePath, true /* isGlobalLockAcquired */, createIfNotExist)) { _ASSERTE(!createIfNotExist); return nullptr; @@ -688,7 +672,7 @@ SharedMemoryProcessDataHeader *SharedMemoryProcessDataHeader::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(filePath.Append(id.GetName(), id.GetNameCharCount())); bool createdFile; - int fileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(filePath, createIfNotExist, id.IsSessionScope(), &createdFile); + int fileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(filePath, createIfNotExist, &createdFile); if (fileDescriptor == -1) { _ASSERTE(!createIfNotExist); @@ -1163,8 +1147,6 @@ void SharedMemoryManager::AcquireCreationDeletionFileLock() if (!SharedMemoryHelpers::EnsureDirectoryExists( *gSharedFilesPath, false /* isGlobalLockAcquired */, - false /* hasCurrentUserAccessOnly */, - true /* setStickyFlag */, false /* createIfNotExist */, true /* isSystemDirectory */)) { @@ -1172,14 +1154,10 @@ void SharedMemoryManager::AcquireCreationDeletionFileLock() } SharedMemoryHelpers::EnsureDirectoryExists( *s_runtimeTempDirectoryPath, - false /* isGlobalLockAcquired */, - false /* hasCurrentUserAccessOnly */, - true /* setStickyFlag */); + false /* isGlobalLockAcquired */); SharedMemoryHelpers::EnsureDirectoryExists( *s_sharedMemoryDirectoryPath, - false /* isGlobalLockAcquired */, - false /* hasCurrentUserAccessOnly */, - true /* setStickyFlag */); + false /* isGlobalLockAcquired */); 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 d2499c7c2ae27..85bf3da1e79a6 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 */, false /* hasCurrentUserAccessOnly */, true /* setStickyFlag */); + SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */); } // Create the session directory @@ -1130,7 +1130,7 @@ SharedMemoryProcessDataHeader *NamedMutexProcessData::CreateOrOpen( SharedMemoryHelpers::VerifyStringOperation(id->AppendSessionDirectoryName(lockFilePath)); if (created) { - SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */, id->IsSessionScope(), false /* setStickyFlag */); + SharedMemoryHelpers::EnsureDirectoryExists(lockFilePath, true /* isGlobalLockAcquired */); 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, id->IsSessionScope()); + int lockFileDescriptor = SharedMemoryHelpers::CreateOrOpenFile(lockFilePath, created); if (lockFileDescriptor == -1) { _ASSERTE(!created); From 105d3b2424d32997d6e3b7a1d92c065803ce9f80 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Tue, 8 Aug 2023 11:34:32 -0700 Subject: [PATCH 2/2] Restore a change to improve backward compatibility --- src/coreclr/pal/src/sharedmemory/sharedmemory.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp b/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp index b1d7b3b683045..2cec200858211 100644 --- a/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp +++ b/src/coreclr/pal/src/sharedmemory/sharedmemory.cpp @@ -188,7 +188,12 @@ bool SharedMemoryHelpers::EnsureDirectoryExists( } if (!createIfNotExist || chmod(path, PermissionsMask_AllUsers_ReadWriteExecute) != 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; }