From cc5e12c720b6fe4576139213b579d9871be53dd7 Mon Sep 17 00:00:00 2001 From: Steven Lizano Date: Fri, 19 Apr 2024 09:49:01 -0600 Subject: [PATCH] [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); + } + } } }