adam/Piwigo
1
0 Fork 0
mirror of https://github.com/Piwigo/Piwigo.git synced 2026-10-30 21:52:00 +01:00
Code Issues 657 Packages Projects Releases Wiki Activity

Turn SQL statements into prepared statements to protect against SQL-Injection #1750

New Issue
Open
opened 2026-10-05 00:55:29 +02:00 by adam · 4 comments
adam commented 2026-10-05 00:55:29 +02:00
Owner
Copy link
Copy source

My initiative was triggered by this issue: https://github.com/Piwigo/piwigo-openstreetmap/issues/163

SQL-Injections are serious security threats, in Security and coding guideline it is mentioned at first place. However, many developers are lazy and it is fully up to them whether they use pwg_db_real_escape_string() or not. I think they should be nudged a bit stronger to write secure code, which means they should use prepared statements. Personally, I see real_escape_string() just as a workaround - not as a solution!

My proposal for https://github.com/Piwigo/Piwigo/blob/master/include/dblayer/functions_mysqli.inc.php would be this one:

/**
 * Execute a query
 *
 * @param string $query The SQL query/command to be executed. 
 * @param array<mixed> $values Array of parameters values. Use an emtpy array [] if the query does not have any parameter.
 * @return mysqli_result|bool
 */
function pwg_query($query, $values = NULL)
{
  global $mysqli, $conf, $page, $debug, $t2;

  if (is_null($values) or strpos($query, "'") !== false)
  {
    trigger_error('
  The query is vulnerable to SQL-Injection, it is highly recommended to use prepared statements.
  If the query does not contain any parameters, pass an emtpy array [] to get rid of this warning', E_USER_WARNING);
  }

  $start = microtime(true);
  if (is_array($values) and count($values) > 0)
  {
    $stmt = $mysqli->prepare($query);
    $types = '';
    foreach ($values as $val)
    {
      $type = gettype($val);
      if ($type == 'integer')
      {
        $types .= 'i';
      }
      elseif ($type == 'double')
      {
        $types .= 'd';
      }
      elseif ($type == 'string')
      {
        $types .= 's';
      }
      else
      {
        my_error("Data type '$type' not supported as bind parameter", $conf['die_on_sql_error']);
      }
    }
    $stmt->bind_param($types, ...$values);
    ($result = $stmt->execute()) or my_error($query, $conf['die_on_sql_error']);
    $result = $stmt->get_result();
  }
  else
  {
    ($result = $mysqli->query($query)) or my_error($query, $conf['die_on_sql_error']);
  }
  $time = microtime(true) - $start;

  if (!isset($page['count_queries']))
  {
    $page['count_queries'] = 0;
    $page['queries_time'] = 0;
  }

  $page['count_queries']++;
  $page['queries_time'] += $time;

  if ($conf['show_queries'])
  {
    $output = '';
    $output .= '<pre>[' . $page['count_queries'] . '] ';
    $output .= "\n" . $query;
    if (is_array($values) and count($values) > 0)
    {
      $parameter_query = $query;
      for ($i = 0; $i < count($values); $i++)
      {
        $parameter_query = substr_replace($parameter_query, ":p$i", strpos($parameter_query, '?'), 1);
      }
      $parameter_value = [];
      foreach (pwg_bind_values($values) as $val)
      {
        array_push($parameter_value, $val);
      }
      $output .= "\n" . $parameter_query;
      $output .= "\n" . implode(', ', $parameter_value);
    }
    $output .= "\n" . '(this query time : ';
    $output .= '<b>' . number_format($time, 3, '.', ' ') . ' s)</b>';
    $output .= "\n" . '(total SQL time  : ';
    $output .= number_format($page['queries_time'], 3, '.', ' ') . ' s)';
    $output .= "\n" . '(total time      : ';
    $output .= number_format(($time + $start - $t2), 3, '.', ' ') . ' s)';
    if ($result != null and preg_match('/\s*SELECT\s+/i', $query))
    {
      $output .= "\n" . '(num rows        : ';
      $output .= pwg_db_num_rows($result) . ' )';
    }
    elseif ($result != null
      and preg_match('/\s*INSERT|UPDATE|REPLACE|DELETE\s+/i', $query))
    {
      $output .= "\n" . '(affected rows   : ';
      $output .= pwg_db_changes() . ' )';
    }
    $output .= "</pre>\n";

    $debug .= $output;
  }

  return $result;
}

/**
 * Generates a SQL parameter sequence array, like [ ':p0 => foo', ':p1 => bar', ':p2 => 3' ]
 * @param array<mixed> $values Array of parameters values. 
 * @return array<string>
 */
function pwg_bind_values($values)
{
  for ($i = 0; $i < count($values); $i++)
  {
    yield "p$i => $values[$i]";
  }
}

Then SQL query would look like this:

$query = '
 SELECT id
 FROM '.TAGS_TABLE.'
 WHERE name = ?';

pwg_query($query, [$tag_name] );

The developer will see a warning, if they use vulnerable SQL strings where parameter values are composed as string in the query. Of course, all existing code will remain working, it raises just a (well visible) warning.

I know, it will be a lot of work to transform 722 function calls of pwg_query but in the end there is no alternative to get more secure code. Parameterized statements are much easier to use, than checking user input with regular expressions, dealing with quotes, etc. As a positive side-effect, the performance will be better than hard-coded statements.

What is your opinion about this proposal?

Wernfried

My initiative was triggered by this issue: https://github.com/Piwigo/piwigo-openstreetmap/issues/163 SQL-Injections are serious security threats, in [Security and coding](https://github.com/Piwigo/Piwigo/wiki/Security-and-coding) guideline it is mentioned at first place. However, many developers are lazy and it is fully up to them whether they use `pwg_db_real_escape_string()` or not. I think they should be nudged a bit stronger to write secure code, which means they should use prepared statements. Personally, I see `real_escape_string()` just as a workaround - not as a solution! My proposal for https://github.com/Piwigo/Piwigo/blob/master/include/dblayer/functions_mysqli.inc.php would be this one: ``` /** * Execute a query * * @param string $query The SQL query/command to be executed. * @param array<mixed> $values Array of parameters values. Use an emtpy array [] if the query does not have any parameter. * @return mysqli_result|bool */ function pwg_query($query, $values = NULL) { global $mysqli, $conf, $page, $debug, $t2; if (is_null($values) or strpos($query, "'") !== false) { trigger_error(' The query is vulnerable to SQL-Injection, it is highly recommended to use prepared statements. If the query does not contain any parameters, pass an emtpy array [] to get rid of this warning', E_USER_WARNING); } $start = microtime(true); if (is_array($values) and count($values) > 0) { $stmt = $mysqli->prepare($query); $types = ''; foreach ($values as $val) { $type = gettype($val); if ($type == 'integer') { $types .= 'i'; } elseif ($type == 'double') { $types .= 'd'; } elseif ($type == 'string') { $types .= 's'; } else { my_error("Data type '$type' not supported as bind parameter", $conf['die_on_sql_error']); } } $stmt->bind_param($types, ...$values); ($result = $stmt->execute()) or my_error($query, $conf['die_on_sql_error']); $result = $stmt->get_result(); } else { ($result = $mysqli->query($query)) or my_error($query, $conf['die_on_sql_error']); } $time = microtime(true) - $start; if (!isset($page['count_queries'])) { $page['count_queries'] = 0; $page['queries_time'] = 0; } $page['count_queries']++; $page['queries_time'] += $time; if ($conf['show_queries']) { $output = ''; $output .= '<pre>[' . $page['count_queries'] . '] '; $output .= "\n" . $query; if (is_array($values) and count($values) > 0) { $parameter_query = $query; for ($i = 0; $i < count($values); $i++) { $parameter_query = substr_replace($parameter_query, ":p$i", strpos($parameter_query, '?'), 1); } $parameter_value = []; foreach (pwg_bind_values($values) as $val) { array_push($parameter_value, $val); } $output .= "\n" . $parameter_query; $output .= "\n" . implode(', ', $parameter_value); } $output .= "\n" . '(this query time : '; $output .= '<b>' . number_format($time, 3, '.', ' ') . ' s)</b>'; $output .= "\n" . '(total SQL time : '; $output .= number_format($page['queries_time'], 3, '.', ' ') . ' s)'; $output .= "\n" . '(total time : '; $output .= number_format(($time + $start - $t2), 3, '.', ' ') . ' s)'; if ($result != null and preg_match('/\s*SELECT\s+/i', $query)) { $output .= "\n" . '(num rows : '; $output .= pwg_db_num_rows($result) . ' )'; } elseif ($result != null and preg_match('/\s*INSERT|UPDATE|REPLACE|DELETE\s+/i', $query)) { $output .= "\n" . '(affected rows : '; $output .= pwg_db_changes() . ' )'; } $output .= "</pre>\n"; $debug .= $output; } return $result; } /** * Generates a SQL parameter sequence array, like [ ':p0 => foo', ':p1 => bar', ':p2 => 3' ] * @param array<mixed> $values Array of parameters values. * @return array<string> */ function pwg_bind_values($values) { for ($i = 0; $i < count($values); $i++) { yield "p$i => $values[$i]"; } } ``` Then SQL query would look like this: ``` $query = ' SELECT id FROM '.TAGS_TABLE.' WHERE name = ?'; pwg_query($query, [$tag_name] ); ``` The developer will see a warning, if they use vulnerable SQL strings where parameter values are composed as string in the query. Of course, all existing code will remain working, it raises just a (well visible) warning. I know, it will be a lot of work to transform 722 function calls of `pwg_query` but in the end there is no alternative to get more secure code. Parameterized statements are much easier to use, than checking user input with regular expressions, dealing with quotes, etc. As a positive side-effect, the performance will be better than hard-coded statements. What is your opinion about this proposal? Wernfried <!-- github-sync-id:1932053218 -->
adam commented 2026-10-05 00:55:30 +02:00
Author
Owner
Copy link
Copy source

Very interesting. A deep impact and certainly a lot of code to change.

  1. If we implement this, I don't think we should immediately "warn". Instead I would work on 3 versions:
  • first major version would let developer use prepared statements and no warning for "old-style" queries + we add a $conf['warn_non_prepared_statement'] = false; by default
  • second major version with $conf['warn_non_prepared_statement'] = true; by default (so, possible to turn it off)
  • third major version, remove configuration setting and break on non-prepared statements

  1. how to deal with this kind of query pattern WHERE id IN ('.implode(',', $ids).') ?

  2. what I fear most is when an SQL query is built from several functions. For example what I've been working on lately is the search engine. In include/functions_search.inc.php you have SQL queries built upon several functions. If we need to gather all values appart from the statement, to provide them only when the statement is executed, it's going to be very complicated :-/

Very interesting. A deep impact and certainly a lot of code to change. 1) If we implement this, I don't think we should immediately "warn". Instead I would work on 3 versions: - first major version would let developer use prepared statements and no warning for "old-style" queries + we add a `$conf['warn_non_prepared_statement'] = false;` by default - second major version with `$conf['warn_non_prepared_statement'] = true;` by default (so, possible to turn it off) - third major version, remove configuration setting and break on non-prepared statements 2) how to deal with this kind of query pattern `WHERE id IN ('.implode(',', $ids).')` ? 3) what I fear most is when an SQL query is built from several functions. For example what I've been working on lately is the search engine. In `include/functions_search.inc.php` you have SQL queries built upon several functions. If we need to gather all values appart from the statement, to provide them only when the statement is executed, it's going to be very complicated :-/ <!-- github-comment-id:1759181256 -->
adam commented 2026-10-05 00:55:31 +02:00
Author
Owner
Copy link
Copy source

