From 5642a8e51586df7a129b14904ea4f558fd704e9d Mon Sep 17 00:00:00 2001 From: Aaron Barnes Date: Wed, 8 Sep 2010 02:32:43 +0000 Subject: [PATCH] MDL-24081 completion Fixing sql injections and use of sql_ilike() --- course/report/completion/index.php | 13 +++-- course/report/progress/index.php | 13 +++-- lib/completionlib.php | 76 ++++++++++++++++++++---------- 3 files changed, 68 insertions(+), 34 deletions(-) diff --git a/course/report/completion/index.php b/course/report/completion/index.php index fa7239999be..ff26f2cc05a 100644 --- a/course/report/completion/index.php +++ b/course/report/completion/index.php @@ -184,21 +184,23 @@ if ($csv) { // Generate where clause $where = array(); -$ilike = $DB->sql_ilike(); +$where_params = array(); if ($sifirst !== 'all') { - $where[] = "u.firstname $ilike '$sifirst%'"; + $where[] = $DB->sql_like('u.firstname', ':sifirst', false); + $where_params['sifirst'] = $sifirst.'%'; } if ($silast !== 'all') { - $where[] = "u.lastname $ilike '$silast%'"; + $where[] = $DB->sql_like('u.lastname', ':silast', false); + $where_params['silast'] = $silast.'%'; } // Get user match count -$total = $completion->get_num_tracked_users(implode(' AND ', $where), $group); +$total = $completion->get_num_tracked_users(implode(' AND ', $where), $where_params, $group); // Total user count -$grandtotal = $completion->get_num_tracked_users('', $group); +$grandtotal = $completion->get_num_tracked_users('', array(), $group); // If no users in this course what-so-ever if (!$grandtotal) { @@ -213,6 +215,7 @@ $progress = array(); if ($total) { $progress = $completion->get_progress_all( implode(' AND ', $where), + $where_params, $group, $firstnamesort ? 'u.firstname ASC' : 'u.lastname ASC', $csv ? 0 : COMPLETION_REPORT_PAGE, diff --git a/course/report/progress/index.php b/course/report/progress/index.php index 6a682f0faf7..122ea93b56c 100644 --- a/course/report/progress/index.php +++ b/course/report/progress/index.php @@ -78,21 +78,23 @@ if(count($activities)==0) { // Generate where clause $where = array(); -$ilike = $DB->sql_ilike(); +$where_params = array(); if ($sifirst !== 'all') { - $where[] = "u.firstname $ilike '$sifirst%'"; + $where[] = $DB->sql_like('u.firstname', ':sifirst', false); + $where_params['sifirst'] = $sifirst.'%'; } if ($silast !== 'all') { - $where[] = "u.lastname $ilike '$silast%'"; + $where[] = $DB->sql_like('u.lastname', ':silast', false); + $where_params['silast'] = $silast.'%'; } // Get user match count -$total = $completion->get_num_tracked_users(implode(' AND ', $where), $group); +$total = $completion->get_num_tracked_users(implode(' AND ', $where), $where_params, $group); // Total user count -$grandtotal = $completion->get_num_tracked_users('', $group); +$grandtotal = $completion->get_num_tracked_users('', array(), $group); // If no users in this course what-so-ever if (!$grandtotal) { @@ -110,6 +112,7 @@ $progress = array(); if ($total) { $progress = $completion->get_progress_all( implode(' AND ', $where), + $where_params, $group, $firstnamesort ? 'u.firstname ASC' : 'u.lastname ASC', $csv ? 0 : COMPLETION_REPORT_PAGE, diff --git a/lib/completionlib.php b/lib/completionlib.php index c12622ea819..d1d8141dadd 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -973,10 +973,15 @@ class completion_info { function is_tracked_user($userid) { global $DB; + $tracked = $this->generate_tracked_user_sql(); + $sql = "SELECT u.id "; - $sql .= $this->generate_tracked_user_sql(); + $sql .= $tracked->sql; $sql .= ' AND u.id = :user'; - return $DB->record_exists_sql($sql, array('user' => (int)$userid)); + + $params = $tracked->data; + $params['user'] = (int)$userid; + return $DB->record_exists_sql($sql, $params); } @@ -985,21 +990,25 @@ class completion_info { * * Optionally supply a search's where clause, or a group id * - * @param string $where Where clause + * @param string $where Where clause sql + * @param array $where_params Where clause params * @param int $groupid Group id * @return int */ - function get_num_tracked_users($where = '', $groupid = 0) { + function get_num_tracked_users($where = '', $where_params = array(), $groupid = 0) { global $DB; + $tracked = $this->generate_tracked_user_sql($groupid); + $sql = "SELECT COUNT(u.id) "; - $sql .= $this->generate_tracked_user_sql($groupid); + $sql .= $tracked->sql; if ($where) { $sql .= " AND $where"; } - return $DB->count_records_sql($sql); + $params = array_merge($tracked->data, $where_params); + return $DB->count_records_sql($sql, $params); } @@ -1008,18 +1017,22 @@ class completion_info { * * Optionally supply a search's where caluse, group id, sorting, paging * - * @param string $where Where clause (optional) + * @param string $where Where clause sql (optional) + * @param array $where_params Where clause params (optional) * @param integer $groupid Group ID to restrict to (optional) * @param string $sort Order by clause (optional) * @param integer $limitfrom Result start (optional) * @param integer $limitnum Result max size (optional) * @return array */ - function get_tracked_users($where = '', $groupid = 0, $sort = '', - $limitfrom = '', $limitnum = '') { + function get_tracked_users($where = '', $where_params = array(), $groupid = 0, + $sort = '', $limitfrom = '', $limitnum = '') { global $DB; + $tracked = $this->generate_tracked_user_sql($groupid); + $params = $tracked->data; + $sql = " SELECT u.id, @@ -1028,17 +1041,18 @@ class completion_info { u.idnumber "; - $sql .= $this->generate_tracked_user_sql($groupid); + $sql .= $tracked->sql; if ($where) { $sql .= " AND $where"; + $params = array_merge($params, $where_params); } if ($sort) { $sql .= " ORDER BY $sort"; } - $users = $DB->get_records_sql($sql, null, $limitfrom, $limitnum); + $users = $DB->get_records_sql($sql, $params, $limitfrom, $limitnum); return $users ? $users : array(); // In case it returns false } @@ -1046,14 +1060,19 @@ class completion_info { /** * Generate the SQL for finding tracked users in this course * - * Optionally supply a group id + * Returns an object containing the sql fragment and an array of + * bound data params. * * @param integer $groupid - * @return string + * @return object */ function generate_tracked_user_sql($groupid = 0) { global $CFG; + $return = new stdClass(); + $return->sql = ''; + $return->data = array(); + if (!empty($CFG->progresstrackedroles)) { $roles = ' AND ra.roleid IN ('.$CFG->progresstrackedroles.')'; } else { @@ -1074,12 +1093,12 @@ class completion_info { if ($groupid) { $groupjoin = "JOIN {groups_members} gm ON gm.userid = u.id"; - $groupselect = " AND gm.groupid = $groupid "; + $groupselect = " AND gm.groupid = :groupid "; + + $return->data['groupid'] = $groupid; } - $now = time(); - - $sql = " + $return->sql = " FROM {user} u INNER JOIN @@ -1099,17 +1118,23 @@ class completion_info { ON c.id = e.courseid $groupjoin WHERE - (ra.contextid = {$context->id} $parentcontexts) - AND c.id = {$this->course->id} + (ra.contextid = :contextid $parentcontexts) + AND c.id = :courseid AND ue.status = 0 AND e.status = 0 - AND ue.timestart < $now - AND (ue.timeend > $now OR ue.timeend = 0) + AND ue.timestart < :now1 + AND (ue.timeend > :now2 OR ue.timeend = 0) $groupselect $roles "; - return $sql; + $now = time(); + $return->data['now1'] = $now; + $return->data['now2'] = $now; + $return->data['contextid'] = $context->id; + $return->data['courseid'] = $this->course->id; + + return $return; } /** @@ -1126,6 +1151,8 @@ class completion_info { * @global object * @param bool $sortfirstname If true, sort by first name, otherwise sort by * last name + * @param string $where Where clause sql (optional) + * @param array $where_params Where clause params (optional) * @param int $groupid Group ID or 0 (default)/false for all groups * @param int $pagesize Number of users to actually return (optional) * @param int $start User to start at if paging (optional) @@ -1133,11 +1160,12 @@ class completion_info { * an array of user objects (like mdl_user id, firstname, lastname) * containing an additional ->progress array of coursemoduleid => completionstate */ - public function get_progress_all($where = '', $groupid = 0, $sort = '', $pagesize = '', $start = '') { + public function get_progress_all($where = '', $where_params = array(), $groupid = 0, + $sort = '', $pagesize = '', $start = '') { global $CFG, $DB; // Get list of applicable users - $users = $this->get_tracked_users($where, $groupid, $sort, $start, $pagesize); + $users = $this->get_tracked_users($where, $where_params, $groupid, $sort, $start, $pagesize); // Get progress information for these users in groups of 1, 000 (if needed) // to avoid making the SQL IN too long