dotnet / roslyn-analyzers

MIT License
1.58k stars 464 forks source link

CA3001 (taint analysis) doesn't work with interfaces #7303

Open dbalikhin opened 5 months ago

dbalikhin commented 5 months ago

Analyzer

Diagnostic ID: CA3001: CA3001: Review code for SQL injection vulnerabilities

Analyzer source

SDK: Built-in CA analyzers in .NET 5 SDK or later

Version: SDK 8.0.100

Describe the bug

Steps To Reproduce

  1. Create a new unit test in ReviewCodeForSqlInjectionVulnerabilitiesTests

        [Fact]
        public async Task InterfaceTest()
        {
            await VerifyCSharpWithDependenciesAsync(@"
    namespace VulnerableWebApp
    {
    using System;
    using System.Data;
    using System.Data.SqlClient;
    using System.Linq;
    using System.Web;
    using System.Web.UI;
    
    public partial class WebForm : System.Web.UI.Page
    {
        public interface IBlah
        {
            void ProcessInput(string input);
        }
    
        public class BlahImplementation : IBlah
        {
            public void ProcessInput(string input)
            {
                SqlCommand sqlCommand = new SqlCommand()
                {
                    CommandText = input,
                    CommandType = CommandType.Text,
                };
            }
        }
    
        protected void Page_Load(object sender, EventArgs e)
        {
            IBlah Blah = new BlahImplementation();
            if (Request.Form != null)
            {
                string userInput = Request.Form[""in""];
                //(Blah as BlahImplementation).ProcessInput(userInput);  //  reports vulnerability
                Blah.ProcessInput(userInput);                                             //  doesn't report anything
            }
        }
    
    }
    }
            ",
                GetCSharpResultAt(24, 21, 19, 31, "string SqlCommand.CommandText", "void WebForm.Page_Load(object sender, EventArgs e)", "NameValueCollection HttpRequest.Form", "void WebForm.Page_Load(object sender, EventArgs e)"));
    
        }

Expected behavior

IBlah Blah = new BlahImplementation(); Blah.ProcessInput(userInput); // CA3001 triggered

Actual behavior

0 diagnostics

Additional context

The concrete type seems to work: (Blah as BlahImplementation).ProcessInput(userInput); works.

I'm unsure if it is a part of taint analysis or PointsToAnalysis or something else. Another use case will be DI, but we could try using a default naming convention to resolve the concrete type, e.g. IRepo -> Repo (just remove I).

Sounds like a good topic for discussion.