Sounds reasonable to implement the function in steps.
Would setting $conf['warn_non_prepared_statement'] be global? I.e. if a user or a developer of a plugin enable/disable it, then it would apply for entire application?

Regarding second question:

how to deal with this kind of query pattern WHERE id IN ('.implode(',', $ids).') ?

There are several tutorials available, e.g. Mysqli prepared statement with multiple values for IN clause

It would be for example

$ids = [1, 2, 3];
$query = 'SELECT * FROM table WHERE id IN (' . implode(',', array_fill(0, count($ids), '?')) . ')';
pwg_query($query, $ids);

I fully agree, for complex queries it can be a lot of work to transpose them into prepared statement.

Unfortunately (and unlike you can do in other languages) you can bind parameters only by position, not by name. This requires more attention when you compose the statement and parameter array.

Another variant to compose a query using associated array could be this one:

$where = [
   'id' => 1,
   'name' => 'foo',
   'active' => null
];
$values = array_filter(
   array_values($where),
   function (?string $v) { return !is_null($v); } );

$query = array_map(
   function (string $k, ?string $v) { return is_null($v) ? "$k IS NULL" : "$k = ?"; },
   array_keys($where),
   $values
);

$query = 'SELECT * FROM table WHERE ' . implode(' AND ', $query);
pwg_query($query, $values);
Sounds reasonable to implement the function in steps. Would setting `$conf['warn_non_prepared_statement']` be global? I.e. if a user or a developer of a plugin enable/disable it, then it would apply for entire application? Regarding second question: > how to deal with this kind of query pattern `WHERE id IN ('.implode(',', $ids).')` ? There are several tutorials available, e.g. [Mysqli prepared statement with multiple values for IN clause](https://phpdelusions.net/mysqli_examples/prepared_statement_with_in_clause) It would be for example ``` $ids = [1, 2, 3]; $query = 'SELECT * FROM table WHERE id IN (' . implode(',', array_fill(0, count($ids), '?')) . ')'; pwg_query($query, $ids); ``` I fully agree, for complex queries it can be a lot of work to transpose them into prepared statement. Unfortunately (and unlike you can do in other languages) you can bind parameters only by position, not by name. This requires more attention when you compose the statement and parameter array. Another variant to compose a query using associated array could be this one: ``` $where = [ 'id' => 1, 'name' => 'foo', 'active' => null ]; $values = array_filter( array_values($where), function (?string $v) { return !is_null($v); } ); $query = array_map( function (string $k, ?string $v) { return is_null($v) ? "$k IS NULL" : "$k = ?"; }, array_keys($where), $values ); $query = 'SELECT * FROM table WHERE ' . implode(' AND ', $query); pwg_query($query, $values); ``` <!-- github-comment-id:1759470138 -->
adam commented 2026-10-05 00:55:32 +02:00
Author
Owner
Copy link
Copy source

configuration settings are global.

The more I think about this "prepared statement" thing, the more I think "of course it's an improvement for security, but a huge mess to implement". And I really mean hundreds (or more) hours of coding to prevent a few potential SQL injections that we already have a procedure to avoid. Such a huge amount of changes on Piwigo codebase will obviously introduce bugs on perfectly working code. Is it worth the price? Saying it another way: would a developer contribute to Piwigo by implementing such a huge change, and take the risk to break many things? Saying it even another way: would the company pay a developer to work on it several months? (answer is clearly no, we have much higher priority tasks, more interesting for the developer and users)

On one side I would certainly use prepared statements if we were starting Piwigo code from scratch: the cost would be low and we won't take the risk to break existing working code. On the other side, it's not because we don't force all existing SQL queries to switch to prepared statements that we should not recommend to use them from now, and slowly use them on old SQL queries when reworking specific pieces of code.

Unfortunately (and unlike you can do in other languages) you can bind parameters
only by position, not by name.

Very unfortunate, indeed. If we had a solution to write:

$query = '
SELECT
    *
  FROM '.IMAGES_TABLE.'
  WHERE id = %id%
;';

pwg_query($query, ['id'=>18]);

it would be much more easy to adopt.

I like the idea to have this feature (with associate array) and recommend it for future code but I don't think it's a good idea to force it on existing code.

configuration settings are global. The more I think about this "prepared statement" thing, the more I think "of course it's an improvement for security, but a huge mess to implement". And I really mean hundreds (or more) hours of coding to prevent a few potential SQL injections that we already have a procedure to avoid. Such a huge amount of changes on Piwigo codebase will obviously introduce bugs on perfectly working code. Is it worth the price? Saying it another way: would a developer contribute to Piwigo by implementing such a huge change, and take the risk to break many things? Saying it even another way: would the company pay a developer to work on it several months? (answer is clearly no, we have much higher priority tasks, more interesting for the developer and users) On one side I would certainly use prepared statements if we were starting Piwigo code from scratch: the cost would be low and we won't take the risk to break existing working code. On the other side, it's not because we don't force all existing SQL queries to switch to prepared statements that we should not recommend to use them from now, and slowly use them on old SQL queries when reworking specific pieces of code. > Unfortunately (and unlike you can do in other languages) you can bind parameters > only by position, not by name. Very unfortunate, indeed. If we had a solution to write: ``` $query = ' SELECT * FROM '.IMAGES_TABLE.' WHERE id = %id% ;'; pwg_query($query, ['id'=>18]); ``` it would be much more easy to adopt. I like the idea to have this feature (with associate array) and recommend it for future code but I don't think it's a good idea to force it on existing code. <!-- github-comment-id:1761491930 -->
adam commented 2026-10-05 00:55:32 +02:00
Author
Owner
Copy link
Copy source

Yes, most likely you are right. Nevertheless it would be a good idea to add the optional parameter for bind values. Then developers can use them.

Instead of

$query = 'SELECT * FROM '.IMAGES_TABLE.' WHERE id = %id%';
pwg_query($query, ['id'=>18]);

I would propose one of these

$query = 'SELECT * FROM '.IMAGES_TABLE.' WHERE id = :id';
pwg_query($query, ['id'=>18]);

or

$query = 'SELECT * FROM '.IMAGES_TABLE.' WHERE id = @id';
pwg_query($query, ['id'=>18]);

which are more common syntax for parameters.

btw, PDO would support bind parameters by name, see https://www.php.net/manual/en/pdo.prepared-statements.php

Yes, most likely you are right. Nevertheless it would be a good idea to add the optional parameter for bind values. Then developers can use them. Instead of ``` $query = 'SELECT * FROM '.IMAGES_TABLE.' WHERE id = %id%'; pwg_query($query, ['id'=>18]); ``` I would propose one of these ``` $query = 'SELECT * FROM '.IMAGES_TABLE.' WHERE id = :id'; pwg_query($query, ['id'=>18]); ``` or ``` $query = 'SELECT * FROM '.IMAGES_TABLE.' WHERE id = @id'; pwg_query($query, ['id'=>18]); ``` which are more common syntax for parameters. btw, [PDO](https://www.php.net/manual/de/book.pdo.php) would support bind parameters by name, see https://www.php.net/manual/en/pdo.prepared-statements.php <!-- github-comment-id:1761952336 -->
Sign in to join this conversation.
No Branch/Tag Specified
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
adam (Adam Wojcieszonek)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: adam/Piwigo#1750