CVE-2021-41154: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: Tuleap is a Free & Open Source Suite to improve management of software developments and collaboration. In affected versions an attacker with read access to a "SVN core" repository could execute arbitrary SQL queries. The following versions contain the fix: Tuleap Community Edition 11.17.99.144, Tuleap Enterprise Edition 11.17-5, Tuleap Enterprise Edition 11.16-7.
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.8
Published: 2021-10-18T22:15Z
Repository: https://github.com/Enalean/tuleap
Language: PHP
Files changed: src/common/Widget/Widget_MyLatestSvnCommits.class.php, src/common/Widget/Widget_ProjectLatestSvnCommits.class.php, src/common/svn/SVN_LogFactory.class.php, src/www/svn/browse_revision.php, src/www/svn/svn_utils.php, tests/psalm/tuleap-baseline.xml
Commit date: 2020-08-12 13:01:20 +0200
Commit message: request #16213: SQL injection in the "SVN core" commits browser

Also fixes a XSS via the same injection point.

Change-Id: Ib33199b7d0ec5f8a07a65d74766477f23df4a75b

Diff: diff --git a/src/common/Widget/Widget_MyLatestSvnCommits.class.php b/src/common/Widget/Widget_MyLatestSvnCommits.class.php
index d98308a620d..f5cb8586d48 100644
--- a/src/common/Widget/Widget_MyLatestSvnCommits.class.php
+++ b/src/common/Widget/Widget_MyLatestSvnCommits.class.php
@@ -1,8 +1,8 @@
 <?php
 /**
- * Copyright (c) Enalean, 2013 - 2018. All Rights Reserved.
+ * Copyright (c) Enalean, 2013 - Present. All Rights Reserved.
  * Copyright (c) Xerox Corporation, Codendi 2001-2009.
  *
  * This file is a part of Tuleap.
  *
  * Tuleap is free software; you can redistribute it and/or modify
@@ -78,11 +78,11 @@ class Widget_MyLatestSvnCommits extends Widget
                 list($hide_now,$count_diff,$hide_url) = my_hide_url('my_svn_group', $project_id, $request->get('hide_item_id'), count($project_ids), $request->get('hide_my_svn_group'), $request->get('dashboard_id'));
                 $html .= $hide_url;
 
                 $html .= '<strong>' . $hp->purify($project->getPublicName()) . '</strong>';
                 if (! $hide_now) {
-                    list($latest_revisions, $nb_revisions) = svn_get_revisions($project, 0, $this->_nb_svn_commits, '', $user->getUserName(), '', '', 0, false);
+                    list($latest_revisions, $nb_revisions) = svn_get_revisions($project, 0, $this->_nb_svn_commits, '', $user->getUserName(), '', [], 0, false);
                     $revision_total += $nb_revisions;
                     if (db_numrows($latest_revisions) > 0) {
                         $i = 0;
                         while ($data = db_fetch_array($latest_revisions)) {
                             $html .= '<div class="' . util_get_alt_row_color($i++) . '" style="border-bottom:1px solid #ddd">';
diff --git a/src/common/Widget/Widget_ProjectLatestSvnCommits.class.php b/src/common/Widget/Widget_ProjectLatestSvnCommits.class.php
index ee235508094..a28cd2fa2aa 100644
--- a/src/common/Widget/Widget_ProjectLatestSvnCommits.class.php
+++ b/src/common/Widget/Widget_ProjectLatestSvnCommits.class.php
@@ -59,11 +59,11 @@ class Widget_ProjectLatestSvnCommits extends Widget_ProjectLatestCommits
     {
         if (! $this->latest_revisions) {
             $pm = ProjectManager::instance();
             $project = $pm->getProject($this->group_id);
             if ($project && $this->canBeUsedByProject($project)) {
-                list($this->latest_revisions,) = svn_get_revisions($project, 0, 5, '', '', '', '', 0, false);
+                list($this->latest_revisions,) = svn_get_revisions($project, 0, 5, '', '', '', [], 0, false);
             }
         }
         return $this->latest_revisions;
     }
 }
diff --git a/src/common/svn/SVN_LogFactory.class.php b/src/common/svn/SVN_LogFactory.class.php
index fbe5d163ef7..ee7f185a4eb 100644
--- a/src/common/svn/SVN_LogFactory.class.php
+++ b/src/common/svn/SVN_LogFactory.class.php
@@ -1,8 +1,8 @@
 <?php
 /**
- * Copyright (c) Enalean, 2012-2018. All Rights Reserved.
+ * Copyright (c) Enalean, 2012-Present. All Rights Reserved.
  *
  * This file is a part of Tuleap.
  *
  * Tuleap is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -136,11 +136,11 @@ class SVN_LogFactory
             0,
             $limit,
             '',
             $author->getUserName(),
             '',
-            '',
+            [],
             0,
             false
         );
     }
 
diff --git a/src/www/svn/browse_revision.php b/src/www/svn/browse_revision.php
index 6c72481cd9c..666f9c07fc2 100644
--- a/src/www/svn/browse_revision.php
+++ b/src/www/svn/browse_revision.php
@@ -79,20 +79,20 @@ if (! $request->valid($vGroupId)) {
             // reset list of sort criteria
             $morder = '';
         }
     }
 
-    $order_by = '';
+    $order_by = [];
     if (isset($morder)) {
         if (user_isloggedin()) {
             if ($morder != user_get_preference('svn_commit_browse_order' . $group_id)) {
                 user_set_preference('svn_commit_browse_order' . $group_id, $morder);
             }
         }
 
         if ($morder != '') {
-            $order_by = ' ORDER BY ' . svn_utils_criteria_list_to_query($morder);
+            $order_by = svn_utils_criteria_list_to_query($morder);
         }
     }
 
     $vPath = new Valid_String('_path');
     $vPath->required();
diff --git a/src/www/svn/svn_utils.php b/src/www/svn/svn_utils.php
index a5358c1b914..7efad0c68ce 100644
--- a/src/www/svn/svn_utils.php
+++ b/src/www/svn/svn_utils.php
@@ -334,34 +334,53 @@ function svn_utils_add_sort_criteria($criteria_list, $order, $msort)
     //echo "<br>DBG \$arr[]=".join(',',$arr);
 
     return(join(',', $arr));
 }
 
-// Transform criteria list to SQL query (+ means ascending
-// - is descending)
-function svn_utils_criteria_list_to_query($criteria_list)
+/**
+ * @psalm-return array<array{order:"ASC"|"DESC", column: "revision"|"commit_id"|"description"|"date"|"whoid"}>
+ */
+function svn_utils_criteria_list_to_query(string $criteria_list): array
 {
-    $criteria_list = str_replace('>', ' ASC', $criteria_list);
-    $criteria_list = str_replace('<', ' DESC', $criteria_list);
-    return $criteria_list;
+    $order_list = [];
+
+    foreach (explode(',', $criteria_list) as $criteria) {
+        if (preg_match('/^(?<column>[a-z]+)(?<order>[<>]?)$/', $criteria, $matches) === 1) {
+            $column = $matches['column'];
+            if (! in_array($column, ['revision', 'commit_id', 'description', 'date', 'whoid'], true)) {
+                continue;
+            }
+
+            $order = 'ASC';
+            if ($matches['order'] === '<') {
+                $order = 'DESC';
+            }
+
+            $order_list[] = ['order' => $order, 'column' => $column];
+        }
+    }
+
+    return $order_list;
 }
 
 // Transform criteria list to readable text statement
 // $url must not contain the morder parameter
 function svn_utils_criteria_list_to_text($criteria_list, $url)
 {
     if ($criteria_list) {
         $morder = '';
         $arr = explode(',', $criteria_list);
 
+        $purifier = Codendi_HTMLPurifier::instance();
+
         foreach ($arr as $crit) {
             $morder .= ($morder ? "," . $crit : $crit);
             $attr = str_replace('>', '', $crit);
             $attr = str_replace('<', '', $attr);
 
-            $arr_text[] = '<a href="' . $url . '&morder=' . $morder . '#results">' .
-            svn_utils_field_get_label($attr) . '</a><img src="' . util_get_dir_image_theme() .
+            $arr_text[] = '<a href="' . $url . '&morder=' . $purifier->purify(urlencode($morder)) . '#results">' .
+            $purifier->purify(svn_utils_field_get_label($attr)) . '</a><img src="' . util_get_dir_image_theme() .
             ((substr($crit, -1) == '<') ? 'dn' : 'up') .
             '_arrow.png" border="0">';
         }
     }
 
@@ -781,11 +800,14 @@ function svn_utils_is_there_specific_permission($project_svnroot)
 {
     $specifics = svn_utils_read_svn_access_file($project_svnroot);
     return ! $specifics || $specifics != '';
 }
 
-function svn_get_revisions(Project $project, $offset, $chunksz, $_rev_id = '', $_commiter = '', $_srch = '', $order_by = '', $pv = 0, $foundRows = true)
+/**
+ * @psalm-param array<array{order:"ASC"|"DESC", column: "revision"|"commit_id"|"description"|"date"|"whoid"}> $order_by
+ */
+function svn_get_revisions(Project $project, $offset, $chunksz, $_rev_id = '', $_commiter = '', $_srch = '', array $order_by = [], $pv = 0, $foundRows = true)
 {
     global $_path;
 
     $um = UserManager::instance();
 
@@ -850,30 +872,41 @@ function svn_get_revisions(Project $project, $offset, $chunksz, $_rev_id = '', $
         $srch_str = "";
     }
 
     $where .= $commiter_str . $commit_str . $srch_str . $path_str;
 
+    $limit = '';
     if (! isset($pv) || ! $pv) {
         $limit = " LIMIT " . db_ei($offset) . "," . db_ei($chunksz);
     }
 
-    // SQLi Warning: no real possibility to escape $order_by here.
-    // We rely on a proper filtering of user input by calling methods.
-    if (! isset($order_by) || $order_by == '') {
-        $order_by = " ORDER BY revision DESC ";
-    }
-
-    $sql = $select . $from . $where . $group_by . $order_by . $limit;
+    if (empty($order_by)) {
+        $order_by_sql = " ORDER BY revision DESC ";
+    } else {
+        $order_by_sql = ' ORDER BY ';
+        $order_by_sql .= implode(
+            ',',
+            array_map(
+                static function (array $order_by_row) {
+                    return $order_by_row['column'] . ' ' . $order_by_row['order'];
+                },
+                $order_by
+            )
+        );
+        $order_by_sql .= ' ';
+    }
+
+    $sql = $select . $from . $where . $group_by . $order_by_sql . $limit;
     //echo $sql."<br>\n";
     $result = db_query($sql);
 
     // Compute the number of rows.
     $totalrows = -1;
     if ($foundRows) {
         $sql1 = 'SELECT FOUND_ROWS() as nb';
         $result1 = db_query($sql1);
-        if ($result1 && ! db_error($result1)) {
+        if ($result1 && ! db_error()) {
             $row1 = db_fetch_array($result1);
             $totalrows = $row1['nb'];
         }
     }
 
diff --git a/tests/psalm/tuleap-baseline.xml b/tests/psalm/tuleap-baseline.xml
index d1c28dc7256..ff713f3a9e8 100644
--- a/tests/psalm/tuleap-baseline.xml
+++ b/tests/psalm/tuleap-baseline.xml
@@ -78672,11 +78672,11 @@
       <code>$Language</code>
       <code>$Language</code>
     </UndefinedGlobalVariable>
   </file>
   <file src="src/www/svn/svn_utils.php">
-    <DeprecatedFunction occurrences="20">
+    <DeprecatedFunction occurrences="25">
       <code>html_build_select_box_from_arrays($userids, $usernames, $name, $checked, true, $text_100, false, '', false, '', CODENDI_PURIFIER_CONVERT_HTML)</code>
       <code>db_fetch_array($result)</code>
       <code>db_numrows($result)</code>
       <code>db_result($result, 0, 'description')</code>
       <code>db_result($result, 0, 'revision')</code>
@@ -78693,10 +78693,14 @@
       <code>db_ei($_rev_id)</code>
       <code>db_ei($um-&gt;getUserByUserName($_commiter)-&gt;getId())</code>
       <code>db_es(htmlspecialchars($_srch))</code>
       <code>db_ei($offset)</code>
       <code>db_ei($chunksz)</code>
+      <code>db_query($sql)</code>
+      <code>db_query($sql1)</code>
+      <code>db_fetch_array($sql1)</code>
+      <code>db_error()</code>
     </DeprecatedFunction>
     <MissingParamType occurrences="51">
       <code>$params</code>
       <code>$params</code>
       <code>$params</code>

Vulnerable code:  * Copyright (c) Enalean, 2013 - 2018. All Rights Reserved.
                    list($latest_revisions, $nb_revisions) = svn_get_revisions($project, 0, $this->_nb_svn_commits, '', $user->getUserName(), '', '', 0, false);
                list($this->latest_revisions,) = svn_get_revisions($project, 0, 5, '', '', '', '', 0, false);
 * Copyright (c) Enalean, 2012-2018. All Rights Reserved.
            '',
    $order_by = '';
            $order_by = ' ORDER BY ' . svn_utils_criteria_list_to_query($morder);
// Transform criteria list to SQL query (+ means ascending
// - is descending)
function svn_utils_criteria_list_to_query($criteria_list)
    $criteria_list = str_replace('>', ' ASC', $criteria_list);
    $criteria_list = str_replace('<', ' DESC', $criteria_list);
    return $criteria_list;
            $arr_text[] = '<a href="' . $url . '&morder=' . $morder . '#results">' .
            svn_utils_field_get_label($attr) . '</a><img src="' . util_get_dir_image_theme() .
function svn_get_revisions(Project $project, $offset, $chunksz, $_rev_id = '', $_commiter = '', $_srch = '', $order_by = '', $pv = 0, $foundRows = true)
    // SQLi Warning: no real possibility to escape $order_by here.
    // We rely on a proper filtering of user input by calling methods.
    if (! isset($order_by) || $order_by == '') {
        $order_by = " ORDER BY revision DESC ";
    }

    $sql = $select . $from . $where . $group_by . $order_by . $limit;
        if ($result1 && ! db_error($result1)) {
    <DeprecatedFunction occurrences="20">
Fixed code:  * Copyright (c) Enalean, 2013 - Present. All Rights Reserved.
                    list($latest_revisions, $nb_revisions) = svn_get_revisions($project, 0, $this->_nb_svn_commits, '', $user->getUserName(), '', [], 0, false);
                list($this->latest_revisions,) = svn_get_revisions($project, 0, 5, '', '', '', [], 0, false);
 * Copyright (c) Enalean, 2012-Present. All Rights Reserved.
            [],
    $order_by = [];
            $order_by = svn_utils_criteria_list_to_query($morder);
/**
 * @psalm-return array<array{order:"ASC"|"DESC", column: "revision"|"commit_id"|"description"|"date"|"whoid"}>
 */
function svn_utils_criteria_list_to_query(string $criteria_list): array
    $order_list = [];

    foreach (explode(',', $criteria_list) as $criteria) {
        if (preg_match('/^(?<column>[a-z]+)(?<order>[<>]?)$/', $criteria, $matches) === 1) {
            $column = $matches['column'];
            if (! in_array($column, ['revision', 'commit_id', 'description', 'date', 'whoid'], true)) {
                continue;
            }

            $order = 'ASC';
            if ($matches['order'] === '<') {
                $order = 'DESC';
            }

            $order_list[] = ['order' => $order, 'column' => $column];
        }
    }

    return $order_list;
        $purifier = Codendi_HTMLPurifier::instance();

            $arr_text[] = '<a href="' . $url . '&morder=' . $purifier->purify(urlencode($morder)) . '#results">' .
            $purifier->purify(svn_utils_field_get_label($attr)) . '</a><img src="' . util_get_dir_image_theme() .
/**
 * @psalm-param array<array{order:"ASC"|"DESC", column: "revision"|"commit_id"|"description"|"date"|"whoid"}> $order_by
 */
function svn_get_revisions(Project $project, $offset, $chunksz, $_rev_id = '', $_commiter = '', $_srch = '', array $order_by = [], $pv = 0, $foundRows = true)
    $limit = '';
    if (empty($order_by)) {
        $order_by_sql = " ORDER BY revision DESC ";
    } else {
        $order_by_sql = ' ORDER BY ';
        $order_by_sql .= implode(
            ',',
            array_map(
                static function (array $order_by_row) {
                    return $order_by_row['column'] . ' ' . $order_by_row['order'];
                },
                $order_by
            )
        );
        $order_by_sql .= ' ';
    }

    $sql = $select . $from . $where . $group_by . $order_by_sql . $limit;
        if ($result1 && ! db_error()) {
    <DeprecatedFunction occurrences="25">
      <code>db_query($sql)</code>
      <code>db_query($sql1)</code>
      <code>db_fetch_array($sql1)</code>
      <code>db_error()</code>