From 8844d4630d383bb435218af7cff99ed506920594 Mon Sep 17 00:00:00 2001 From: Paul Ming Date: Wed, 16 Oct 2019 17:29:52 -0700 Subject: [PATCH 1/3] Some unit tests --- ...alizerBinaryFormatterWithoutBinderTests.cs | 56 +++++++++++++++++++ .../UseXmlReaderForSchemaReadTests.cs | 24 ++++++++ 2 files changed, 80 insertions(+) diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs index 1c3e33d58d..52cd31b28b 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs @@ -1196,5 +1196,61 @@ public BookRecord DeserializeBookRecord(byte[] bytes) } }", GetEditorConfigAdditionalFile(editorConfigText), expected); } + + [Fact] + public void Deserialize_SharedBinderInstance_MaybeDiagnostic() + { + VerifyCSharpWithMyBinderDefined(@" +using System; +using System.IO; +using System.Runtime.Serialization; +using System.Runtime.Serialization.Formatters.Binary; +using System.Runtime.Remoting.Messaging; + +namespace Blah +{ + public class Program + { + public static SerializationBinder B { get; set; } + + private object DoDeserialization(Stream stream) + { + BinaryFormatter f = new BinaryFormatter(); + f.Binder = B ?? throw new Exception(""Expected a non-null SerializationBinder""); + return f.Deserialize(stream); + } + } +}", + GetCSharpResultAt(18, 20, BinderNotSetRule, "object BinaryFormatter.Deserialize(Stream serializationStream)")); + + // Ideally, we'd be able to tell f.Binder is non-null. + } + + [Fact] + public void Deserialize_SharedBinderInstanceIntermediate_MaybeDiagnostic() + { + VerifyCSharpWithMyBinderDefined(@" +using System; +using System.IO; +using System.Runtime.Serialization; +using System.Runtime.Serialization.Formatters.Binary; +using System.Runtime.Remoting.Messaging; + +namespace Blah +{ + public class Program + { + public static SerializationBinder B { get; set; } + + private object DoDeserialization(Stream stream) + { + BinaryFormatter f = new BinaryFormatter(); + SerializationBinder b = B ?? throw new Exception(""Expected a non-null SerializationBinder""); + f.Binder = b; + return f.Deserialize(stream); + } + } +}"); + } } } diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseXmlReaderForSchemaReadTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseXmlReaderForSchemaReadTests.cs index 8ee3acf821..2a22638caf 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseXmlReaderForSchemaReadTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseXmlReaderForSchemaReadTests.cs @@ -66,6 +66,30 @@ public void TestMethod(XmlReader reader, ValidationEventHandler validationEventH }"); } + [Fact] + public void XmlSchemaReadDocSample1_Solution() + { + VerifyCSharp(@" +using System.IO; +using System.Xml; +using System.Xml.Schema; + +class TestClass +{ + public XmlSchema Test + { + get + { + var src = """"; + TextReader tr = new StreamReader(src); + XmlReader reader = XmlReader.Create(tr, new XmlReaderSettings() { XmlResolver = null }); + XmlSchema schema = XmlSchema.Read(reader , null); + return schema; + } + } +}"); + } + protected override DiagnosticAnalyzer GetBasicDiagnosticAnalyzer() { return new UseXmlReaderForSchemaRead(); From f474a64d29125f3b523e42a9cb611e385668c2f9 Mon Sep 17 00:00:00 2001 From: Paul Ming Date: Tue, 22 Oct 2019 13:21:32 -0700 Subject: [PATCH 2/3] Renaming methods --- ...seInsecureDeserializerBinaryFormatterWithoutBinderTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs index 52cd31b28b..1457a4ff8c 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs @@ -1198,7 +1198,7 @@ public BookRecord DeserializeBookRecord(byte[] bytes) } [Fact] - public void Deserialize_SharedBinderInstance_MaybeDiagnostic() + public void Deserialize_SharedBinderInstance_Diagnostic() { VerifyCSharpWithMyBinderDefined(@" using System; @@ -1227,7 +1227,7 @@ private object DoDeserialization(Stream stream) } [Fact] - public void Deserialize_SharedBinderInstanceIntermediate_MaybeDiagnostic() + public void Deserialize_SharedBinderInstanceIntermediate_NoDiagnostic() { VerifyCSharpWithMyBinderDefined(@" using System; From 51dd600c9e40ec9be16da3853372354597d8c088 Mon Sep 17 00:00:00 2001 From: Paul Ming Date: Tue, 22 Oct 2019 14:13:14 -0700 Subject: [PATCH 3/3] PropertySetAnalysis: Handling assignments to flow captures when tracking properties --- ...alizerBinaryFormatterWithoutBinderTests.cs | 7 +-- ...sis.PropertySetDataFlowOperationVisitor.cs | 54 ++++++++++++++----- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs index 1457a4ff8c..73b92561e2 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs @@ -1198,7 +1198,7 @@ public BookRecord DeserializeBookRecord(byte[] bytes) } [Fact] - public void Deserialize_SharedBinderInstance_Diagnostic() + public void Deserialize_SharedBinderInstance_NoDiagnostic() { VerifyCSharpWithMyBinderDefined(@" using System; @@ -1220,10 +1220,7 @@ private object DoDeserialization(Stream stream) return f.Deserialize(stream); } } -}", - GetCSharpResultAt(18, 20, BinderNotSetRule, "object BinaryFormatter.Deserialize(Stream serializationStream)")); - - // Ideally, we'd be able to tell f.Binder is non-null. +}"); } [Fact] diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PropertySetAnalysis/PropertySetAnalysis.PropertySetDataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PropertySetAnalysis/PropertySetAnalysis.PropertySetDataFlowOperationVisitor.cs index 0f70bae52e..54256e0ec1 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PropertySetAnalysis/PropertySetAnalysis.PropertySetDataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PropertySetAnalysis/PropertySetAnalysis.PropertySetDataFlowOperationVisitor.cs @@ -241,19 +241,9 @@ protected override PropertySetAbstractValue VisitAssignmentOperation(IAssignment AnalysisEntity targetAnalysisEntity = null; if (operation.Target.Kind == OperationKind.FlowCaptureReference) { - PointsToAbstractValue lValuePointsToAbstractValue = this.GetPointsToAbstractValue(operation.Target); - if (lValuePointsToAbstractValue.LValueCapturedOperations.Count == 1) + if (this.TryUnwrapFlowCaptureReference(operation.Target, out IOperation lValueOperation, OperationKind.PropertyReference, OperationKind.FieldReference)) { - IOperation lValueOperation = lValuePointsToAbstractValue.LValueCapturedOperations.First(); - if (lValueOperation.Kind == OperationKind.FieldReference - || lValueOperation.Kind == OperationKind.PropertyReference) - { - this.AnalysisEntityFactory.TryCreate(lValueOperation, out targetAnalysisEntity); - } - } - else - { - Debug.Fail("Can LValues FlowCaptureReferences have more than one operation?"); + this.AnalysisEntityFactory.TryCreate(lValueOperation, out targetAnalysisEntity); } } else @@ -295,7 +285,14 @@ protected override PropertySetAbstractValue VisitAssignmentOperation(IAssignment } } - if (operation.Target is IPropertyReferenceOperation propertyReferenceOperation + IPropertyReferenceOperation propertyReferenceOperation = operation.Target as IPropertyReferenceOperation; + if (propertyReferenceOperation == null && operation.Target.Kind == OperationKind.FlowCaptureReference) + { + this.TryUnwrapFlowCaptureReference(operation.Target, out IOperation lValue, OperationKind.PropertyReference); + propertyReferenceOperation = lValue as IPropertyReferenceOperation; + } + + if (propertyReferenceOperation != null && propertyReferenceOperation.Instance != null && this.TrackedTypeSymbols.Any(s => propertyReferenceOperation.Instance.Type.GetBaseTypesAndThis().Contains(s)) && this.DataFlowAnalysisContext.PropertyMappers.TryGetPropertyMapper( @@ -698,6 +695,37 @@ private void MergeInterproceduralResults(IOperation originalOperation) this._visitedLambdas.Add(lambdaOperation); } } + + /// + /// Attempts to find the underlying IOperation that a flow capture reference refers to. + /// + /// Operation that may be a flow capture reference to look at. + /// The found underlying operation, if any. + /// Kinds of operations to look for. + /// True if found, false otherwise. + private bool TryUnwrapFlowCaptureReference(IOperation flowCaptureReferenceOperation, out IOperation unwrappedOperation, params OperationKind[] kinds) + { + unwrappedOperation = null; + if (flowCaptureReferenceOperation != null && flowCaptureReferenceOperation.Kind == OperationKind.FlowCaptureReference) + { + PointsToAbstractValue lValuePointsToAbstractValue = this.GetPointsToAbstractValue(flowCaptureReferenceOperation); + if (lValuePointsToAbstractValue.LValueCapturedOperations.Count == 1) + { + IOperation lValueOperation = lValuePointsToAbstractValue.LValueCapturedOperations.First(); + if (kinds == null || kinds.Contains(lValueOperation.Kind)) + { + unwrappedOperation = lValueOperation; + return true; + } + } + else + { + Debug.Fail("Can LValues FlowCaptureReferences have more than one operation?"); + } + } + + return false; + } } } }