[SNOW-1156046] fix toctou vulnerability in EasyLogginConfig

snowflakedb · Apr 19, 2024 · fa7df40 · fa7df40
1 parent 47235fb
commit fa7df40
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 30 deletions.
diff --git 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
@@ -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<EasyLoggingConfigParser>();
 
+        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<ClientConfig>(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);
+            }
+        }
     }
 }