Skip to content

Commit

Permalink
Migrate S5773: Implement the rule in the new SE (#7580)
Browse files Browse the repository at this point in the history
  • Loading branch information
Tim-Pohlmann authored Jul 20, 2023
1 parent 5558364 commit 0b7202c
Show file tree
Hide file tree
Showing 14 changed files with 558 additions and 380 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ public class SymbolicExecutionRunner : SymbolicExecutionRunnerBase
{
// ToDo: This should be migrated to SymbolicExecutionRunnerBase.AllRules.
private static readonly ImmutableArray<ISymbolicExecutionAnalyzer> SonarRules = ImmutableArray.Create<ISymbolicExecutionAnalyzer>(
new SonarRules.ConditionEvaluatesToConstant(),
new SonarRules.RestrictDeserializedTypes());
new SonarRules.ConditionEvaluatesToConstant());

public SymbolicExecutionRunner() : this(AnalyzerConfiguration.AlwaysEnabled) { }

Expand All @@ -53,7 +52,8 @@ public SymbolicExecutionRunner() : this(AnalyzerConfiguration.AlwaysEnabled) { }
.Add(PublicMethodArgumentsShouldBeCheckedForNull.S3900, CreateFactory<PublicMethodArgumentsShouldBeCheckedForNull, SonarRules.PublicMethodArgumentsShouldBeCheckedForNull>())
.Add(CalculationsShouldNotOverflow.S3949, CreateFactory<CalculationsShouldNotOverflow>())
.Add(ObjectsShouldNotBeDisposedMoreThanOnce.S3966, CreateFactory<ObjectsShouldNotBeDisposedMoreThanOnce, SonarRules.ObjectsShouldNotBeDisposedMoreThanOnce>())
.Add(EmptyCollectionsShouldNotBeEnumerated.S4158, CreateFactory<EmptyCollectionsShouldNotBeEnumerated, SonarRules.EmptyCollectionsShouldNotBeEnumerated>());
.Add(EmptyCollectionsShouldNotBeEnumerated.S4158, CreateFactory<EmptyCollectionsShouldNotBeEnumerated, SonarRules.EmptyCollectionsShouldNotBeEnumerated>())
.Add(RestrictDeserializedTypes.S5773, CreateFactory<RestrictDeserializedTypes, SonarRules.RestrictDeserializedTypes>());

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => base.SupportedDiagnostics.Concat(SonarRules.SelectMany(x => x.SupportedDiagnostics)).ToImmutableArray();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Runtime.Serialization;

namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks.CSharp;

public sealed class RestrictDeserializedTypes : RestrictDeserializedTypesBase
Expand All @@ -26,5 +28,20 @@ public sealed class RestrictDeserializedTypes : RestrictDeserializedTypesBase

protected override DiagnosticDescriptor Rule => S5773;

public override bool ShouldExecute() => false;
public override bool ShouldExecute() => true;

protected override bool IsBindToTypeMethod(SyntaxNode methodDeclaration) =>
methodDeclaration is MethodDeclarationSyntax { Identifier.Text: nameof(SerializationBinder.BindToType), ParameterList.Parameters.Count: 2 } syntax
&& syntax.EnsureCorrectSemanticModelOrDefault(SemanticModel) is { } semanticModel
&& syntax.ParameterList.Parameters[0].Type.IsKnownType(KnownType.System_String, semanticModel)
&& syntax.ParameterList.Parameters[1].Type.IsKnownType(KnownType.System_String, semanticModel);

protected override bool IsResolveTypeMethod(SyntaxNode methodDeclaration) =>
methodDeclaration is MethodDeclarationSyntax { Identifier.Text: "ResolveType", ParameterList.Parameters.Count: 1 } syntax
&& syntax.EnsureCorrectSemanticModelOrDefault(SemanticModel) is { } semanticModel
&& syntax.ParameterList.Parameters[0].Type.IsKnownType(KnownType.System_String, semanticModel);

protected override bool ThrowsOrReturnsNull(SyntaxNode methodDeclaration) => ((MethodDeclarationSyntax)methodDeclaration).ThrowsOrReturnsNull();

protected override SyntaxToken GetIdentifier(SyntaxNode methodDeclaration) => ((MethodDeclarationSyntax)methodDeclaration).Identifier;
}

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,15 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using SonarAnalyzer.SymbolicExecution.Constraints;
namespace SonarAnalyzer.SymbolicExecution.Constraints;

