Review data adapters vulnerable to SQL injection

Rule ND3108 Security

Matches methods creating a DB adapter (like SqlDataAdapter) that call Fill() without using DB command parameters: prone to SQL Injection https://en.wikipedia.org/wiki/SQL_injection since malicious SQL might be injected in string parameter values.

False positives are possible; review matches and suppress issues when needed. To limit them, the rule also checks command parameters access from sub method calls (a solid indication of non-vulnerability).

Flagged Code
public DataTable LoadOrders(SqlConnection connection, string customerId) {
   var sql = "SELECT * FROM Orders WHERE CustomerId = '" + customerId + "'";
   using var adapter = new SqlDataAdapter(sql, connection);  // flagged: no command parameter
   var table = new DataTable();
   adapter.Fill(table);
   return table;
}
Fixed Code
public DataTable LoadOrders(SqlConnection connection, string customerId) {
   const string sql = "SELECT * FROM Orders WHERE CustomerId = @customerId";
   using var adapter = new SqlDataAdapter(sql, connection);
   adapter.SelectCommand.Parameters.AddWithValue("@customerId", customerId);  // parameterized
   var table = new DataTable();
   adapter.Fill(table);
   return table;
}

How to fix

If the method indeed injects parameter values in the SQL string, use adapter.SelectCommand.Parameters.Add(...) instead (or adapter.UpdateCommand / adapter.InsertCommand, depending on context). More on explicit parameters: https://stackoverflow.com/questions/4892166/how-does-sqlparameter-prevent-sql-injection

CQLinq source

This rule is written in CQLinq, C# LINQ over the NDepend code model. It ships with every new NDepend project: you can read it, adapt it, or use it as a starting point for your own custom rules.

// <Name>Review data adapters vulnerable to SQL injection</Name>
// <Id>ND3108:ReviewDataAdaptersVulnerableToSQLInjection</Id>

warnif count > 0

let adapters = ThirdParty.Types.WithFullNameIn(
  "System.Data.Common.DataAdapter",
  "System.Data.Common.DbDataAdapter",
  "Microsoft.Data.SqlClient.SqlDataAdapter",
  "System.Data.SqlClient.SqlDataAdapter",
  "System.Data.OleDb.OleDbDataAdapter",
  "System.Data.Odbc.OdbcDataAdapter",
  "System.Data.OracleClient.OracleDataAdapter")

where adapters.Any()
let adapterCtors = adapters.ChildMethods().Where(m => m.IsConstructor).ToHashSetEx()
let adapterFill = adapters.ChildMethods().Where(m => m.SimpleName.Contains("Fill")).ToHashSetEx()


let commands = ThirdParty.Types.WithFullNameIn(
  "System.Data.Common.DbCommand",
  "Microsoft.Data.SqlClient.SqlCommand",
  "System.Data.SqlClient.SqlCommand",
  "System.Data.OleDb.OleDbCommand",
  "System.Data.Odbc.OdbcCommand",
  "System.Data.OracleClient.OracleCommand")
where commands.Any()
let commandParameters = commands.ChildMethods().Where(m => m.IsPropertyGetter && m.SimpleNameEquals("get_Parameters")).ToHashSetEx()


from m in Application.Methods.UsingAny(adapterCtors)
                             .UsingAny(adapterFill)
where !m.MethodsCalled.Intersect(commandParameters).Any() &&
      // Check also that non of the method call rely on command parameters to get less false positives: 
      !m.MethodsCalled.SelectMany(mc => mc.MethodsCalled).Intersect(commandParameters).Any()

select new { m, 
   createA = m.MethodsCalled.Intersect(adapterCtors).First().ParentType,
   calls = m.MethodsCalled.Intersect(adapterFill).First(),
   Debt = 15.ToMinutes().ToDebt(),
   Severity = Severity.High
}

//<Expl>
// The *{0}* creates an instance of *{1}* and calls the *{2}*.
// This is susceptible to *SQL Injection* as malicious SQL code 
// can be injected into string parameter values. 
// Instead, use *adapter.SelectCommand.Parameters.Add(...)*.
//</Expl>

// <Desc>
// Matches methods creating a DB adapter (like *SqlDataAdapter*) that call
// *Fill()* without using DB command parameters: prone to **SQL Injection**
// https://en.wikipedia.org/wiki/SQL_injection since malicious SQL might be
// injected in string parameter values.
//
// False positives are possible; review matches and suppress issues when
// needed. To limit them, the rule also checks command parameters access
// from sub method calls (a solid indication of non-vulnerability).
//
// **Flagged Code**
// ```csharp
// public DataTable LoadOrders(SqlConnection connection, string customerId) {
//    var sql = "SELECT * FROM Orders WHERE CustomerId = '" + customerId + "'";
//    using var adapter = new SqlDataAdapter(sql, connection);  // flagged: no command parameter
//    var table = new DataTable();
//    adapter.Fill(table);
//    return table;
// }
// ```
//
// **Fixed Code**
// ```csharp
// public DataTable LoadOrders(SqlConnection connection, string customerId) {
//    const string sql = "SELECT * FROM Orders WHERE CustomerId = @customerId";
//    using var adapter = new SqlDataAdapter(sql, connection);
//    adapter.SelectCommand.Parameters.AddWithValue("@customerId", customerId);  // parameterized
//    var table = new DataTable();
//    adapter.Fill(table);
//    return table;
// }
// ```
// </Desc>

// <HowToFix>
// If the method indeed injects parameter values in the SQL string, use
// **adapter.SelectCommand.Parameters.Add(...)** instead (or
// *adapter.UpdateCommand* / *adapter.InsertCommand*, depending on context).
// More on explicit parameters:
// https://stackoverflow.com/questions/4892166/how-does-sqlparameter-prevent-sql-injection
// </HowToFix>

Open in the interactive Rules Explorer · All default rules

Security: all 39 items