request #16213: SQL injection in the "SVN core" commits browser

Also fixes a XSS via the same injection point.

Change-Id: Ib33199b7d0ec5f8a07a65d74766477f23df4a75b

src/common/Widget/Widget_MyLatestSvnCommits.class.php

@@ -1,6 +1,6 @@
 <?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.
@@ -80,7 +80,7 @@ public function getContent()
 
                 $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;

src/common/Widget/Widget_ProjectLatestSvnCommits.class.php

@@ -61,7 +61,7 @@ public function getLatestRevisions()
             $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;

src/common/svn/SVN_LogFactory.class.php

@@ -1,6 +1,6 @@
 <?php
 /**
- * Copyright (c) Enalean, 2012-2018. All Rights Reserved.
+ * Copyright (c) Enalean, 2012-Present. All Rights Reserved.
  *
  * This file is a part of Tuleap.
  *
@@ -138,7 +138,7 @@ public function getRawRevisionsAndCount($limit, PFUser $author)
             '',
             $author->getUserName(),
             '',
-            '',
+            [],
             0,
             false
         );

src/www/svn/browse_revision.php

@@ -81,7 +81,7 @@
         }
     }
 
-    $order_by = '';
+    $order_by = [];
     if (isset($morder)) {
         if (user_isloggedin()) {
             if ($morder != user_get_preference('svn_commit_browse_order' . $group_id)) {
@@ -90,7 +90,7 @@
         }
 
         if ($morder != '') {
-            $order_by = ' ORDER BY ' . svn_utils_criteria_list_to_query($morder);
+            $order_by = svn_utils_criteria_list_to_query($morder);
         }
     }
 

src/www/svn/svn_utils.php

@@ -336,13 +336,30 @@ function svn_utils_add_sort_criteria($criteria_list, $order, $msort)
     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
@@ -353,13 +370,15 @@ function svn_utils_criteria_list_to_text($criteria_list, $url)
         $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">';
         }
@@ -783,7 +802,10 @@ function svn_utils_is_there_specific_permission($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;
 
@@ -852,17 +874,28 @@ function svn_get_revisions(Project $project, $offset, $chunksz, $_rev_id = '', $
 
     $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);
 
@@ -871,7 +904,7 @@ function svn_get_revisions(Project $project, $offset, $chunksz, $_rev_id = '', $
     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'];
         }

tests/psalm/tuleap-baseline.xml

@@ -78674,7 +78674,7 @@
     </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>
@@ -78695,6 +78695,10 @@
       <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>