From 54d05c11b97b20e5286b9cb5ce9e9670a96d3c64 Mon Sep 17 00:00:00 2001 From: Yannick Warnier Date: Thu, 20 Dec 2018 23:56:49 -0500 Subject: [PATCH] Security: Fix suspected XSS/SQL injections vulnerabilities in tickets --- main/inc/lib/TicketManager.php | 46 ++++++++++++++--------------- main/ticket/assign_tickets.php | 10 +++---- main/ticket/categories.php | 6 ++-- main/ticket/categories_add_user.php | 2 +- main/ticket/course_user_list.php | 2 +- main/ticket/download.php | 2 +- main/ticket/new_ticket.php | 16 +++++----- main/ticket/projects.php | 6 ++-- main/ticket/status.php | 6 ++-- main/ticket/ticket_assign_log.php | 2 +- main/ticket/ticket_details.php | 26 ++++++++-------- main/ticket/tutor_report.lib.php | 4 +-- main/ticket/update_report.php | 10 +++---- 13 files changed, 68 insertions(+), 70 deletions(-) diff --git a/main/inc/lib/TicketManager.php b/main/inc/lib/TicketManager.php index 92034b895a..e2b7263dc0 100644 --- a/main/inc/lib/TicketManager.php +++ b/main/inc/lib/TicketManager.php @@ -113,7 +113,7 @@ class TicketManager public static function getCategory($id) { $table = Database::get_main_table(TABLE_TICKET_CATEGORY); - $id = intval($id); + $id = (int) $id; $sql = "SELECT id, name, description, total_tickets FROM $table WHERE id = $id"; @@ -146,7 +146,7 @@ class TicketManager public static function updateCategory($id, $params) { $table = Database::get_main_table(TABLE_TICKET_CATEGORY); - $id = intval($id); + $id = (int) $id; Database::update($table, $params, ['id = ?' => $id]); } @@ -314,10 +314,10 @@ class TicketManager $currentUserId = api_get_user_id(); $currentUserInfo = api_get_user_info(); $now = api_get_utc_datetime(); - $course_id = intval($course_id); - $category_id = intval($category_id); - $project_id = intval($project_id); - $priority = empty($priority) ? self::PRIORITY_NORMAL : $priority; + $course_id = (int) $course_id; + $category_id = (int) $category_id; + $project_id = (int) $project_id; + $priority = empty($priority) ? self::PRIORITY_NORMAL : (int) $priority; if ($status === '') { $status = self::STATUS_NEW; @@ -360,8 +360,8 @@ class TicketManager 'sys_lastedit_datetime' => $now, 'source' => $source, 'assigned_last_user' => $assignedUserId, - 'subject' => $subject, - 'message' => $content, + 'subject' => Database::escape_string($subject), + 'message' => Database::escape_string($content), ]; if (!empty($course_id)) { @@ -653,26 +653,26 @@ class TicketManager $params = [ 'ticket_id' => $ticketId, - 'subject' => $subject, - 'message' => $content, - 'ip_address' => $_SERVER['REMOTE_ADDR'], + 'subject' => Database::escape_string($subject), + 'message' => Database::escape_string($content), + 'ip_address' => Database::escape_string(api_get_real_ip()), 'sys_insert_user_id' => $userId, 'sys_insert_datetime' => $now, 'sys_lastedit_user_id' => $userId, 'sys_lastedit_datetime' => $now, - 'status' => $status, + 'status' => Database::escape_string($status), ]; $messageId = Database::insert($table_support_messages, $params); if ($messageId) { // update_total_message $sql = "UPDATE $table_support_tickets SET - sys_lastedit_user_id ='$userId', - sys_lastedit_datetime ='$now', + sys_lastedit_user_id = $userId, + sys_lastedit_datetime = '$now', total_messages = ( SELECT COUNT(*) as total_messages FROM $table_support_messages - WHERE ticket_id ='$ticketId' + WHERE ticket_id = $ticketId ) WHERE id = $ticketId "; Database::query($sql); @@ -1409,9 +1409,9 @@ class TicketManager $now = api_get_utc_datetime(); $table = Database::get_main_table(TABLE_TICKET_TICKET); $newParams = [ - 'priority_id' => isset($params['priority_id']) ? $params['priority_id'] : '', - 'status_id' => isset($params['status_id']) ? $params['status_id'] : '', - 'sys_lastedit_user_id' => $userId, + 'priority_id' => isset($params['priority_id']) ? (int) $params['priority_id'] : '', + 'status_id' => isset($params['status_id']) ? (int) $params['status_id'] : '', + 'sys_lastedit_user_id' => (int) $userId, 'sys_lastedit_datetime' => $now, ]; Database::update($table, $newParams, ['id = ? ' => $ticketId]); @@ -1503,14 +1503,14 @@ class TicketManager $table_support_tickets = Database::get_main_table(TABLE_TICKET_TICKET); $now = api_get_utc_datetime(); - $ticketId = intval($ticketId); - $userId = intval($userId); + $ticketId = (int) $ticketId; + $userId = (int) $userId; $sql = "UPDATE $table_support_tickets SET priority_id = '".self::PRIORITY_HIGH."', - sys_lastedit_user_id ='$userId', - sys_lastedit_datetime ='$now' - WHERE id = '$ticketId'"; + sys_lastedit_user_id = $userId, + sys_lastedit_datetime = '$now' + WHERE id = $ticketId"; Database::query($sql); } diff --git a/main/ticket/assign_tickets.php b/main/ticket/assign_tickets.php index d6d9dcdd49..1389f787c9 100644 --- a/main/ticket/assign_tickets.php +++ b/main/ticket/assign_tickets.php @@ -16,9 +16,9 @@ $course_code = $course_info['code']; echo '
'; echo '
'; -$id = intval($_GET['id']); +$id = (int) $_GET['id']; $tblWeeklyReport = Database::get_main_table('rp_reporte_semanas'); -$sql = "SELECT * FROM $tblWeeklyReport WHERE id = '$id'"; +$sql = "SELECT * FROM $tblWeeklyReport WHERE id = $id"; $sql_tasks = "SELECT id AS colid, title as coltitle FROM ".Database::get_course_table(TABLE_STUDENT_PUBLICATION)." WHERE parent_id = 0 @@ -27,7 +27,7 @@ $sql_tasks = "SELECT id AS colid, title as coltitle FROM $tblWeeklyReport WHERE course_code = '$course_code' AND - id != '$id' + id != $id )"; $sql_forum = "SELECT thread_id AS colid, thread_title AS coltitle FROM ".Database::get_course_table(TABLE_FORUM_THREAD)." @@ -36,7 +36,7 @@ $sql_forum = "SELECT thread_id AS colid, thread_title AS coltitle FROM $tblWeeklyReport WHERE course_code = '$course_code' AND - id != '$id' + id != $id )"; $rs = Database::fetch_object(Database::query($sql)); $result_tareas = Database::query($sql_tasks); @@ -65,7 +65,7 @@ while ($row = Database::fetch_assoc($result_forum)) { echo '
'; echo '
-
'; diff --git a/main/ticket/categories.php b/main/ticket/categories.php index aeb12b126f..55cf4d5ca5 100644 --- a/main/ticket/categories.php +++ b/main/ticket/categories.php @@ -37,7 +37,7 @@ if ($table->per_page == 0) { } $formToString = ''; -$id = isset($_GET['id']) ? intval($_GET['id']) : 0; +$id = isset($_GET['id']) ? (int) $_GET['id'] : 0; $projectId = isset($_GET['project_id']) ? (int) $_GET['project_id'] : 0; $project = TicketManager::getProject($projectId); @@ -129,8 +129,8 @@ switch ($action) { $values = $form->getSubmitValues(); $params = [ - 'name' => $values['name'], - 'description' => $values['description'], + 'name' => Database::escape_string($values['name']), + 'description' => Database::escape_string($values['description']), 'sys_lastedit_datetime' => api_get_utc_datetime(), 'sys_lastedit_user_id' => api_get_user_id(), ]; diff --git a/main/ticket/categories_add_user.php b/main/ticket/categories_add_user.php index e51e78c73e..58ea0bbccc 100644 --- a/main/ticket/categories_add_user.php +++ b/main/ticket/categories_add_user.php @@ -8,7 +8,7 @@ require_once __DIR__.'/../inc/global.inc.php'; api_protect_admin_script(true); -$categoryId = isset($_REQUEST['id']) ? intval($_REQUEST['id']) : 0; +$categoryId = isset($_REQUEST['id']) ? (int) $_REQUEST['id'] : 0; $projectId = isset($_GET['project_id']) ? (int) $_GET['project_id'] : ''; $categoryInfo = TicketManager::getCategory($categoryId); diff --git a/main/ticket/course_user_list.php b/main/ticket/course_user_list.php index 2d51c65582..24c86c8dd3 100644 --- a/main/ticket/course_user_list.php +++ b/main/ticket/course_user_list.php @@ -6,7 +6,7 @@ */ require_once __DIR__.'/../inc/global.inc.php'; -$userId = intval($_GET['user_id']); +$userId = (int) $_GET['user_id']; $userInfo = api_get_user_info($userId); $coursesList = CourseManager::get_courses_list_by_user_id($userId, false, true); diff --git a/main/ticket/download.php b/main/ticket/download.php index 0acece95a1..7bffcee2d6 100644 --- a/main/ticket/download.php +++ b/main/ticket/download.php @@ -13,7 +13,7 @@ if (!isset($_GET['id']) || !isset($_GET['ticket_id'])) { api_not_allowed(true); } -$ticket_id = intval($_GET['ticket_id']); +$ticket_id = (int) $_GET['ticket_id']; $ticketInfo = TicketManager::get_ticket_detail_by_id($ticket_id); if (empty($ticketInfo)) { api_not_allowed(true); diff --git a/main/ticket/new_ticket.php b/main/ticket/new_ticket.php index e81e573e77..bf2c3efd74 100644 --- a/main/ticket/new_ticket.php +++ b/main/ticket/new_ticket.php @@ -149,18 +149,18 @@ function save_ticket() if ($_POST['phone'] != '') { $content .= '

 '.get_lang('Phone').': '.$_POST['phone'].'

'; } - $course_id = isset($_POST['course_id']) ? $_POST['course_id'] : ''; - $sessionId = isset($_POST['session_id']) ? $_POST['session_id'] : ''; - $category_id = isset($_POST['category_id']) ? $_POST['category_id'] : ''; + $course_id = isset($_POST['course_id']) ? (int) $_POST['course_id'] : ''; + $sessionId = isset($_POST['session_id']) ? (int) $_POST['session_id'] : ''; + $category_id = isset($_POST['category_id']) ? (int) $_POST['category_id'] : ''; - $project_id = $_POST['project_id']; + $project_id = (int) $_POST['project_id']; $subject = $_POST['subject']; $other_area = (int) $_POST['other_area']; $personal_email = $_POST['personal_email']; - $source = $_POST['source_id']; - $user_id = isset($_POST['user_id']) ? $_POST['user_id'] : 0; - $priority = isset($_POST['priority_id']) ? $_POST['priority_id'] : ''; - $status = isset($_POST['status_id']) ? $_POST['status_id'] : ''; + $source = (int) $_POST['source_id']; + $user_id = isset($_POST['user_id']) ? (int) $_POST['user_id'] : 0; + $priority = isset($_POST['priority_id']) ? (int) $_POST['priority_id'] : ''; + $status = isset($_POST['status_id']) ? (int) $_POST['status_id'] : ''; $file_attachments = $_FILES; if (TicketManager::add( diff --git a/main/ticket/projects.php b/main/ticket/projects.php index 888e20eece..466afefaea 100644 --- a/main/ticket/projects.php +++ b/main/ticket/projects.php @@ -30,7 +30,7 @@ if ($table->per_page == 0) { } $formToString = ''; -$id = isset($_GET['id']) ? intval($_GET['id']) : 0; +$id = isset($_GET['id']) ? (int) $_GET['id'] : 0; $action = isset($_GET['action']) ? $_GET['action'] : ''; $interbreadcrumb[] = [ @@ -82,7 +82,7 @@ switch ($action) { } break; case 'edit': - $item = TicketManager::getProject($_GET['id']); + $item = TicketManager::getProject($id); if (empty($item)) { api_not_allowed(true); } @@ -105,7 +105,7 @@ switch ($action) { 'sys_lastedit_datetime' => api_get_utc_datetime(), 'sys_lastedit_user_id' => api_get_user_id(), ]; - TicketManager::updateProject($_GET['id'], $params); + TicketManager::updateProject($id, $params); Display::addFlash(Display::return_message(get_lang('Updated'))); header("Location: ".api_get_self()); exit; diff --git a/main/ticket/status.php b/main/ticket/status.php index 86d91e6758..c20fe683aa 100644 --- a/main/ticket/status.php +++ b/main/ticket/status.php @@ -32,7 +32,7 @@ if ($table->per_page == 0) { } $formToString = ''; -$id = isset($_GET['id']) ? intval($_GET['id']) : 0; +$id = isset($_GET['id']) ? (int) $_GET['id'] : 0; $action = isset($_GET['action']) ? $_GET['action'] : ''; $interbreadcrumb[] = [ @@ -89,7 +89,7 @@ switch ($action) { $url = api_get_self().'?action=edit&id='.$id; $form = TicketManager::getStatusForm($url); - $item = TicketManager::getStatus($_GET['id']); + $item = TicketManager::getStatus($id); $form->setDefaults([ 'name' => $item->getName(), 'description' => $item->getDescription(), @@ -102,7 +102,7 @@ switch ($action) { 'name' => $values['name'], 'description' => $values['description'], ]; - $cat = TicketManager::updateStatus($_GET['id'], $params); + $cat = TicketManager::updateStatus($id, $params); Display::addFlash(Display::return_message(get_lang('Updated'))); header("Location: ".api_get_self()); exit; diff --git a/main/ticket/ticket_assign_log.php b/main/ticket/ticket_assign_log.php index 60aa425969..3fa31275ec 100644 --- a/main/ticket/ticket_assign_log.php +++ b/main/ticket/ticket_assign_log.php @@ -12,7 +12,7 @@ if (!isset($_POST['ticket_id'])) { exit; } -$ticket_id = intval($_POST['ticket_id']); +$ticket_id = (int) $_POST['ticket_id']; $history = TicketManager::get_assign_log($ticket_id); ?> diff --git a/main/ticket/ticket_details.php b/main/ticket/ticket_details.php index 0f79824b13..e2c081c70c 100644 --- a/main/ticket/ticket_details.php +++ b/main/ticket/ticket_details.php @@ -119,12 +119,12 @@ $htmlHeadXtra[] = ''; -$ticket_id = $_GET['ticket_id']; +$ticket_id = (int) $_REQUEST['ticket_id']; $ticket = TicketManager::get_ticket_detail_by_id($ticket_id); if (!isset($ticket['ticket'])) { api_not_allowed(true); } -if (!isset($_GET['ticket_id'])) { +if (!isset($_REQUEST['ticket_id'])) { header('Location: '.api_get_path(WEB_CODE_PATH).'ticket/tickets.php'); exit; } @@ -150,7 +150,7 @@ if (!isset($_GET['ticket_id'])) { $title = 'Ticket #'.$ticket['ticket']['code']; if (isset($_REQUEST['close'])) { - TicketManager::close_ticket($_REQUEST['ticket_id'], $user_id); + TicketManager::close_ticket($ticket_id, $user_id); $ticket['ticket']['status_id'] = TicketManager::STATUS_CLOSE; $ticket['ticket']['status'] = get_lang('Closed'); } @@ -169,11 +169,11 @@ foreach ($messages as $message) { $receivedMessage = ''; if (!empty($message['subject'])) { - $receivedMessage = ''.get_lang('Subject').': '.$message['subject'].'
'; + $receivedMessage = ''.get_lang('Subject').': '.Security::remove_XSS($message['subject']).'
'; } if (!empty($message['message'])) { - $receivedMessage = ''.get_lang('Message').':
'.$message['message'].'
'; + $receivedMessage = ''.get_lang('Message').':
'.Security::remove_XSS($message['message']).'
'; } $attachmentLinks = ''; @@ -206,7 +206,7 @@ foreach ($messages as $message) { $counter++; } -$subject = get_lang('ReplyShort').': '.$ticket['ticket']['subject']; +$subject = get_lang('ReplyShort').': '.Security::remove_XSS($ticket['ticket']['subject']); if ($ticket['ticket']['status_id'] != TicketManager::STATUS_FORWARDED && $ticket['ticket']['status_id'] != TicketManager::STATUS_CLOSE @@ -219,10 +219,8 @@ if ($ticket['ticket']['status_id'] != TicketManager::STATUS_FORWARDED && $formToShow = $form->returnForm(); if ($form->validate()) { - $ticket_id = $_POST['ticket_id']; - $content = $_POST['content']; + $ticket_id = (int) $_POST['ticket_id']; $messageToSend = ''; - $subject = $_POST['subject']; $message = isset($_POST['confirmation']) ? true : false; $file_attachments = $_FILES; @@ -258,8 +256,8 @@ if ($ticket['ticket']['status_id'] != TicketManager::STATUS_FORWARDED && TicketManager::updateTicket( [ - 'priority_id' => $_POST['priority_id'], - 'status_id' => $_POST['status_id'], + 'priority_id' => (int) $_POST['priority_id'], + 'status_id' => (int) $_POST['status_id'], ], $ticket_id, api_get_user_id() @@ -311,16 +309,16 @@ if ($ticket['ticket']['status_id'] != TicketManager::STATUS_FORWARDED && } } - $messageToSend .= $content; + $messageToSend .= $_POST['content']; TicketManager::insertMessage( $ticket_id, - $subject, + $_POST['subject'], $messageToSend, $file_attachments, $user_id, 'NOL', - $message + Database::escape_string($message) ); TicketManager::sendNotification( diff --git a/main/ticket/tutor_report.lib.php b/main/ticket/tutor_report.lib.php index 200118f792..2f350277cb 100644 --- a/main/ticket/tutor_report.lib.php +++ b/main/ticket/tutor_report.lib.php @@ -28,7 +28,7 @@ function initializeReport($course_code) $resWeeks = Database::query($sqlWeeks); $weeks = Database::fetch_object($resWeeks); $obj = Database::fetch_object($res); - $weeksCount = (!isset($_POST['weeksNumber'])) ? (($weeks->semanas == 0) ? 7 : $weeks->semanas) : $_POST['weeksNumber']; + $weeksCount = (!isset($_POST['weeksNumber'])) ? (($weeks->semanas == 0) ? 7 : $weeks->semanas) : (int) $_POST['weeksNumber']; $weeksCount = Database::escape_string($weeksCount); Database::query("REPLACE INTO $table_semanas_curso (course_code , semanas) VALUES ('$course_code','$weeksCount')"); if (intval($obj->cant) != $weeksCount) { @@ -57,7 +57,7 @@ function initializeReport($course_code) if (!Database::query($sql)) { return false; } else { - $page = (!isset($_GET['page'])) ? 1 : $_GET['page']; + $page = (!isset($_GET['page'])) ? 1 : (int) $_GET['page']; Database::query("UPDATE $table_students_report sr SET sr.work_ok = 1 WHERE CONCAT (sr.user_id,',',sr.week_report_id) diff --git a/main/ticket/update_report.php b/main/ticket/update_report.php index dc0ff777bd..adb8553aa2 100644 --- a/main/ticket/update_report.php +++ b/main/ticket/update_report.php @@ -7,17 +7,17 @@ exit; require_once __DIR__.'/../inc/global.inc.php'; -$work_id = intval($_POST['work_id']); -$forum_id = intval($_POST['forum_id']); -$rs_id = intval($_POST['rs_id']); +$work_id = (int) $_POST['work_id']; +$forum_id = (int) $_POST['forum_id']; +$rs_id = (int) $_POST['rs_id']; api_protect_course_script(); if (!api_is_allowed_to_edit()) { echo Display::return_message(get_lang("DeniedAccess"), 'error'); } else { $sql = "UPDATE ".Database::get_main_table('rp_reporte_semanas')." - SET work_id = '$work_id', forum_id = '$forum_id' - WHERE id ='$rs_id'"; + SET work_id = $work_id, forum_id = $forum_id + WHERE id = $rs_id"; Database::query($sql); echo Display::return_message(get_lang('Updated'), 'confirm'); }