CODEWITHSUNDEEP
sqloracle-databasesql-injection
aaston13
aaston13

Reputation: 33

Does this check avoid SQL injections?

I need to pass a where condition from my client to the webservice, which performs the SQL query on an oracle database.

 const res = await db.execute(`SELECT count(ID) FROM table WHERE ${where}`);

Since the where condition can be totally dynamic, I cannot use prepared statements, a stored procedure or parameters.

Is it sufficient, to just run the where condition through the following check?

static isDangerousSql(sql: string): boolean {
    const characters = [ '--', ';'];

    for (const char of characters) {
        if (sql.indexOf(char) > -1) {
            return true;
        }
    }

    return false;
}

Upvotes: 2

Views: 93

Answers (1)

MT0
MT0

Reputation: 167972

No, it is not sufficient.

If ${where} is:

EXISTS (
  SELECT 1
  FROM   security_table
  WHERE  userid        = 1234
  AND    password_hash = CHR(65) || CHR(66) || CHR(67) || CHR(68)
)

Then it will:

  1. Pass your check;
  2. Confirm that there is a table named security_table;
  3. Confirm that the table has the columns userid and password_hash; and
  4. Confirm that there is a user with the id 1234 and the password hash ABCD

This can be used to map your entire database structure and to check for the existence of any data. Definitely a vulnerability.

An example would be 

WHERE status = 'Active' and ID in (SELECT cell_id FROM alerts WHERE alert_status = 1)

Rather than use dynamic SQL, you can list the valid filter conditions in your query and use bind parameters to populate the filter conditions:

So a slightly more complicated example would be:

SELECT count(ID)
FROM   table t
WHERE  ( :status IS NULL OR status = :status )
AND    (  ( :alert_status IS NULL AND :other_status IS NULL )
       OR EXISTS (
            SELECT 1
            FROM   alerts a
            WHERE  t.id = a.cell_id
            AND    ( :alert_status IS NULL OR alert_status = :alert_status )
            AND    ( :other_status IS NULL OR other_status = :other_status )
       ) )

Then you have a static query and can pass in the (named) bind variables :status, :alert_status and :other_status.

  • Your users do not need to know the underlying table structures.
  • The code is not vulnerable to SQL injection.
  • You can control what data they can access.
  • The database can cache the static query.

If you do not want to have a single static query with all the parameters then you can build the filters from fixed snippet component parts in the middle tier.

Some pseudo-code:

sql             = "SELECT count(ID) FROM table";
filters         = [];
bind_parameters = [];
if ( [ 'Active', 'Inactive' ].indexOf( user_input.status ) > -1 )
{
  filters.push( "status = ?" )
  bind_parameters.push( user_input.status );
}
if ( user_input.active_status == 0 || user_input.active_status == 1 )
{
  filters.push( "id IN ( SELECT cell_id FROM alerts WHERE alert_status = ?)" );
  bind_parameters.push( user_input.active_status );
}
if ( filters.length > 0 )
{
  sql = sql + " WHERE " + filters.join( " AND " );
}
db.setSQL( sql );
for ( i = 0; i < bind_parameters.length; i++ )
{
  db.setBindParameter( i, bind_parameters[i] );
}
const res = await db.executeQuery();

(The above pseudo-code is to indicate the concept of building the query dynamically in the middle tier from fixed snippets and does not consider implementation details such as setting the data type of the bind parameters and probably many other things - use the idea with caution and validate your implementation's security before you put it into production.)

Upvotes: 5

Related Questions