namespace SonarAnalyzer.SymbolicExecution.Sonar.Constraints
internal sealed class SerializationConstraint : SymbolicConstraint
{
internal sealed class SerializationConstraint : SymbolicConstraint
{
public static readonly SerializationConstraint Unsafe = new(ConstraintKind.SerializationUnsafe);
public static readonly SerializationConstraint Safe = new(ConstraintKind.SerializationSafe);
public static readonly SerializationConstraint Unsafe = new(ConstraintKind.SerializationUnsafe);
public static readonly SerializationConstraint Safe = new(ConstraintKind.SerializationSafe);

public override SymbolicConstraint Opposite =>
this == Safe ? Unsafe : Safe;
public override SymbolicConstraint Opposite =>
this == Safe ? Unsafe : Safe;

private SerializationConstraint(ConstraintKind kind) : base(kind) { }
}
private SerializationConstraint(ConstraintKind kind) : base(kind) { }
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ internal static ILocalReferenceOperationWrapper ToLocalReference(this IOperation
internal static IMethodReferenceOperationWrapper ToMethodReference(this IOperation operation) =>
IMethodReferenceOperationWrapper.FromOperation(operation);

internal static IObjectCreationOperationWrapper ToObjectCreation(this IOperation operation) =>
IObjectCreationOperationWrapper.FromOperation(operation);

internal static IPropertyReferenceOperationWrapper ToPropertyReference(this IOperation operation) =>
IPropertyReferenceOperationWrapper.FromOperation(operation);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Runtime.Serialization;
using SonarAnalyzer.SymbolicExecution.Constraints;

namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks;

public abstract class RestrictDeserializedTypesBase : SymbolicRuleCheck
Expand All @@ -26,4 +29,166 @@ public abstract class RestrictDeserializedTypesBase : SymbolicRuleCheck
protected const string MessageFormat = "{0}";
private const string RestrictTypesMessage = "Restrict types of objects allowed to be deserialized.";
private const string VerifyMacMessage = "Serialized data signature (MAC) should be verified.";

private static readonly KnownType[] FormattersWithBinder = new[]
{
KnownType.System_Runtime_Serialization_Formatters_Binary_BinaryFormatter,
KnownType.System_Runtime_Serialization_NetDataContractSerializer,
KnownType.System_Runtime_Serialization_Formatters_Soap_SoapFormatter
};
private static readonly KnownType JavaScriptSerializer = KnownType.System_Web_Script_Serialization_JavaScriptSerializer;
private static readonly KnownType LosFormatter = KnownType.System_Web_UI_LosFormatter;
private static readonly KnownType[] TypesWithDeserializeMethod = FormattersWithBinder.Append(JavaScriptSerializer).ToArray();

private readonly Dictionary<ISymbol, SyntaxNode> additionalLocationsForSymbols = new();
private readonly Dictionary<IOperation, SyntaxNode> additionalLocationsForOperations = new();

protected abstract bool IsBindToTypeMethod(SyntaxNode methodDeclaration);
protected abstract bool IsResolveTypeMethod(SyntaxNode methodDeclaration);
protected abstract bool ThrowsOrReturnsNull(SyntaxNode methodDeclaration);
protected abstract SyntaxToken GetIdentifier(SyntaxNode methodDeclaration);

protected override ProgramState PostProcessSimple(SymbolicContext context)
{
var state = context.State;
var operation = context.Operation.Instance;
if (operation.Kind == OperationKindEx.ObjectCreation)
{
return operation.Type.IsAny(FormattersWithBinder)
? state.SetOperationConstraint(operation, SerializationConstraint.Unsafe)
: ProcessOtherSerializerCreations(state, operation.ToObjectCreation());
}
else if (operation.AsAssignment() is { } assignment)
{
if (ProcessBinderAssignment(state, assignment) is { } binderProcessedState)
{
return binderProcessedState;
}
else if (AdditionalLocation(state, assignment.Value) is { } methodDeclaration
&& assignment.Target.TrackedSymbol() is { } symbol)
{
additionalLocationsForSymbols[symbol] = methodDeclaration;
}
}
else if (UnsafeDeserialization(state, operation) is { } invocation)
{
var methodDeclaration = AdditionalLocation(state, invocation.Instance);
var additionalLocations = methodDeclaration is not null
? new[] { GetIdentifier(methodDeclaration).GetLocation() }
: Array.Empty<Location>();
ReportIssue(operation, additionalLocations, RestrictTypesMessage);
}
return state;
}

private ProgramState ProcessOtherSerializerCreations(ProgramState state, IObjectCreationOperationWrapper objectCreation)
{
if (UnsafeJavaScriptSerializer(state, objectCreation, out var resolveTypeDeclaration))
{
additionalLocationsForOperations[objectCreation.WrappedOperation] = resolveTypeDeclaration;
return state.SetOperationConstraint(objectCreation.WrappedOperation, SerializationConstraint.Unsafe);
}
else if (objectCreation.Type.Is(LosFormatter) && !EnableMacIsTrue(state, objectCreation))
{
ReportIssue(objectCreation.WrappedOperation, VerifyMacMessage);
}
return state;
}

private bool UnsafeJavaScriptSerializer(ProgramState state, IObjectCreationOperationWrapper objectCreation, out SyntaxNode resolveTypeDeclaration)
{
resolveTypeDeclaration = null;
return objectCreation.Type.Is(JavaScriptSerializer)
&& objectCreation.Arguments.Length == 1
&& UnsafeResolver(state, objectCreation.Arguments[0].ToArgument().Value, out resolveTypeDeclaration);
}

private bool UnsafeResolver(ProgramState state, IOperation operation, out SyntaxNode resolveTypeDeclaration)
{
resolveTypeDeclaration = null;
operation = state.ResolveCaptureAndUnwrapConversion(operation);
if (operation.Type.Is(KnownType.System_Web_Script_Serialization_SimpleTypeResolver))
{
return true;
}
else if (DeclarationCandidates(operation)?.FirstOrDefault(IsResolveTypeMethod) is { } declaration
&& !ThrowsOrReturnsNull(declaration))
{
resolveTypeDeclaration = declaration;
return true;
}
else
{
return false;
}
}

private static bool EnableMacIsTrue(ProgramState state, IObjectCreationOperationWrapper objectCreation) =>
objectCreation.ArgumentValue("enableMac") is { } enableMacArgument
&& state[enableMacArgument].HasConstraint(BoolConstraint.True) is true;

private ProgramState ProcessBinderAssignment(ProgramState state, IAssignmentOperationWrapper assignment)
{
if (BinderAssignmentInstance(state, assignment) is { } instance)
{
var constraint = BinderIsSafe(state, assignment, out var bindToTypeDeclaration)
? SerializationConstraint.Safe
: SerializationConstraint.Unsafe;
if (constraint == SerializationConstraint.Unsafe)
{
additionalLocationsForOperations[instance] = bindToTypeDeclaration;
}
state = state.SetOperationConstraint(instance, constraint);

if (instance.TrackedSymbol() is { } symbol)
{
if (constraint == SerializationConstraint.Unsafe)
{
additionalLocationsForSymbols[symbol] = bindToTypeDeclaration;
}
state = state.SetSymbolConstraint(symbol, constraint);
}
return state;
}
return null;
}

private static IOperation BinderAssignmentInstance(ProgramState state, IAssignmentOperationWrapper assignment) =>
state.ResolveCaptureAndUnwrapConversion(assignment.Target).AsPropertyReference() is { Property.Name: nameof(IFormatter.Binder), Instance: { } propertyInstance }
&& propertyInstance.Type.IsAny(FormattersWithBinder)
? state.ResolveCaptureAndUnwrapConversion(propertyInstance)
: null;

private bool BinderIsSafe(ProgramState state, IAssignmentOperationWrapper assignment, out SyntaxNode bindToTypeDeclaration)
{
if (state[assignment.Value]?.HasConstraint(ObjectConstraint.Null) is true)
{
bindToTypeDeclaration = null;
return false;
}
else
{
bindToTypeDeclaration = DeclarationCandidates(state.ResolveCaptureAndUnwrapConversion(assignment.Value)).FirstOrDefault(IsBindToTypeMethod);
return bindToTypeDeclaration is null || ThrowsOrReturnsNull(bindToTypeDeclaration);
}
}

private static IEnumerable<SyntaxNode> DeclarationCandidates(IOperation operation) =>
operation.Type?.DeclaringSyntaxReferences.SelectMany(x => x.GetSyntax().ChildNodes());

private SyntaxNode AdditionalLocation(ProgramState state, IOperation operation)
{
operation = state.ResolveCaptureAndUnwrapConversion(operation);
return additionalLocationsForOperations.TryGetValue(operation, out var methodDeclaration)
|| (operation.TrackedSymbol() is { } symbol && additionalLocationsForSymbols.TryGetValue(symbol, out methodDeclaration))
? methodDeclaration
: null;
}

private static IInvocationOperationWrapper? UnsafeDeserialization(ProgramState state, IOperation operation) =>
operation.AsInvocation() is { } invocation
&& invocation.TargetMethod.Name == nameof(IFormatter.Deserialize)
&& invocation.TargetMethod.ContainingType.IsAny(TypesWithDeserializeMethod)
&& state[invocation.Instance]?.HasConstraint(SerializationConstraint.Unsafe) is true
? invocation : null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,14 @@ public void Init(SonarAnalysisContext sonarContext, SonarSyntaxNodeReportingCont
protected void ReportIssue(IOperationWrapperSonar operation, params object[] messageArgs) =>
ReportIssue(operation.Instance, messageArgs);

protected void ReportIssue(IOperation operation, params object[] messageArgs)
protected void ReportIssue(IOperation operation, params object[] messageArgs) => ReportIssue(operation, null, messageArgs);

protected void ReportIssue(IOperation operation, IEnumerable<Location> additionalLocations, params object[] messageArgs)
{
var location = operation.Syntax.GetLocation();
if (reportedDiagnostics.Add(location))
{
context.ReportIssue(Diagnostic.Create(Rule, location, messageArgs));
context.ReportIssue(Diagnostic.Create(Rule, location, additionalLocations, messageArgs));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,14 @@ public void RestrictDeserializedTypesFormatters_Sonar()
.Verify();
}

[Ignore] // ToDo: Remove after implementation
[TestMethod]
public void RestrictDeserializedTypesFormatters_Roslyn_CS() =>
roslynCS.AddPaths("RestrictDeserializedTypes.cs")
.Verify();

[Ignore] // ToDo: Remove after implementation
[TestMethod]
public void RestrictDeserializedTypesFormatters_Roslyn_CSharp8() =>
roslynCS.AddPaths("RestrictDeserializedTypes.cs")
roslynCS.AddPaths("RestrictDeserializedTypes.CSharp8.cs")
.WithOptions(ParseOptionsHelper.FromCSharp8)
.Verify();

Expand All @@ -71,7 +69,6 @@ public void RestrictDeserializedTypes_DoesNotRaiseIssuesForTestProject_Sonar() =
.AddTestReference()
.VerifyNoIssueReported();

[Ignore] // ToDo: Remove after implementation
[TestMethod]
public void RestrictDeserializedTypes_DoesNotRaiseIssuesForTestProject_Roslyn_CS() =>
roslynCS.AddPaths("RestrictDeserializedTypes.cs")
Expand All @@ -84,7 +81,6 @@ public void RestrictDeserializedTypesJavaScriptSerializer_Sonar() =>
.WithOptions(ParseOptionsHelper.FromCSharp8)
.Verify();

[Ignore] // ToDo: Remove after implementation
[TestMethod]
public void RestrictDeserializedTypesJavaScriptSerializer_Roslyn_CS() =>
roslynCS.AddPaths("RestrictDeserializedTypes.JavaScriptSerializer.cs")
Expand All @@ -96,7 +92,6 @@ public void RestrictDeserializedTypesLosFormatter_Sonar() =>
.WithOptions(ParseOptionsHelper.FromCSharp8)
.Verify();

[Ignore] // ToDo: Remove after implementation
[TestMethod]
public void RestrictDeserializedTypesLosFormatter_Roslyn_CS() =>
roslynCS.AddPaths("RestrictDeserializedTypes.LosFormatter.cs")
Expand All @@ -118,7 +113,6 @@ public void RestrictDeserializedTypesFormatters_Sonar_CSharp9() =>
.WithTopLevelStatements()
.Verify();

[Ignore] // ToDo: Remove after implementation
[TestMethod]
public void RestrictDeserializedTypesFormatters_Roslyn_CSharp9() =>
roslynCS.AddPaths("RestrictDeserializedTypes.CSharp9.cs")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using SonarAnalyzer.SymbolicExecution.Sonar.Constraints;
using SonarAnalyzer.SymbolicExecution.Constraints;

namespace SonarAnalyzer.UnitTest.SymbolicExecution.Sonar.Constraints
{
Expand Down
Loading

0 comments on commit 0b7202c

Please sign in to comment.