Roslyn-analyzers: CA2100 rule (SQL queries potential vulnerability) doesn't take into account that a type may be sealed

[Mikejo5000]

Originally opened by @Crono1981 and ported from MicrosoftDocs/visualstudio-docs#4482

The rule says that the ToString method of a type will also cause the warning to happen, because a malicious user could override the method.

Shouldn't the ToString method of a sealed type solves this concern?

[Evangelink]

As long as the ToString method of the sealed class is "safe" I think it is ok.

[dotpaul]

I'm not sure what the scenario of a malicious user overriding the ToString() method is (the malicious user is modifying source code? the application is loading and executing arbitrary code?), but I don't think sealed or not matters, just whether ToString() can potentially be used as a SQL injection. You could have a parameter type like so:

public class SomeParameterContainer
{
     public string UserInput { get; set; }
     public sealed string ToString() => UserInput;
}

public class ExampleClass
{
    public void ExampleMethod(SomeParameterContainer p)
    {
        Command c = new Command();
        c.CommandText = p.ToString();
    }
}

We could have an allow list for "safe" types to ToString(), but in my opinion, parameterized SQL is so easy that people should always do that and not worry whether or not they're introducing SQL injection vulnerabilities.

[Crono1981]

@dotpaul You are correct, but then again, the same could be said about any string method, not just ToString as the rule implies.

A sealed type can at least still be safely implemented and make it (almost) impossible for a malicious developer to use its ToString method as a conductor for Sql Injection.

[dotpaul]

Hi @Crono1981,

@dotpaul You are correct, but then again, the same could be said about any string method, not just ToString as the rule implies.

Yep, absolutely.

The rule's implementation seems to check if the SQL command text is a literal constant or a literal value known at compile time through dataflow analysis (@mavasani, I think maybe you ported this rule? Please yell if I'm missing something about its behavior).

Thinking about it more, the doc's example of this producing a violation:

int x = 10;
string query = "SELECT TOP " + x.ToString() + " FROM Table";

is probably more of a limitation of ValueContent DFA's current implementation not computing the value of query. Maybe that's what @mavasani was thinking all along? 🙂

[mavasani]

Yes, @dotpaul's understanding is correct.

1. Current implementation matches the documentation
2. If desired, we can enhance DFA to consider ToString invocations on objects with constant value and built-in types as effectively constant.
3. Considering ToString invocations on sealed types as safe is likely a risky design change. I tend to agree with @dotpaul that users should always be using parameterized SQL as a good practice.

[Crono1981]

Considering ToString invocations on sealed types as safe is likely a risky design change. I tend to agree with @dotpaul that users should always be using parameterized SQL as a good practice.

I can understand that. 👍

In my scenario, queries are created from an abstraction library that by itself has no dependency to ADO.NET. It creates SELECT queries out of typed schemas, among other things. It is safe from Sql Injection but it ain't something I can add parameters to.

I suppose I could introduce an additional library which would have both the abstraction library and ADO.NET as dependencies, then have IDbCommand setters of some sort which could then be used by apps / client libraries. This way I could limit the removal of warnings down to these setters instead.

In any case, I appreciate the input. Thanks, gentlemen. Have a nice day. 😃

[dotpaul]

Ah, thanks for letting us know your scenario, @Crono1981.

@mavasani, I think the documentation should say that the rule is making sure the command text is known at compile time, but in some cases won't know command text value and can produce false positives. Right now it says:

Notice that this rule is violated when the ToString method of a type is used explicitly or implicitly to construct the query string. The following is an example.

int x = 10;
string query = "SELECT TOP " + x.ToString() + " FROM Table";

The rule is violated because a malicious user can override the ToString() method.

The rule also is violated when ToString is used implicitly.

int x = 10;
string query = String.Format("SELECT TOP {0} FROM Table", x);

The whole "malicious user can override the ToString() method" thing is a red herring.

I can probably get to updating the docs in the next week or two, unless someone beats me to it.

[dotpaul]

Updated doc has been published. Thanks again for bringing this up, @Crono1981 !