Merge pull request #4501 from dotpaul/fix4495

Find tainted data parameter SyntaxNodes more robustly, and don't treat ASP.NET MVC Controller property setter parameters as tainted

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs

@@ -3668,6 +3668,25 @@ public void DoSomething(string input)
             await csharpTest.RunAsync();
         }
 
+        [Fact]
+        public async Task AspNetMvcController_HasPropertySetter()
+        {
+            await VerifyCSharpWithDependenciesAsync(@"
+using System.Data.SqlClient;
+
+public class MyController
+{
+    public void DoSomething(string input)
+    {
+    }
+
+    public string AString 
+    {
+        set { _ = value; }
+    }
+}");
+        }
+
         [Fact]
         public async Task TaintFunctionArguments()
         {

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs

@@ -119,7 +119,27 @@ protected override TaintedDataAbstractValue GetDefaultValueForParameterOnEntry(I
             {
                 if (this.DataFlowAnalysisContext.SourceInfos.IsSourceParameter(parameter, WellKnownTypeProvider))
                 {
-                    return TaintedDataAbstractValue.CreateTainted(parameter, parameter.DeclaringSyntaxReferences[0].GetSyntax(), this.OwningSymbol);
+                    // Location of the parameter, so we can track where the tainted data appears in code.
+                    // The parameter itself may not have any DeclaringSyntaxReferences, e.g. 'value' inside property setters.
+                    SyntaxNode parameterSyntaxNode;
+                    if (!parameter.DeclaringSyntaxReferences.IsEmpty)
+                    {
+                        parameterSyntaxNode = parameter.DeclaringSyntaxReferences[0].GetSyntax();
+                    }
+                    else if (!parameter.ContainingSymbol.DeclaringSyntaxReferences.IsEmpty)
+                    {
+                        parameterSyntaxNode = parameter.ContainingSymbol.DeclaringSyntaxReferences[0].GetSyntax();
+                    }
+                    else
+                    {
+                        // Unless there are others, the only case we have for parameters being tainted data sources is inside
+                        // ASP.NET Core MVC controller action methods (see WebInputSources.cs), so those parameters should
+                        // always be declared somewhere.
+                        Debug.Fail("Can we have a tainted data parameter with no syntax references?");
+                        return ValueDomain.UnknownOrMayBeValue;
+                    }
+
+                    return TaintedDataAbstractValue.CreateTainted(parameter, parameterSyntaxNode, this.OwningSymbol);
                 }
 
                 return ValueDomain.UnknownOrMayBeValue;

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/WebInputSources.cs

@@ -89,6 +89,7 @@ static WebInputSources()
                         if (methodSymbol.DeclaredAccessibility != Accessibility.Public
                             || methodSymbol.IsConstructor()
                             || methodSymbol.IsStatic
+                            || methodSymbol.MethodKind != MethodKind.Ordinary
                             || methodSymbol.HasDerivedMethodAttribute(wellKnownTypeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.MicrosoftAspNetCoreMvcNonActionAttribute)))
                         {
                             return false;