From f975a404afec063f600da097132b0f9782cbe9df Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Fri, 19 Apr 2024 09:49:01 -0600 Subject: [PATCH 01/13] [SNOW-1156046] fix toctou vulnerability in EasyLogginConfig --- .../EasyLoggingConfigFinderTest.cs | 33 ++++++++------- .../Configuration/EasyLoggingConfigFinder.cs | 32 +++------------ .../Configuration/EasyLoggingConfigParser.cs | 40 +++++++++++++++++-- 3 files changed, 58 insertions(+), 47 deletions(-) diff --git a/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs b/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs index b23fbbf0e..56634afe8 100644 --- a/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs @@ -39,13 +39,12 @@ public class EasyLoggingConfigFinderTest public void Setup() { t_fileOperations = new Mock(); - t_unixOperations = new Mock(); t_environmentOperations = new Mock(); - t_finder = new EasyLoggingConfigFinder(t_fileOperations.Object, t_unixOperations.Object, t_environmentOperations.Object); + t_finder = new EasyLoggingConfigFinder(t_fileOperations.Object, t_environmentOperations.Object); MockHomeDirectory(); MockExecutionDirectory(); } - + [Test] public void TestThatTakesFilePathFromTheInput() { @@ -53,10 +52,10 @@ public void TestThatTakesFilePathFromTheInput() MockFileFromEnvironmentalVariable(); MockFileOnDriverPath(); MockFileOnHomePath(); - + // act var filePath = t_finder.FindConfigFilePath(InputConfigFilePath); - + // assert Assert.AreEqual(InputConfigFilePath, filePath); t_fileOperations.VerifyNoOtherCalls(); @@ -71,14 +70,14 @@ public void TestThatTakesFilePathFromEnvironmentVariableIfInputNotPresent( MockFileFromEnvironmentalVariable(); MockFileOnDriverPath(); MockFileOnHomePath(); - + // act var filePath = t_finder.FindConfigFilePath(inputFilePath); - + // assert Assert.AreEqual(EnvironmentalConfigFilePath, filePath); } - + [Test] public void TestThatTakesFilePathFromDriverLocationWhenNoInputParameterNorEnvironmentVariable() { @@ -88,20 +87,20 @@ public void TestThatTakesFilePathFromDriverLocationWhenNoInputParameterNorEnviro // act var filePath = t_finder.FindConfigFilePath(null); - + // assert Assert.AreEqual(s_driverConfigFilePath, filePath); } - + [Test] public void TestThatTakesFilePathFromHomeLocationWhenNoInputParamEnvironmentVarNorDriverLocation() { // arrange MockFileOnHomePath(); - + // act var filePath = t_finder.FindConfigFilePath(null); - + // assert Assert.AreEqual(s_homeConfigFilePath, filePath); } @@ -138,13 +137,13 @@ public void TestThatConfigFileIsNotUsedIfOthersCanModifyTheConfigFile() Assert.IsNotNull(thrown); Assert.AreEqual(thrown.Message, $"Error due to other users having permission to modify the config file: {s_homeConfigFilePath}"); } - + [Test] public void TestThatReturnsNullIfNoWayOfGettingTheFile() { // act var filePath = t_finder.FindConfigFilePath(null); - + // assert Assert.IsNull(filePath); } @@ -157,7 +156,7 @@ public void TestThatDoesNotFailWhenSearchForOneOfDirectoriesFails() // act var filePath = t_finder.FindConfigFilePath(null); - + // assert Assert.IsNull(filePath); t_environmentOperations.Verify(e => e.GetFolderPath(Environment.SpecialFolder.UserProfile), Times.Once); @@ -186,7 +185,7 @@ public void TestThatDoesNotFailWhenHomeDirectoryDoesNotExist() // act var filePath = t_finder.FindConfigFilePath(null); - + // assert Assert.IsNull(filePath); t_environmentOperations.Verify(e => e.GetFolderPath(Environment.SpecialFolder.UserProfile), Times.Once); @@ -220,7 +219,7 @@ private static void MockExecutionDirectory() .Setup(e => e.GetExecutionDirectory()) .Returns(DriverDirectory); } - + private static void MockFileOnHomePathDoesNotExist() { t_fileOperations diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigFinder.cs b/Snowflake.Data/Configuration/EasyLoggingConfigFinder.cs index d417eb4d8..6d2060a52 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigFinder.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigFinder.cs @@ -19,15 +19,13 @@ internal class EasyLoggingConfigFinder internal const string ClientConfigEnvironmentName = "SF_CLIENT_CONFIG_FILE"; private readonly FileOperations _fileOperations; - private readonly UnixOperations _unixOperations; private readonly EnvironmentOperations _environmentOperations; - public static readonly EasyLoggingConfigFinder Instance = new EasyLoggingConfigFinder(FileOperations.Instance, UnixOperations.Instance, EnvironmentOperations.Instance); + public static readonly EasyLoggingConfigFinder Instance = new EasyLoggingConfigFinder(FileOperations.Instance, EnvironmentOperations.Instance); - internal EasyLoggingConfigFinder(FileOperations fileOperations, UnixOperations unixFileOperations, EnvironmentOperations environmentOperations) + internal EasyLoggingConfigFinder(FileOperations fileOperations, EnvironmentOperations environmentOperations) { _fileOperations = fileOperations; - _unixOperations = unixFileOperations; _environmentOperations = environmentOperations; } @@ -38,16 +36,12 @@ internal EasyLoggingConfigFinder() public virtual string FindConfigFilePath(string configFilePathFromConnectionString) { var configFilePath = GetFilePathFromInputParameter(configFilePathFromConnectionString, "connection string") - ?? GetFilePathEnvironmentVariable() - ?? GetFilePathFromDriverLocation() - ?? GetFilePathFromHomeDirectory(); - if (configFilePath != null) - { - CheckIfValidPermissions(configFilePath); - } + ?? GetFilePathEnvironmentVariable() + ?? GetFilePathFromDriverLocation() + ?? GetFilePathFromHomeDirectory(); return configFilePath; } - + private string GetFilePathEnvironmentVariable() { var filePath = _environmentOperations.GetEnvironmentVariable(ClientConfigEnvironmentName); @@ -100,19 +94,5 @@ private string OnlyIfFileExists(string filePath, string directoryDescription) } return null; } - - private void CheckIfValidPermissions(string filePath) - { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - return; - - // Check if others have permissions to modify the file and fail if so - if (_unixOperations.CheckFileHasAnyOfPermissions(filePath, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite)) - { - var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}"; - s_logger.Error(errorMessage); - throw new Exception(errorMessage); - } - } } } diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index dbc6820b4..466e7b2bd 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -7,8 +7,12 @@ using System.IO; using System.Linq; using System.Reflection; +using System.Runtime.InteropServices; +using Microsoft.IdentityModel.Tokens; +using Mono.Unix; using Newtonsoft.Json; using Newtonsoft.Json.Linq; +using Snowflake.Data.Core.Tools; using Snowflake.Data.Log; namespace Snowflake.Data.Configuration @@ -17,23 +21,36 @@ internal class EasyLoggingConfigParser { private static readonly SFLogger s_logger = SFLoggerFactory.GetLogger(); + private readonly UnixOperations _unixOperations = UnixOperations.Instance; + public static readonly EasyLoggingConfigParser Instance = new EasyLoggingConfigParser(); public virtual ClientConfig Parse(string filePath) { var configFile = TryToReadFile(filePath); - return configFile == null ? null : TryToParseFile(configFile); + return configFile.IsNullOrEmpty() ? null : TryToParseFile(configFile); } private string TryToReadFile(string filePath) { if (string.IsNullOrEmpty(filePath)) { - return null; + return String.Empty; } + try { - return File.ReadAllText(filePath); + using (FileStream fileStream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read)) + { + using (StreamReader reader = new StreamReader(fileStream)) + { + CheckIfValidPermissions(filePath); + + string fileContent = reader.ReadToEnd(); + + return fileContent; + } + } } catch (Exception e) { @@ -45,7 +62,8 @@ private string TryToReadFile(string filePath) private ClientConfig TryToParseFile(string fileContent) { - try { + try + { var config = JsonConvert.DeserializeObject(fileContent); Validate(config); CheckForUnknownFields(fileContent); @@ -80,5 +98,19 @@ private void CheckForUnknownFields(string fileContent) .ToList() .ForEach(unknownKey => s_logger.Warn($"Unknown field from config: {unknownKey.Name}")); } + + private void CheckIfValidPermissions(string filePath) + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + return; + + // Check if others have permissions to modify the file and fail if so + if (_unixOperations.CheckFileHasAnyOfPermissions(filePath, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite)) + { + var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}"; + s_logger.Error(errorMessage); + throw new Exception(errorMessage); + } + } } } From 4334bd9d03cd47c7f21dd2fe8fca817d389ac74d Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Tue, 23 Apr 2024 15:45:18 -0600 Subject: [PATCH 02/13] Add safe handle for net8 --- .../Configuration/EasyLoggingConfigParser.cs | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index 466e7b2bd..4fbb1c729 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -42,10 +42,10 @@ private string TryToReadFile(string filePath) { using (FileStream fileStream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read)) { + CheckIfValidPermissions(fileStream, filePath); + using (StreamReader reader = new StreamReader(fileStream)) { - CheckIfValidPermissions(filePath); - string fileContent = reader.ReadToEnd(); return fileContent; @@ -99,18 +99,28 @@ private void CheckForUnknownFields(string fileContent) .ForEach(unknownKey => s_logger.Warn($"Unknown field from config: {unknownKey.Name}")); } - private void CheckIfValidPermissions(string filePath) + private void CheckIfValidPermissions(FileStream fileStream, string filePath) { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { return; + } - // Check if others have permissions to modify the file and fail if so - if (_unixOperations.CheckFileHasAnyOfPermissions(filePath, FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite)) +#if NET8_0_OR_GREATER + var unixFileMode = File.GetUnixFileMode(fileStream.SafeFileHandle); + var hasPermissions = ((UnixFileMode.OtherRead | UnixFileMode.GroupWrite) & unixFileMode) == 0; +#else + var entitlements = FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite; + var hasPermissions = _unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); +#endif + if (hasPermissions) { - var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}"; - s_logger.Error(errorMessage); - throw new Exception(errorMessage); + return; } + + var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}"; + s_logger.Error(errorMessage); + throw new Exception(errorMessage); } } } From ed361bed7b19432a1310c8b14ddd7885418da726 Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Wed, 24 Apr 2024 10:41:53 -0600 Subject: [PATCH 03/13] test update docker images to dotnet 8 --- ci/image/Dockerfile.dotnet-centos7-net6-build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/image/Dockerfile.dotnet-centos7-net6-build b/ci/image/Dockerfile.dotnet-centos7-net6-build index 9bc9de834..9924387c8 100644 --- a/ci/image/Dockerfile.dotnet-centos7-net6-build +++ b/ci/image/Dockerfile.dotnet-centos7-net6-build @@ -34,7 +34,7 @@ SHELL [ "/usr/bin/scl", "enable", "devtoolset-8"] # dotnet-sdk-6.0 RUN rpm -Uvh https://packages.microsoft.com/config/centos/7/packages-microsoft-prod.rpm -RUN yum -y install --exclude 1:openssl-libs-1.0.2k-26.el7_9.x86_64 dotnet-sdk-6.0 +RUN yum -y install --exclude 1:openssl-libs-1.0.2k-26.el7_9.x86_64 dotnet-sdk-8.0 # workspace RUN mkdir -p /home/user && \ From 2b450bee77b9c014f8594f502d46f8f07b570fb9 Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Tue, 14 May 2024 14:59:40 -0600 Subject: [PATCH 04/13] fix failing test --- .../IntegrationTests/EasyLoggingIT.cs | 14 +++----------- .../Configuration/EasyLoggingConfigParser.cs | 2 +- ci/image/Dockerfile.dotnet-centos7-net6-build | 2 +- 3 files changed, 5 insertions(+), 13 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs b/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs index fd2e79409..fad264195 100644 --- a/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs @@ -73,13 +73,9 @@ public void TestFailToEnableEasyLoggingForWrongConfiguration() } [Test] + [Platform(Exclude="Win", Reason="skip test on Windows")] public void TestFailToEnableEasyLoggingWhenConfigHasWrongPermissions() { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - Assert.Ignore("skip test on Windows"); - } - // arrange var configFilePath = CreateConfigTempFile(s_workingDirectory, Config("WARN", s_workingDirectory)); Syscall.chmod(configFilePath, FilePermissions.S_IRUSR | FilePermissions.S_IWUSR | FilePermissions.S_IWGRP); @@ -91,19 +87,15 @@ public void TestFailToEnableEasyLoggingWhenConfigHasWrongPermissions() var thrown = Assert.Throws(() => conn.Open()); // assert - Assert.That(thrown.Message, Does.Contain("Connection string is invalid: Unable to initialize session")); + Assert.That(thrown.Message, Does.Contain("Connection string is invalid: Unable to connect")); Assert.IsFalse(EasyLoggerManager.HasEasyLoggingAppender()); } } [Test] + [Platform(Exclude="Win", Reason="skip test on Windows")] public void TestFailToEnableEasyLoggingWhenLogDirectoryNotAccessible() { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - Assert.Ignore("skip test on Windows"); - } - // arrange var configFilePath = CreateConfigTempFile(s_workingDirectory, Config("WARN", "/")); using (IDbConnection conn = new SnowflakeDbConnection()) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index 4fbb1c729..f11dabdd2 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -111,7 +111,7 @@ private void CheckIfValidPermissions(FileStream fileStream, string filePath) var hasPermissions = ((UnixFileMode.OtherRead | UnixFileMode.GroupWrite) & unixFileMode) == 0; #else var entitlements = FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite; - var hasPermissions = _unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); + var hasPermissions = !_unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); #endif if (hasPermissions) { diff --git a/ci/image/Dockerfile.dotnet-centos7-net6-build b/ci/image/Dockerfile.dotnet-centos7-net6-build index 9924387c8..9bc9de834 100644 --- a/ci/image/Dockerfile.dotnet-centos7-net6-build +++ b/ci/image/Dockerfile.dotnet-centos7-net6-build @@ -34,7 +34,7 @@ SHELL [ "/usr/bin/scl", "enable", "devtoolset-8"] # dotnet-sdk-6.0 RUN rpm -Uvh https://packages.microsoft.com/config/centos/7/packages-microsoft-prod.rpm -RUN yum -y install --exclude 1:openssl-libs-1.0.2k-26.el7_9.x86_64 dotnet-sdk-8.0 +RUN yum -y install --exclude 1:openssl-libs-1.0.2k-26.el7_9.x86_64 dotnet-sdk-6.0 # workspace RUN mkdir -p /home/user && \ From 23b08ab89e8be002a1bf04daa6a793487f7a59d1 Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Tue, 14 May 2024 16:09:49 -0600 Subject: [PATCH 05/13] remove unit test as is no longer valid scenario --- .../EasyLoggingConfigFinderTest.cs | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs b/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs index 56634afe8..1552745a7 100644 --- a/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs @@ -118,26 +118,6 @@ public void TestThatTakesFilePathFromHomeDirectoryWhenNoOtherWaysPossible() Assert.AreEqual(s_homeConfigFilePath, filePath); } - [Test] - public void TestThatConfigFileIsNotUsedIfOthersCanModifyTheConfigFile() - { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - Assert.Ignore("skip test on Windows"); - } - - // arrange - MockFileOnHomePath(); - MockHasFlagReturnsTrue(); - - // act - var thrown = Assert.Throws(() => t_finder.FindConfigFilePath(null)); - - // assert - Assert.IsNotNull(thrown); - Assert.AreEqual(thrown.Message, $"Error due to other users having permission to modify the config file: {s_homeConfigFilePath}"); - } - [Test] public void TestThatReturnsNullIfNoWayOfGettingTheFile() { From 933d7507e0c000355a12b4d436a68d76b85b218f Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Wed, 15 May 2024 13:52:29 -0600 Subject: [PATCH 06/13] fix failing test on net8.0 --- Snowflake.Data/Configuration/EasyLoggingConfigParser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index f11dabdd2..01b63b8a4 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -108,7 +108,7 @@ private void CheckIfValidPermissions(FileStream fileStream, string filePath) #if NET8_0_OR_GREATER var unixFileMode = File.GetUnixFileMode(fileStream.SafeFileHandle); - var hasPermissions = ((UnixFileMode.OtherRead | UnixFileMode.GroupWrite) & unixFileMode) == 0; + var hasPermissions = !(((UnixFileMode.OtherRead | UnixFileMode.GroupWrite) & unixFileMode) == 0); #else var entitlements = FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite; var hasPermissions = !_unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); From c970a1be09855918da66b93dfb9fd6657164fadb Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Wed, 15 May 2024 16:25:07 -0600 Subject: [PATCH 07/13] fix remaining dotnet 8.0 test --- Snowflake.Data/Configuration/EasyLoggingConfigParser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index 01b63b8a4..b247037ba 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -108,7 +108,7 @@ private void CheckIfValidPermissions(FileStream fileStream, string filePath) #if NET8_0_OR_GREATER var unixFileMode = File.GetUnixFileMode(fileStream.SafeFileHandle); - var hasPermissions = !(((UnixFileMode.OtherRead | UnixFileMode.GroupWrite) & unixFileMode) == 0); + var hasPermissions = !(((UnixFileMode.GroupWrite | UnixFileMode.OtherWrite) & unixFileMode) != 0); #else var entitlements = FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite; var hasPermissions = !_unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); From 29fa29f41bf68cbdf6809ec7449ca09a6b8da2fe Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Thu, 16 May 2024 14:28:52 -0600 Subject: [PATCH 08/13] change variable name hasPermissions --- .../Configuration/EasyLoggingConfigParser.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index b247037ba..c1d75ee72 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -108,19 +108,17 @@ private void CheckIfValidPermissions(FileStream fileStream, string filePath) #if NET8_0_OR_GREATER var unixFileMode = File.GetUnixFileMode(fileStream.SafeFileHandle); - var hasPermissions = !(((UnixFileMode.GroupWrite | UnixFileMode.OtherWrite) & unixFileMode) != 0); + var hasGroupOrOtherWritePermissions = (((UnixFileMode.GroupWrite | UnixFileMode.OtherWrite) & unixFileMode) != 0); #else var entitlements = FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite; - var hasPermissions = !_unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); + var hasGroupOrOtherWritePermissions = _unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); #endif - if (hasPermissions) + if (hasGroupOrOtherWritePermissions) { - return; + var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}"; + s_logger.Error(errorMessage); + throw new Exception(errorMessage); } - - var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}"; - s_logger.Error(errorMessage); - throw new Exception(errorMessage); } } } From 0a6fb5200a44b7bb77b711c23042aef9cb877a13 Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Wed, 10 Jul 2024 15:25:52 -0600 Subject: [PATCH 09/13] implement fix from knordezco --- .../Configuration/EasyLoggingConfigParser.cs | 58 ++++++++----------- Snowflake.Data/Snowflake.Data.csproj | 2 +- 2 files changed, 25 insertions(+), 35 deletions(-) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index c1d75ee72..77a93d01d 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -7,9 +7,11 @@ using System.IO; using System.Linq; using System.Reflection; -using System.Runtime.InteropServices; +using System.Security; +using System.Text; using Microsoft.IdentityModel.Tokens; using Mono.Unix; +using Mono.Unix.Native; using Newtonsoft.Json; using Newtonsoft.Json.Linq; using Snowflake.Data.Core.Tools; @@ -21,8 +23,6 @@ internal class EasyLoggingConfigParser { private static readonly SFLogger s_logger = SFLoggerFactory.GetLogger(); - private readonly UnixOperations _unixOperations = UnixOperations.Instance; - public static readonly EasyLoggingConfigParser Instance = new EasyLoggingConfigParser(); public virtual ClientConfig Parse(string filePath) @@ -31,6 +31,16 @@ public virtual ClientConfig Parse(string filePath) return configFile.IsNullOrEmpty() ? null : TryToParseFile(configFile); } + /// + /// ReadAllText function reads contents of a file at the making sure, in a way not prone to race-conditions, that: + /// - A file is owned by the same user as effective user of the current process. + /// - A file is owned by the same group as effective group of the current process. + /// - A file permissions do not include `forbiddenPermissions` (any others' permissions by default) + /// + /// + /// The file path of the configuration file + /// + /// An exception will be thrown if the file is no owned by the user or group private string TryToReadFile(string filePath) { if (string.IsNullOrEmpty(filePath)) @@ -40,17 +50,19 @@ private string TryToReadFile(string filePath) try { - using (FileStream fileStream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read)) - { - CheckIfValidPermissions(fileStream, filePath); + FileAccessPermissions forbiddenPermissions = FileAccessPermissions.OtherReadWriteExecute; + var fileInfo = new UnixFileInfo(path: filePath); - using (StreamReader reader = new StreamReader(fileStream)) - { - string fileContent = reader.ReadToEnd(); + using var handle = fileInfo.OpenRead(); + if (handle.OwnerUser.UserId != Syscall.geteuid()) + throw new SecurityException("Attempting to read a file not owned by the effective user of the current process"); + if (handle.OwnerGroup.GroupId != Syscall.getegid()) + throw new SecurityException("Attempting to read a file not owned by the effective group of the current process"); + if ((handle.FileAccessPermissions & forbiddenPermissions) != 0) + throw new SecurityException("Attempting to read a file with too broad permissions assigned"); - return fileContent; - } - } + using var streamReader = new StreamReader(handle, Encoding.Default); + return streamReader.ReadToEnd(); } catch (Exception e) { @@ -98,27 +110,5 @@ private void CheckForUnknownFields(string fileContent) .ToList() .ForEach(unknownKey => s_logger.Warn($"Unknown field from config: {unknownKey.Name}")); } - - private void CheckIfValidPermissions(FileStream fileStream, string filePath) - { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - return; - } - -#if NET8_0_OR_GREATER - var unixFileMode = File.GetUnixFileMode(fileStream.SafeFileHandle); - var hasGroupOrOtherWritePermissions = (((UnixFileMode.GroupWrite | UnixFileMode.OtherWrite) & unixFileMode) != 0); -#else - var entitlements = FileAccessPermissions.GroupWrite | FileAccessPermissions.OtherWrite; - var hasGroupOrOtherWritePermissions = _unixOperations.CheckFileHasAnyOfPermissions(filePath, entitlements); -#endif - if (hasGroupOrOtherWritePermissions) - { - var errorMessage = $"Error due to other users having permission to modify the config file: {filePath}"; - s_logger.Error(errorMessage); - throw new Exception(errorMessage); - } - } } } diff --git a/Snowflake.Data/Snowflake.Data.csproj b/Snowflake.Data/Snowflake.Data.csproj index 01fc2891e..c72636fac 100644 --- a/Snowflake.Data/Snowflake.Data.csproj +++ b/Snowflake.Data/Snowflake.Data.csproj @@ -13,7 +13,7 @@ Snowflake 4.0.0 Full - 7.3 + 8.0 From bfbf91b2700df387104e1a5b6b8f7179f25a79c5 Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Tue, 16 Jul 2024 15:54:00 -0600 Subject: [PATCH 10/13] fix failing tests --- .../IntegrationTests/EasyLoggingIT.cs | 3 ++- .../Configuration/EasyLoggingConfigParser.cs | 12 +++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs b/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs index fad264195..f0a43ea26 100644 --- a/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/EasyLoggingIT.cs @@ -1,3 +1,4 @@ +using System; using System.Data; using System.IO; using System.Runtime.InteropServices; @@ -87,7 +88,7 @@ public void TestFailToEnableEasyLoggingWhenConfigHasWrongPermissions() var thrown = Assert.Throws(() => conn.Open()); // assert - Assert.That(thrown.Message, Does.Contain("Connection string is invalid: Unable to connect")); + Assert.That(thrown.Message, Does.Contain("Attempting to read a file with too broad permissions assigned")); Assert.IsFalse(EasyLoggerManager.HasEasyLoggingAppender()); } } diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index 77a93d01d..ab30c4cd5 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -14,6 +14,8 @@ using Mono.Unix.Native; using Newtonsoft.Json; using Newtonsoft.Json.Linq; +using Snowflake.Data.Client; +using Snowflake.Data.Core; using Snowflake.Data.Core.Tools; using Snowflake.Data.Log; @@ -50,16 +52,16 @@ private string TryToReadFile(string filePath) try { - FileAccessPermissions forbiddenPermissions = FileAccessPermissions.OtherReadWriteExecute; + FileAccessPermissions forbiddenPermissions = FileAccessPermissions.OtherWrite | FileAccessPermissions.GroupWrite; var fileInfo = new UnixFileInfo(path: filePath); using var handle = fileInfo.OpenRead(); if (handle.OwnerUser.UserId != Syscall.geteuid()) - throw new SecurityException("Attempting to read a file not owned by the effective user of the current process"); + throw new SnowflakeDbException(SFError.INTERNAL_ERROR,"Attempting to read a file not owned by the effective user of the current process"); if (handle.OwnerGroup.GroupId != Syscall.getegid()) - throw new SecurityException("Attempting to read a file not owned by the effective group of the current process"); + throw new SnowflakeDbException(SFError.INTERNAL_ERROR,"Attempting to read a file not owned by the effective group of the current process"); if ((handle.FileAccessPermissions & forbiddenPermissions) != 0) - throw new SecurityException("Attempting to read a file with too broad permissions assigned"); + throw new SnowflakeDbException( SFError.INTERNAL_ERROR,"Attempting to read a file with too broad permissions assigned"); using var streamReader = new StreamReader(handle, Encoding.Default); return streamReader.ReadToEnd(); @@ -68,7 +70,7 @@ private string TryToReadFile(string filePath) { var errorMessage = "Finding easy logging configuration failed"; s_logger.Error(errorMessage, e); - throw new Exception(errorMessage); + throw; } } From d2cc095ebe96d18b31d9fb220d311dc7b318a9ee Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Fri, 19 Jul 2024 14:57:25 -0600 Subject: [PATCH 11/13] fix remaining failing test --- .../EasyLoggingConfigParserTest.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigParserTest.cs b/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigParserTest.cs index 847d57c51..2d516cc97 100644 --- a/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigParserTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigParserTest.cs @@ -33,7 +33,7 @@ public static void AfterAll() { Directory.Delete(s_workingDirectory, true); } - + [Test] public void TestThatParsesConfigFile() { @@ -59,12 +59,12 @@ public void TestThatParsesConfigFileWithNullValues(string filePath) // act var config = parser.Parse(filePath); - + // assert Assert.IsNotNull(config); Assert.IsNotNull(config.CommonProps); Assert.IsNull(config.CommonProps.LogLevel); - Assert.IsNull(config.CommonProps.LogPath); + Assert.IsNull(config.CommonProps.LogPath); } [Test] @@ -77,23 +77,23 @@ public void TestThatReturnsNullWhenNothingToParse(string noFilePath) // act var config = parser.Parse(noFilePath); - + // assert Assert.IsNull(config); } - + [Test] public void TestThatFailsWhenTheFileDoesNotExist() { // arrange var parser = new EasyLoggingConfigParser(); - + // act - var thrown = Assert.Throws(() => parser.Parse(NotExistingFilePath)); - + var thrown = Assert.Throws(() => parser.Parse(NotExistingFilePath)); + // assert Assert.IsNotNull(thrown); - Assert.AreEqual("Finding easy logging configuration failed", thrown.Message); + Assert.AreEqual("No such file or directory", thrown.Message); } [Test, TestCaseSource(nameof(WrongConfigFiles))] From 65f15f9c5a86b7472e8f230d5fadcf5e988ec02d Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Mon, 29 Jul 2024 14:32:06 -0600 Subject: [PATCH 12/13] fix failing on windows OS --- .../Configuration/EasyLoggingConfigParser.cs | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index ab30c4cd5..42278792c 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -7,6 +7,7 @@ using System.IO; using System.Linq; using System.Reflection; +using System.Runtime.InteropServices; using System.Security; using System.Text; using Microsoft.IdentityModel.Tokens; @@ -52,19 +53,18 @@ private string TryToReadFile(string filePath) try { - FileAccessPermissions forbiddenPermissions = FileAccessPermissions.OtherWrite | FileAccessPermissions.GroupWrite; - var fileInfo = new UnixFileInfo(path: filePath); + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + using var streamReader = new StreamReader(filePath, Encoding.Default); + return streamReader.ReadToEnd(); + } + else + { + var handle = VerifyUnixPermissions(filePath); - using var handle = fileInfo.OpenRead(); - if (handle.OwnerUser.UserId != Syscall.geteuid()) - throw new SnowflakeDbException(SFError.INTERNAL_ERROR,"Attempting to read a file not owned by the effective user of the current process"); - if (handle.OwnerGroup.GroupId != Syscall.getegid()) - throw new SnowflakeDbException(SFError.INTERNAL_ERROR,"Attempting to read a file not owned by the effective group of the current process"); - if ((handle.FileAccessPermissions & forbiddenPermissions) != 0) - throw new SnowflakeDbException( SFError.INTERNAL_ERROR,"Attempting to read a file with too broad permissions assigned"); - - using var streamReader = new StreamReader(handle, Encoding.Default); - return streamReader.ReadToEnd(); + using var streamReader = new StreamReader(handle, Encoding.Default); + return streamReader.ReadToEnd(); + } } catch (Exception e) { @@ -74,6 +74,21 @@ private string TryToReadFile(string filePath) } } + private static UnixStream VerifyUnixPermissions(string filePath) + { + FileAccessPermissions forbiddenPermissions = FileAccessPermissions.OtherWrite | FileAccessPermissions.GroupWrite; + var fileInfo = new UnixFileInfo(path: filePath); + + using var handle = fileInfo.OpenRead(); + if (handle.OwnerUser.UserId != Syscall.geteuid()) + throw new SnowflakeDbException(SFError.INTERNAL_ERROR, "Attempting to read a file not owned by the effective user of the current process"); + if (handle.OwnerGroup.GroupId != Syscall.getegid()) + throw new SnowflakeDbException(SFError.INTERNAL_ERROR, "Attempting to read a file not owned by the effective group of the current process"); + if ((handle.FileAccessPermissions & forbiddenPermissions) != 0) + throw new SnowflakeDbException(SFError.INTERNAL_ERROR, "Attempting to read a file with too broad permissions assigned"); + return handle; + } + private ClientConfig TryToParseFile(string fileContent) { try From 58c4cd354ca8ba54eee5954e7ae5e5ae01f2c4b2 Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Mon, 29 Jul 2024 15:41:37 -0600 Subject: [PATCH 13/13] remove disposable object from method --- Snowflake.Data/Configuration/EasyLoggingConfigParser.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs index 42278792c..582badd9d 100644 --- a/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs +++ b/Snowflake.Data/Configuration/EasyLoggingConfigParser.cs @@ -55,14 +55,14 @@ private string TryToReadFile(string filePath) { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - using var streamReader = new StreamReader(filePath, Encoding.Default); + var streamReader = new StreamReader(filePath, Encoding.Default); return streamReader.ReadToEnd(); } else { var handle = VerifyUnixPermissions(filePath); - using var streamReader = new StreamReader(handle, Encoding.Default); + var streamReader = new StreamReader(handle, Encoding.Default); return streamReader.ReadToEnd(); } } @@ -79,7 +79,7 @@ private static UnixStream VerifyUnixPermissions(string filePath) FileAccessPermissions forbiddenPermissions = FileAccessPermissions.OtherWrite | FileAccessPermissions.GroupWrite; var fileInfo = new UnixFileInfo(path: filePath); - using var handle = fileInfo.OpenRead(); + var handle = fileInfo.OpenRead(); if (handle.OwnerUser.UserId != Syscall.geteuid()) throw new SnowflakeDbException(SFError.INTERNAL_ERROR, "Attempting to read a file not owned by the effective user of the current process"); if (handle.OwnerGroup.GroupId != Syscall.getegid())