diff --git a/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs b/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs index 28922f7..0d59ea5 100644 --- a/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs +++ b/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs @@ -8,6 +8,8 @@ using System.Collections.Generic; using Microsoft.CodeAnalysis; using Skyline.DataMiner.Utils.SecureCoding.SecureSerialization.Json.Newtonsoft; +using Skyline.DataMiner.Utils.SecureCoding.Analyzers.SecureIO; +using Skyline.DataMiner.Utils.SecureCoding.SecureIO; namespace Skyline.DataMiner.Utils.SecureCoding.Analyzers.Tests.SecureSerialization.Json { @@ -81,5 +83,109 @@ public async Task VerifyNewtonsoftDeserializationUsageDiagnosticAndKnownTypesCod await analyzerVerifier.RunAsync(); } + + [TestMethod] + public async Task VerifyNewtonsoftDeserializationInsecureUsageDefaultSettings() + { + var packages = new PackageIdentity[] + { + new PackageIdentity("Newtonsoft.json","13.0.3"), + } + .ToImmutableArray(); + + var testCode = @"using Newtonsoft.Json; + using Newtonsoft.Json.Serialization; + using System; + using System.Collections.Generic; + + namespace Skyline.DataMiner.Utils.SecureCoding.Analyzers.Tests.SecureSerialization.TestScenarios + { + internal class Insecure_JsonDefaultSettings + { + public void Insecure_TypeNameHandling_All() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.All + }; + + JsonConvert.DefaultSettings = () => + { + return new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.All + }; + }; + } + + public void Insecure_TypeNameHandling_Auto() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Auto + }; + + JsonConvert.DefaultSettings = () => + { + return new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Auto + }; + }; + } + + + public void Insecure_TypeNameHandling_Objects() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Objects + }; + + JsonConvert.DefaultSettings = () => + { + return new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Objects + }; + }; + } + + + public void Insecure_TypeNameHandling_Arrays() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Arrays + }; + + JsonConvert.DefaultSettings = () => + { + return new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Arrays + }; + }; + } + } + }"; + + var expectedDiagnostics = new List() + { + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 14, 33), + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 21, 37), + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 30, 33), + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 37, 37), + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 47, 33), + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 54, 37), + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 64, 33), + AnalyzerVerifierHelper.BuildDiagnosticResult(NewtonsoftDeserializationAnalyzer.DiagnosticId, DiagnosticSeverity.Warning, 71, 37), + }; + + var analyzerVerifier = AnalyzerVerifierHelper.BuildAnalyzerVerifier(testCode, packages); + analyzerVerifier.TestState.ExpectedDiagnostics.AddRange(expectedDiagnostics); + + await analyzerVerifier.RunAsync(); + } } } diff --git a/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs b/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs index 64d31a0..2b8abbd 100644 --- a/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs +++ b/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs @@ -2,16 +2,23 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; +using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; namespace Skyline.DataMiner.Utils.SecureCoding.Analyzers.SecureSerialization.Json { [DiagnosticAnalyzer(LanguageNames.CSharp)] public class NewtonsoftDeserializationAnalyzer : DiagnosticAnalyzer { + private static readonly HashSet insecureTypeNames = new HashSet() + { + "All", "Auto", "Objects", "Arrays" + }; + public const string DiagnosticId = "SLC_SC0004"; - public static DiagnosticDescriptor Rule => new DiagnosticDescriptor( + public static DiagnosticDescriptor DeserializationRule => new DiagnosticDescriptor( DiagnosticId, "Avoid deserializing json strings by using Newtonsoft directly.", "Consider using SecureNewtonsoftDeserialization.DeserializeObject instead. These secure methods are available in the Skyline.DataMiner.Utils.SecureCoding NuGet package.", @@ -21,7 +28,17 @@ public class NewtonsoftDeserializationAnalyzer : DiagnosticAnalyzer isEnabledByDefault: true ); - public override ImmutableArray SupportedDiagnostics { get { return ImmutableArray.Create(Rule); } } + public static DiagnosticDescriptor DefaultSettingsRule => new DiagnosticDescriptor( + DiagnosticId, + "Insecure TypeNameHandling Usage Without SerializationBinder", + "Setting TypeNameHandling to '{0}' at the global level is insecure and may lead to remote code execution during deserialization", + "Usage", + DiagnosticSeverity.Warning, + helpLinkUri: $"https://github.com/SkylineCommunications/Skyline.DataMiner.Utils.SecureCoding/blob/main/docs/Rules/{DiagnosticId}.md", + isEnabledByDefault: true + ); + + public override ImmutableArray SupportedDiagnostics { get { return ImmutableArray.Create(DeserializationRule); } } public override void Initialize(AnalysisContext context) { @@ -29,10 +46,54 @@ public override void Initialize(AnalysisContext context) context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); - context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.InvocationExpression); + context.RegisterSyntaxNodeAction(AnalyzeJsonConvertInvocation, SyntaxKind.InvocationExpression); + + context.RegisterSyntaxNodeAction(AnalyzeJsonConvertAssignment, SyntaxKind.SimpleAssignmentExpression); } - public static void Analyze(SyntaxNodeAnalysisContext context) + private static void AnalyzeJsonConvertAssignment(SyntaxNodeAnalysisContext context) + { + var assignment = (AssignmentExpressionSyntax)context.Node; + if (assignment == null) + { + return; + } + + // Match JsonConvert.DefaultSettings = ... + if (!assignment.Left.ToString().Contains("JsonConvert.DefaultSettings")) + { + return; + } + + // Look for lambda or method returning JsonSerializerSettings + if (!(assignment.Right is ParenthesizedLambdaExpressionSyntax lambda)) + { + return; + } + + var defaultSettings = GetDefaultSettings(lambda); + if (defaultSettings?.Initializer == null) + { + return; + } + + foreach (var expr in defaultSettings.Initializer.Expressions.OfType()) + { + if (expr.Left is IdentifierNameSyntax leftName && + leftName.Identifier.Text == "TypeNameHandling" && + expr.Right is MemberAccessExpressionSyntax rightMember) + { + var typeName = rightMember.Name.Identifier.Text; + + if (insecureTypeNames.Contains(typeName)) + { + context.ReportDiagnostic(Diagnostic.Create(DefaultSettingsRule, expr.GetLocation(), typeName)); + } + } + } + } + + private static void AnalyzeJsonConvertInvocation(SyntaxNodeAnalysisContext context) { if (!(context.Node is InvocationExpressionSyntax node)) { @@ -46,12 +107,30 @@ public static void Analyze(SyntaxNodeAnalysisContext context) context.ReportDiagnostic( Diagnostic.Create( - Rule, + DeserializationRule, node.GetLocation() ) ); } + private static ObjectCreationExpressionSyntax GetDefaultSettings(ParenthesizedLambdaExpressionSyntax lambda) + { + if (lambda.Body is ObjectCreationExpressionSyntax creation) // Handle: () => new JsonSerializerSettings { ... } + { + return creation; + } + else if (lambda.Body is BlockSyntax block) // Handle: () => { return new JsonSerializerSettings { ... }; } + { + return block.DescendantNodes() + .OfType() + .FirstOrDefault(expr => expr.Type.ToString().Contains("JsonSerializerSettings")); + } + else + { + return null; + } + } + private static bool IsJsonConvertDeserialization(InvocationExpressionSyntax node, SyntaxNodeAnalysisContext context) { if (!(node.Expression is MemberAccessExpressionSyntax deserializeObjectAccess)) diff --git a/SecureCoding/SecureCoding.csproj b/SecureCoding/SecureCoding.csproj index 7491958..3fa7ac1 100644 --- a/SecureCoding/SecureCoding.csproj +++ b/SecureCoding/SecureCoding.csproj @@ -37,4 +37,8 @@ + + + +