From 68bc2507899f98518d94989ab5a602f630cb24f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C3=A1vio=20Jacinto?= Date: Wed, 9 Jul 2025 17:24:33 +0100 Subject: [PATCH 1/2] Improved NewtonsoftDeserializationAnalyzer. --- ...tonsoftDeserializationAnalyzerUnitTests.cs | 167 ++++++++++++++++++ .../Json/NewtonsoftDeserializationAnalyzer.cs | 102 ++++++++++- 2 files changed, 264 insertions(+), 5 deletions(-) diff --git a/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs b/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs index 28922f7..29e0ddb 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,170 @@ 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(); + } + + [TestMethod] + public async Task VerifyNewtonsoftDeserializationSecureUsageDefaultSettings() + { + 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 Secure_JsonDefaultSettings + { + public void Secure_TypeNameHandling_All() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.All, + SerializationBinder = new DefaultSerializationBinder() + }; + } + + public void Secure_TypeNameHandling_Auto() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Auto, + SerializationBinder = new DefaultSerializationBinder() + }; + } + + public void Secure_TypeNameHandling_Objects() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Objects, + SerializationBinder = new DefaultSerializationBinder() + }; + } + + public void Secure_TypeNameHandling_Arrays() + { + JsonConvert.DefaultSettings = () => new JsonSerializerSettings + { + TypeNameHandling = TypeNameHandling.Arrays, + SerializationBinder = new DefaultSerializationBinder() + }; + } + } + }"; + + var analyzerVerifier = AnalyzerVerifierHelper.BuildAnalyzerVerifier(testCode, packages); + + await analyzerVerifier.RunAsync(); + } } } diff --git a/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs b/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs index 64d31a0..66d69e9 100644 --- a/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs +++ b/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs @@ -2,16 +2,24 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; +using System.Collections.Generic; using System.Collections.Immutable; +using System.Data; +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 +29,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", + "TypeNameHandling.{0} is insecure without a configured SerializationBinder. This may allow 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 +47,55 @@ 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; + + // 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; + } + + if (HasSerializationBinder(defaultSettings)) + { + 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 +109,41 @@ public static void Analyze(SyntaxNodeAnalysisContext context) context.ReportDiagnostic( Diagnostic.Create( - Rule, + DeserializationRule, node.GetLocation() ) ); } + private static bool HasSerializationBinder(ObjectCreationExpressionSyntax defaultSettings) + { + return defaultSettings.Initializer.Expressions + .OfType() + .Any(expr => + { + var name = expr.Left.ToString(); + return name.Contains("Binder") || name.Contains("SerializationBinder"); + }); + } + + 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)) From baf506b520ba38cbdf1c2dfd390e75b1d951b79f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fl=C3=A1vio=20Jacinto?= Date: Thu, 10 Jul 2025 09:54:09 +0100 Subject: [PATCH 2/2] Redefined JsonConvert.DefaultSettings logic --- ...tonsoftDeserializationAnalyzerUnitTests.cs | 61 ------------------- .../Json/NewtonsoftDeserializationAnalyzer.cs | 23 ++----- SecureCoding/SecureCoding.csproj | 4 ++ 3 files changed, 9 insertions(+), 79 deletions(-) diff --git a/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs b/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs index 29e0ddb..0d59ea5 100644 --- a/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs +++ b/Analyzers/Analyzers.Test/SecureSerialization/Json/NewtonsoftDeserializationAnalyzerUnitTests.cs @@ -187,66 +187,5 @@ public void Insecure_TypeNameHandling_Arrays() await analyzerVerifier.RunAsync(); } - - [TestMethod] - public async Task VerifyNewtonsoftDeserializationSecureUsageDefaultSettings() - { - 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 Secure_JsonDefaultSettings - { - public void Secure_TypeNameHandling_All() - { - JsonConvert.DefaultSettings = () => new JsonSerializerSettings - { - TypeNameHandling = TypeNameHandling.All, - SerializationBinder = new DefaultSerializationBinder() - }; - } - - public void Secure_TypeNameHandling_Auto() - { - JsonConvert.DefaultSettings = () => new JsonSerializerSettings - { - TypeNameHandling = TypeNameHandling.Auto, - SerializationBinder = new DefaultSerializationBinder() - }; - } - - public void Secure_TypeNameHandling_Objects() - { - JsonConvert.DefaultSettings = () => new JsonSerializerSettings - { - TypeNameHandling = TypeNameHandling.Objects, - SerializationBinder = new DefaultSerializationBinder() - }; - } - - public void Secure_TypeNameHandling_Arrays() - { - JsonConvert.DefaultSettings = () => new JsonSerializerSettings - { - TypeNameHandling = TypeNameHandling.Arrays, - SerializationBinder = new DefaultSerializationBinder() - }; - } - } - }"; - - var analyzerVerifier = AnalyzerVerifierHelper.BuildAnalyzerVerifier(testCode, packages); - - await analyzerVerifier.RunAsync(); - } } } diff --git a/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs b/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs index 66d69e9..2b8abbd 100644 --- a/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs +++ b/Analyzers/Analyzers/SecureSerialization/Json/NewtonsoftDeserializationAnalyzer.cs @@ -4,7 +4,6 @@ using Microsoft.CodeAnalysis.Diagnostics; using System.Collections.Generic; using System.Collections.Immutable; -using System.Data; using System.Linq; namespace Skyline.DataMiner.Utils.SecureCoding.Analyzers.SecureSerialization.Json @@ -32,7 +31,7 @@ public class NewtonsoftDeserializationAnalyzer : DiagnosticAnalyzer public static DiagnosticDescriptor DefaultSettingsRule => new DiagnosticDescriptor( DiagnosticId, "Insecure TypeNameHandling Usage Without SerializationBinder", - "TypeNameHandling.{0} is insecure without a configured SerializationBinder. This may allow remote code execution during deserialization.", + "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", @@ -55,6 +54,10 @@ public override void Initialize(AnalysisContext 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")) @@ -74,11 +77,6 @@ private static void AnalyzeJsonConvertAssignment(SyntaxNodeAnalysisContext conte return; } - if (HasSerializationBinder(defaultSettings)) - { - return; - } - foreach (var expr in defaultSettings.Initializer.Expressions.OfType()) { if (expr.Left is IdentifierNameSyntax leftName && @@ -115,17 +113,6 @@ private static void AnalyzeJsonConvertInvocation(SyntaxNodeAnalysisContext conte ); } - private static bool HasSerializationBinder(ObjectCreationExpressionSyntax defaultSettings) - { - return defaultSettings.Initializer.Expressions - .OfType() - .Any(expr => - { - var name = expr.Left.ToString(); - return name.Contains("Binder") || name.Contains("SerializationBinder"); - }); - } - private static ObjectCreationExpressionSyntax GetDefaultSettings(ParenthesizedLambdaExpressionSyntax lambda) { if (lambda.Body is ObjectCreationExpressionSyntax creation) // Handle: () => new JsonSerializerSettings { ... } 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 @@ + + + +