SQL Injection prevention for concatenated string inside IN clause

Question

I have a variable which is an array of string. I want to pass all the values of the variable, concatenating all elements of it into a single string.

But I'm not sure whether this poses risks of SQL injection. My code:

private string concatenateStrings(string[] sa)
{
    StringBuilder sb = new StringBuilder();

    foreach (string s in sa)
    {
        if (sb.Length > 0)
        {
            sb.Append(",");
        }
        sb.Append("'");
        sb.Append(s);
        sb.Append("'");
    }
    return sb.ToString();
}

public void UpdateClaimSts(string[] ids)
{
    string query = @"UPDATE MYTABLE
                    SET STATUS = 'X'
                    WHERE TABLEID in (" + concatenateStrings(ids) + ")";

    OracleCommand dbCommand = (OracleCommand)this.Database.GetSqlStringCommand(query) as OracleCommand;
    this.Database.ExecuteNonQuery(dbCommand, this.Transaction);
}

I tried changing the query to use parameterized queries:

string query = @"UPDATE MYTABLE
                SET STATUS = 'X'
                WHERE TABLEID in (:ids)";

OracleCommand dbCommand = (OracleCommand)this.Database.GetSqlStringCommand(query) as OracleCommand;

dbCommand.Parameters.Add(":ids", OracleType.VarChar).Value = concatenateStrings(ids);
this.Database.ExecuteNonQuery(dbCommand, this.Transaction);

But it does not work. Any ideas?

Answer 1

Create a PL/SQL procedure (inside a PL/SQL Package) like this:

TYPE TArrayOfVarchar2 IS TABLE OF MYTABLE.TABLEID%TYPE INDEX BY PLS_INTEGER;

PROCEDURE UPDATE_MYTABLE(TABLEIDs IN TArrayOfVarchar2) IS
BEGIN

    FORALL i IN INDICES OF TABLEIDs
    UPDATE MYTABLE SET STATUS = 'X' WHERE TABLEID = TABLEIDs(i);

END;

and make a call like this:

using (OracleCommand cmd = new OracleCommand("BEGIN UPDATE_MYTABLE(:tableId); END;"), con))
{
  cmd.CommandType = CommandType.Text;
  // or
  // OracleCommand cmd = new OracleCommand("UPDATE_MYTABLE"), con);
  // cmd.CommandType = CommandType.StoredProcedure;
  var par = cmd.Parameters.Add("tableId", OracleDbType.Varchar2, ParameterDirection.Input);
  par.CollectionType = OracleCollectionType.PLSQLAssociativeArray;
  par.Value = sa;
  par.Size = sa.Length;

  cmd.ExecuteNonQuery();
}

Answer 2

As a quick and partial (we assume TABLEID field be of type NUMBER) solution you can verify that each item in sa is a valid integer:

private string concatenateStrings(string[] sa) {
   return string.Join(", ", sa
     .Where(item => Regex.IsMatch(item, @"^\-?[0-9]+$"))); 
} 

public void UpdateClaimSts(string[] ids) {
  string query = string.Format(
    @"UPDATE MYTABLE
         SET STATUS = 'X'
       WHERE TABLEID IN ({0})", concatenateStrings(ids));
      ...

In general case, you can try using bind variables (please, notice plural: we have to create many of them):

public void UpdateClaimSts(string[] ids) {  
  // :id_0, :id_1, ..., :id_N   
  string bindVariables = string.Join(", ", ids
    .Select((id, index) => ":id_" + index.ToString()));

  string query = string.Format(
    @"UPDATE MYTABLE
         SET STATUS = 'X'
       WHERE TABLEID IN ({0})", bindVariables);

  // Do not forget to wrap IDisposable into "using"
  using (OracleCommand dbCommand = ...) {
    ...
    // Each item of the ids should be assigned to its bind variable
    for (int i = 0; i < ids.Length; ++i)
      dbCommand.Parameters.Add(":id_" + i.ToString(), OracleType.VarChar).Value = ids[i];

   ...

Answer 3

C# has a OracleCollectionType.PLSQLAssociativeArray type for passing arrays to a PL/SQL Associative Array data type but this cannot be used in SQL queries as it is only a PL/SQL data structure.

It, unfortunately, does not support passing an array to an SQL Collection data type (which could be used in an SQL query).

One work around for this is to ask your DBA to create a simple function to convert a PL/SQL Associative Array to an SQL collection and then use this as an intermediate step in the query:

CREATE TYPE varchar2s_array_type IS TABLE OF VARCHAR2(100)
/

CREATE PACKAGE utils IS
  TYPE varchar2s_assoc_array_type IS TABLE OF VARCHAR2(100) INDEX BY PLS_INTEGER;

  FUNCTION assoc_array_to_collection(
    p_assoc_array IN varchar2s_assoc_array_type
  ) RETURN varchar2s_array_type DETERMINISTIC;
END;
/

CREATE PACKAGE BODY utils IS
  FUNCTION assoc_array_to_collection(
    p_assoc_array IN varchar2s_assoc_array_type
  ) RETURN varchar2s_array_type DETERMINISTIC
  IS
    p_array varchar2s_array_type := varchar2s_array_type();
    i PLS_INTEGER;
  BEGIN
    IF p_assoc_array IS NOT NULL THEN
      i := p_assoc_array.FIRST;
      LOOP
        EXIT WHEN i IS NULL;
        p_array.EXTEND();
        p_array(p_array.COUNT) := p_assoc_array(i);
        i := p_assoc_array.NEXT(i);
      END LOOP;
    END IF;
    RETURN p_array;
  END;
END;
/

Then you can change your code to use MEMBER OF rather than IN within the SQL statement:

UPDATE MYTABLE
SET    STATUS = 'X'
WHERE  TABLEID MEMBER OF utils.assoc_array_to_collection(:ids)

And bind the parameter using something like (I'm not a C# user so this is just to give you a general idea of the method, even if the syntax is not entirely correct):

var par = cmd.Parameters.Add(":ids", OracleDbType.Varchar2, ParameterDirection.Input);
par.CollectionType = OracleCollectionType.PLSQLAssociativeArray;
par.Value = ids;
par.Size = ids.Length;
cmd.ExecuteQuery();

Then you can reuse the generic function across many queries.