CVE-2020-15108: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: In glpi before 9.5.1, there is a SQL injection for all usages of "Clone" feature. This has been fixed in 9.5.1.
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: 7.1
Published: 2020-07-17T21:15Z
Repository: https://github.com/glpi-project/glpi
Language: PHP
Files changed: inc/calendar.class.php, inc/commondbtm.class.php, tests/functionnal/Calendar.php, tests/functionnal/Computer.php
Commit date: 2020-07-16 08:42:46 +0200
Commit message: Merge pull request from GHSA-qv6w-68gq-wx2v

CVE-2020-15108

Add test to reproduce error
Add test on calendar duplication
Diff: diff --git a/inc/calendar.class.php b/inc/calendar.class.php
index b0c231a334..5f30629b86 100644
--- a/inc/calendar.class.php
+++ b/inc/calendar.class.php
@@ -199,11 +199,11 @@ class Calendar extends CommonDropdown {
     * @param $options array of new values to set
     * @return boolean True on success
     */
    function duplicate($options = []) {
 
-      $input = $this->fields;
+      $input = Toolbox::addslashes_deep($this->fields);
       unset($input['id']);
 
       if (is_array($options) && count($options)) {
          foreach ($options as $key => $val) {
             if (isset($this->fields[$key])) {
diff --git a/inc/commondbtm.class.php b/inc/commondbtm.class.php
index f872674ec9..114d49fa76 100644
--- a/inc/commondbtm.class.php
+++ b/inc/commondbtm.class.php
@@ -1204,11 +1204,11 @@ class CommonDBTM extends CommonGLPI {
 
       if ($DB->isSlave()) {
          return false;
       }
       $new_item = new static();
-      $input = $this->fields;
+      $input = Toolbox::addslashes_deep($this->fields);
       foreach ($override_input as $key => $value) {
          $input[$key] = $value;
       }
       $input = $new_item->prepareInputForClone($input);
       if (isset($input['id'])) {
diff --git a/tests/functionnal/Calendar.php b/tests/functionnal/Calendar.php
index 6cd8cdcf92..3350e65724 100644
--- a/tests/functionnal/Calendar.php
+++ b/tests/functionnal/Calendar.php
@@ -252,7 +252,21 @@ class Calendar extends DbTestCase {
       $id = $calendar->clone();
       $this->integer($id)->isGreaterThan($default_id);
       $this->boolean($calendar->getFromDB($id))->isTrue();
       //should have been duplicated too.
       $this->checkXmas($calendar);
+
+      //change name, and clone again
+      $this->boolean($calendar->update(['id' => $id, 'name' => "Je s\'apelle Groot"]))->isTrue();
+
+      $calendar = new \Calendar();
+      $this->boolean($calendar->getFromDB($id))->isTrue();
+
+      $this->boolean($calendar->duplicate())->isTrue();
+      $other_id = $calendar->fields['id'];
+      $this->integer($other_id)->isGreaterThan($id);
+      $this->boolean($calendar->getFromDB($other_id))->isTrue();
+      //should have been duplicated too.
+      $this->checkXmas($calendar);
+
    }
 }
diff --git a/tests/functionnal/Computer.php b/tests/functionnal/Computer.php
index a909e6717c..098221a386 100644
--- a/tests/functionnal/Computer.php
+++ b/tests/functionnal/Computer.php
@@ -36,29 +36,35 @@ use DbTestCase;
 
 /* Test for inc/computer.class.php */
 
 class Computer extends DbTestCase {
 
+   protected function getUniqueString() {
+      $string = parent::getUniqueString();
+      $string .= "with a ' inside!";
+      return $string;
+   }
+
    private function getNewComputer() {
       $computer = getItemByTypeName('Computer', '_test_pc01');
       $fields   = $computer->fields;
       unset($fields['id']);
       unset($fields['date_creation']);
       unset($fields['date_mod']);
       $fields['name'] = $this->getUniqueString();
-      $this->integer((int)$computer->add($fields))->isGreaterThan(0);
+      $this->integer((int)$computer->add(\Toolbox::addslashes_deep($fields)))->isGreaterThan(0);
       return $computer;
    }
 
    private function getNewPrinter() {
       $printer  = getItemByTypeName('Printer', '_test_printer_all');
       $pfields  = $printer->fields;
       unset($pfields['id']);
       unset($pfields['date_creation']);
       unset($pfields['date_mod']);
       $pfields['name'] = $this->getUniqueString();
-      $this->integer((int)$printer->add($pfields))->isGreaterThan(0);
+      $this->integer((int)$printer->add(\Toolbox::addslashes_deep($pfields)))->isGreaterThan(0);
       return $printer;
    }
 
    public function testUpdate() {
       global $CFG_GLPI;
@@ -87,11 +93,11 @@ class Computer extends DbTestCase {
              'users_id'     => $this->getUniqueInteger(),
              'groups_id'    => $this->getUniqueInteger(),
              'states_id'    => $this->getUniqueInteger(),
              'locations_id' => $this->getUniqueInteger(),
       ];
-      $this->boolean($computer->update($in))->isTrue();
+      $this->boolean($computer->update(\Toolbox::addslashes_deep($in)))->isTrue();
       $this->boolean($computer->getFromDB($computer->getID()))->isTrue();
       $this->boolean($printer->getFromDB($printer->getID()))->isTrue();
       unset($in['id']);
       foreach ($in as $k => $v) {
          // Check the computer new values
@@ -132,11 +138,11 @@ class Computer extends DbTestCase {
              'users_id'     => $this->getUniqueInteger(),
              'groups_id'    => $this->getUniqueInteger(),
              'states_id'    => $this->getUniqueInteger(),
              'locations_id' => $this->getUniqueInteger(),
       ];
-      $this->boolean($computer->update($in2))->isTrue();
+      $this->boolean($computer->update(\Toolbox::addslashes_deep($in2)))->isTrue();
       $this->boolean($computer->getFromDB($computer->getID()))->isTrue();
       $this->boolean($printer->getFromDB($printer->getID()))->isTrue();
       unset($in2['id']);
       foreach ($in2 as $k => $v) {
          // Check the computer new values
@@ -253,11 +259,11 @@ class Computer extends DbTestCase {
              'users_id'     => $this->getUniqueInteger(),
              'groups_id'    => $this->getUniqueInteger(),
              'states_id'    => $this->getUniqueInteger(),
              'locations_id' => $this->getUniqueInteger(),
       ];
-      $this->boolean($computer->update($in))->isTrue();
+      $this->boolean($computer->update(\Toolbox::addslashes_deep($in)))->isTrue();
       $this->boolean($computer->getFromDB($computer->getID()))->isTrue();
 
       $printer = new \Printer();
       $pid = $printer->add(
          [
@@ -429,10 +435,12 @@ class Computer extends DbTestCase {
             'items_id'     => $id
          ])
       )->isGreaterThan(0);
 
       //clone!
+      $computer = new \Computer(); //$computer->fields contents is already escaped!
+      $this->boolean($computer->getFromDB($id))->isTrue();
       $added = $computer->clone();
       $this->integer((int)$added)->isGreaterThan(0);
       $this->integer($added)->isNotEqualTo($computer->fields['id']);
 
       $clonedComputer = new \Computer();

Vulnerable code:       $input = $this->fields;
      $input = $this->fields;
      $this->integer((int)$computer->add($fields))->isGreaterThan(0);
      $this->integer((int)$printer->add($pfields))->isGreaterThan(0);
      $this->boolean($computer->update($in))->isTrue();
      $this->boolean($computer->update($in2))->isTrue();
      $this->boolean($computer->update($in))->isTrue();
Fixed code:       $input = Toolbox::addslashes_deep($this->fields);
      $input = Toolbox::addslashes_deep($this->fields);

      //change name, and clone again
      $this->boolean($calendar->update(['id' => $id, 'name' => "Je s\'apelle Groot"]))->isTrue();

      $calendar = new \Calendar();
      $this->boolean($calendar->getFromDB($id))->isTrue();

      $this->boolean($calendar->duplicate())->isTrue();
      $other_id = $calendar->fields['id'];
      $this->integer($other_id)->isGreaterThan($id);
      $this->boolean($calendar->getFromDB($other_id))->isTrue();
      //should have been duplicated too.
      $this->checkXmas($calendar);

   protected function getUniqueString() {
      $string = parent::getUniqueString();
      $string .= "with a ' inside!";
      return $string;
   }

      $this->integer((int)$computer->add(\Toolbox::addslashes_deep($fields)))->isGreaterThan(0);
      $this->integer((int)$printer->add(\Toolbox::addslashes_deep($pfields)))->isGreaterThan(0);
      $this->boolean($computer->update(\Toolbox::addslashes_deep($in)))->isTrue();
      $this->boolean($computer->update(\Toolbox::addslashes_deep($in2)))->isTrue();
      $this->boolean($computer->update(\Toolbox::addslashes_deep($in)))->isTrue();
      $computer = new \Computer(); //$computer->fields contents is already escaped!
      $this->boolean($computer->getFromDB($id))->isTrue();