search

dotnet / roslyn-analyzers

MIT License
1.6k stars 468 forks source link

Port FxCop rule CA2100: ReviewSqlQueriesForSecurityVulnerabilities #628

Closed ghost closed 6 years ago

ghost commented 9 years ago

Title: Review SQL queries for security vulnerabilities

Description:

A method sets the System.Data.IDbCommand.CommandText property by using a string that is built from a string argument to the method. This rule assumes that the string argument contains user input. A SQL command string that is built from user input is vulnerable to SQL injection attacks.

Dependency: Dataflow

Notes:

ghost commented 8 years ago

This analyzer would require dataflow analysis (for example, to verify that information provided by a user contributed to the construction of a SQL command). At present, Roslyn does not support dataflow analysis.

For this reason, we have decided not to port CA2100 from FxCop to Roslyn analyzers. If in future Roslyn supports dataflow analysis, we will revisit this check.

jinujoseph commented 6 years ago

@mavasani moving this to @333fred as discussed. @333fred , can you work with Manish to understand what are our option here?

333fred commented 6 years ago

Moving this out to Unknown milestone, as the non-dfa version is in and the complete version will need dataflow.

mavasani commented 6 years ago

The DFA version of this rule is merged in with https://github.com/dotnet/roslyn-analyzers/pull/1572. The pending items for this rule/analysis are tracked with separate issues (https://github.com/dotnet/roslyn-analyzers/issues/1547, https://github.com/dotnet/roslyn-analyzers/issues/1567).

roji commented 6 years ago

Note something similar implemented at the Entity Framework Core level: https://github.com/aspnet/EntityFrameworkCore/pull/11082#pullrequestreview-99901396. I think they flag any string concatenation involving a string, assuming it may be tainted with user data (i.e. no attempt to do flow analysis to know where data is coming from). It may be worth collaborating on this.