The object oriented database api in Drupal 7 and Drupal 8 has some features to reduce sql injection. There are some pieces of the API that do not protect against sql injection. A developer using the database api can pass user-supplied data to many parameters of the api and trust the api to protect against sql injection. However, there are many parameters that are not protected.
An example of some insecure code:
$query = db_select('node', 'n')
->fields('n', array('nid', 'title'));
$order = isset($_GET['order']) ? $_GET['order'] : 'DESC';
$order_field = isset($_GET['order_field']) ? $_GET['order_field'] : 'n.nid';
$query->orderBy($order_field, $order);
$results = $query->execute();
The above specific weakness would be fixed by #829464: orderby() should verify direction and escape fields.
Here is a list of some of the more frequently used methods and which parameters to those methods are are safe or dangerous:
// db_insert->fields(['dangerous', 'dangerous']);
// db_insert->fields([dangerous => safe])
// db_insert->values([safe => safe]); * when combined with safe ->fields
// i.e. the "degenerate" form provides security if you must put user
// supplied data into the keys of the values.
// db_update->fields([dangerous => safe])
// db_delete->condition(safe, safe, dangerous)
// db_select(safe, safe)
// ->fields(safe, safe)
// ->condition(safe, safe, dangerous)
// ->where(dangerous)
// ->addTag(safe)
// addField(safe, safe, safe)
// ->range(safe, safe)
// ->join(safe, safe, dangerous, safe array of args)
// ->innerJoin(safe, safe, dangerous, safe array of args)
// ->leftJoin(safe, safe, dangerous, safe array of args)
// ->rightJoin(safe, safe, dangerous, safe array of args)
// ->addJoin(dangerous, safe, safe, dangerous, safe array of parameters)
// ->addExpression(dangerous, safe)
// ->isNotNull(safe)
// ->havingCondition(safe, safe, dangerous);
// ->having(dangerous, safe parameter array)
// ->groupBy(dangerous);
// ->orderBy(dangerous, safe);
// ->addMetaData(safe, safe)
We should do some mix of:
Comments
Comment #2
gregglesComment #3
Crell commentedThanks, greggles!
I would suggest we start by adding documentation to those that we cannot safely escape, like where(); That method asks the developer to supply an arbitrary SQL fragment, so there's no way we can safely escape it. If it's based on user input the developer MUST deal with it themselves. Documenting those cases we know we can't handle otherwise is an easy first step.
Comment #19
quietone commentedThere has been work to fix this, such as #3191623: Select queries do not escape the GROUP BY fields and #2986452: Database reserved keywords need to be quoted as per the ANSI standard. closing as a duplicate of the later