CVE-2020-15176: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: In GLPI before version 9.5.2, when supplying a back tick in input that gets put into a SQL query,the application does not escape or sanitize allowing for SQL Injection to occur. Leveraging this vulnerability an attacker is able to exfiltrate sensitive information like passwords, reset tokens, personal details, and more. The issue is patched in version 9.5.2
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
CVSS v3 base score: 8.6
Published: 2020-10-07T19:15Z
Repository: https://github.com/glpi-project/glpi
Language: PHP
Files changed: CHANGELOG.md, ajax/dropdownConnectNetworkPort.php, inc/crontask.class.php, inc/dbmysql.class.php, inc/dbmysqliterator.class.php, inc/report.class.php, tests/units/DB.php, tests/units/DBmysqlIterator.php
Commit date: 2020-10-06 13:51:37 +0200
Commit message: Merge pull request from GHSA-x93w-64x9-58qw

* Remove ability to use SQL expressions as string in criterion values

* Fix iterator syntax

Co-authored-by: Johan Cwiklinski <jcwiklinski@teclib.com>
Diff: diff --git a/CHANGELOG.md b/CHANGELOG.md
index fcfc3eb746..d4cd96b2e8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,11 +1,19 @@
 # GLPI changes
 
 The present file will list all changes made to the project; according to the
 [Keep a Changelog](http://keepachangelog.com/) project.
 
-## [9.5.1] unreleased
+## [9.5.2] unreleased
+
+### API changes
+
+#### Removed
+
+- Ability to use SQL expressions as string in criterion values in SQL iterator (replaced by usage of `QueryExpression`).
+
+## [9.5.1] 2020-07-16
 
 ## [9.5.0] 2020-07-07
 
 ### Added
 
diff --git a/ajax/dropdownConnectNetworkPort.php b/ajax/dropdownConnectNetworkPort.php
index 3e0813ca07..cde5c8d858 100644
--- a/ajax/dropdownConnectNetworkPort.php
+++ b/ajax/dropdownConnectNetworkPort.php
@@ -95,11 +95,11 @@ if (class_exists($_POST["itemtype"])
          'glpi_networkports_networkports' => [
             'ON'  => [
                'glpi_networkports_networkports' => 'networkports_id_1',
                'glpi_networkports'              => 'id', [
                   'OR'  => [
-                     'glpi_networkports_networkports.networkports_id_2' => $DB->quoteName('glpi_networkports.id')
+                     'glpi_networkports_networkports.networkports_id_2' => new QueryExpression($DB->quoteName('glpi_networkports.id'))
                   ]
                ]
             ]
          ]
       ] + $joins,
diff --git a/inc/crontask.class.php b/inc/crontask.class.php
index acf25e53e5..004321552a 100644
--- a/inc/crontask.class.php
+++ b/inc/crontask.class.php
@@ -394,16 +394,16 @@ class CronTask extends CommonDBTM{
          }
 
          // Build query for frequency and allowed hour
          $WHERE[] = ['OR' => [
             ['AND' => [
-               ['hourmin'   => ['<', $DB->quoteName('hourmax')]],
+               ['hourmin'   => ['<', new QueryExpression($DB->quoteName('hourmax'))]],
                'hourmin'   => ['<=', $hour],
                'hourmax'   => ['>', $hour]
             ]],
             ['AND' => [
-               'hourmin'   => ['>', $DB->quoteName('hourmax')],
+               'hourmin'   => ['>', new QueryExpression($DB->quoteName('hourmax'))],
                'OR'        => [
                   'hourmin'   => ['<=', $hour],
                   'hourmax'   => ['>', $hour]
                ]
             ]]
diff --git a/inc/dbmysql.class.php b/inc/dbmysql.class.php
index df2dd1dba6..5c0a68d2e5 100644
--- a/inc/dbmysql.class.php
+++ b/inc/dbmysql.class.php
@@ -1075,11 +1075,11 @@ class DBmysql {
       if ($value instanceof QueryParam || $value instanceof QueryExpression) {
          //no quote for query parameters nor expressions
          $value = $value->getValue();
       } else if ($value === null || $value === 'NULL' || $value === 'null') {
          $value = 'NULL';
-      } else if (!preg_match("/^`.*?`$/", $value)) { //`field` is valid only for mysql :/
+      } else {
          //phone numbers may start with '+' and will be considered as numeric
          $value = "'$value'";
       }
       return $value;
    }
diff --git a/inc/dbmysqliterator.class.php b/inc/dbmysqliterator.class.php
index e822a7aea0..c5fbec0bdd 100644
--- a/inc/dbmysqliterator.class.php
+++ b/inc/dbmysqliterator.class.php
@@ -587,12 +587,10 @@ class DBmysqlIterator implements Iterator, Countable {
    private function getCriterionValue($value) {
       if ($value instanceof \AbstractQuery) {
          return $value->getQuery();
       } else if ($value instanceof \QueryExpression) {
          return $value->getValue();
-      } else if (DBmysql::isNameQuoted($value)) { //FIXME: database related
-         return $value;
       } else if ($value instanceof \QueryParam) {
          return $value->getValue();
       } else {
          return $this->analyseCriterionValue($value);
       }
diff --git a/inc/report.class.php b/inc/report.class.php
index 0e26b5c0ac..72a621fb86 100644
--- a/inc/report.class.php
+++ b/inc/report.class.php
@@ -391,11 +391,11 @@ class Report extends CommonGLPI{
             'glpi_networkports_networkports AS LINK'  => [
                'ON'  =>[
                   'LINK'   => 'networkports_id_1',
                   'PORT_1' => 'id', [
                      'OR'     => [
-                        'LINK.networkports_id_2'   => $DB->quoteName('PORT_1.id')
+                        'LINK.networkports_id_2'   => new QueryExpression($DB->quoteName('PORT_1.id'))
                      ]
                   ]
                ]
             ],
             'glpi_networkports AS PORT_2' => [
diff --git a/tests/units/DB.php b/tests/units/DB.php
index c3e4f7a127..941d3b7307 100644
--- a/tests/units/DB.php
+++ b/tests/units/DB.php
@@ -94,11 +94,11 @@ class DB extends \GLPITestCase {
          ['42', "'42'"],
          ['+33', "'+33'"],
          [null, 'NULL'],
          ['null', 'NULL'],
          ['NULL', 'NULL'],
-         ['`field`', '`field`'],
+         [new \QueryExpression('`field`'), '`field`'],
          ['`field', "'`field'"]
       ];
    }
 
    /**
diff --git a/tests/units/DBmysqlIterator.php b/tests/units/DBmysqlIterator.php
index 618897571a..63431fded5 100644
--- a/tests/units/DBmysqlIterator.php
+++ b/tests/units/DBmysqlIterator.php
@@ -560,11 +560,11 @@ class DBmysqlIterator extends DbTestCase {
       $this->string($it->getSql())->isIdenticalTo("SELECT * FROM `foo` WHERE `bar` IN ('a', 'b', 'c')");
 
       $it = $this->it->execute('foo', ['bar' => 'val']);
       $this->string($it->getSql())->isIdenticalTo("SELECT * FROM `foo` WHERE `bar` = 'val'");
 
-      $it = $this->it->execute('foo', ['bar' => '`field`']);
+      $it = $this->it->execute('foo', ['bar' => new \QueryExpression('`field`')]);
       $this->string($it->getSql())->isIdenticalTo('SELECT * FROM `foo` WHERE `bar` = `field`');
 
       $it = $this->it->execute('foo', ['bar' => '?']);
       $this->string($it->getSql())->isIdenticalTo('SELECT * FROM `foo` WHERE `bar` = \'?\'');
 

Vulnerable code: ## [9.5.1] unreleased
                     'glpi_networkports_networkports.networkports_id_2' => $DB->quoteName('glpi_networkports.id')
               ['hourmin'   => ['<', $DB->quoteName('hourmax')]],
               'hourmin'   => ['>', $DB->quoteName('hourmax')],
      } else if (!preg_match("/^`.*?`$/", $value)) { //`field` is valid only for mysql :/
      } else if (DBmysql::isNameQuoted($value)) { //FIXME: database related
         return $value;
                        'LINK.networkports_id_2'   => $DB->quoteName('PORT_1.id')
         ['`field`', '`field`'],
      $it = $this->it->execute('foo', ['bar' => '`field`']);
Fixed code: ## [9.5.2] unreleased

### API changes

#### Removed

- Ability to use SQL expressions as string in criterion values in SQL iterator (replaced by usage of `QueryExpression`).

## [9.5.1] 2020-07-16
                     'glpi_networkports_networkports.networkports_id_2' => new QueryExpression($DB->quoteName('glpi_networkports.id'))
               ['hourmin'   => ['<', new QueryExpression($DB->quoteName('hourmax'))]],
               'hourmin'   => ['>', new QueryExpression($DB->quoteName('hourmax'))],
      } else {
                        'LINK.networkports_id_2'   => new QueryExpression($DB->quoteName('PORT_1.id'))
         [new \QueryExpression('`field`'), '`field`'],
      $it = $this->it->execute('foo', ['bar' => new \QueryExpression('`field`')]);