Skip to content

Commit

Permalink
[SNOW-1156046] fix toctou vulnerability in EasyLogginConfig
Browse files Browse the repository at this point in the history
  • Loading branch information
@sfc-gh-erojaslizano
sfc-gh-erojaslizano committed Apr 19, 2024
1 parent 47235fb commit cc5e12c
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 47 deletions.
33 changes: 16 additions & 17 deletions Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,24 +39,23 @@ public class EasyLoggingConfigFinderTest
public void Setup()
{
t_fileOperations = new Mock<FileOperations>();
t_unixOperations = new Mock<UnixOperations>();
t_environmentOperations = new Mock<EnvironmentOperations>();
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()
{
// arrange
MockFileFromEnvironmentalVariable();
MockFileOnDriverPath();
MockFileOnHomePath();

// act
var filePath = t_finder.FindConfigFilePath(InputConfigFilePath);

// assert
Assert.AreEqual(InputConfigFilePath, filePath);
t_fileOperations.VerifyNoOtherCalls();
Expand All @@ -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()
{
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -220,7 +219,7 @@ private static void MockExecutionDirectory()
.Setup(e => e.GetExecutionDirectory())
.Returns(DriverDirectory);
}

private static void MockFileOnHomePathDoesNotExist()
{
t_fileOperations
Expand Down
32 changes: 6 additions & 26 deletions Snowflake.Data/Configuration/EasyLoggingConfigFinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
}
}
}
40 changes: 36 additions & 4 deletions Snowflake.Data/Configuration/EasyLoggingConfigParser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
{
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
}
}
}

0 comments on commit cc5e12c

Please sign in to comment.