Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
MDL-57580 mod_assign: Fix the incorrect type of some input parameters
The PARAM_TEXT has been misused in certain cases here. The 'action'
parameter seems to always be alphabetic, with values like
savesubmission, editsubmission and others as handled in assign::view().

Fixing the action handling fixes the reported XSS issue. While working
on it, I spotted two more places where PARAM_TEXT does not seem
appropriate. I include changes for them too, even if they are no
strictly related to the reported bug and there are no known ways to
abuse it.

* The 'plugin' looks like PARAM_PLUGIN and is even declared as such in
  some other parts of the assignment code (such as feedback forms).

* The 'workflowstate' is one of the ASSIGN_MARKING_WORKFLOW_STATE
  constants and is supposed to be alpha in external function input
  parameters handling, too.
  • Loading branch information
mudrd8mz authored and dmonllao committed Jan 5, 2017
1 parent 7716be5 commit 82a8d0d
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 8 deletions.
4 changes: 2 additions & 2 deletions mod/assign/externallib.php
Expand Up @@ -895,7 +895,7 @@ public static function set_user_flags_parameters() {
'locked' => new external_value(PARAM_INT, 'locked', VALUE_OPTIONAL),
'mailed' => new external_value(PARAM_INT, 'mailed', VALUE_OPTIONAL),
'extensionduedate' => new external_value(PARAM_INT, 'extension due date', VALUE_OPTIONAL),
'workflowstate' => new external_value(PARAM_TEXT, 'marking workflow state', VALUE_OPTIONAL),
'workflowstate' => new external_value(PARAM_ALPHA, 'marking workflow state', VALUE_OPTIONAL),
'allocatedmarker' => new external_value(PARAM_INT, 'allocated marker', VALUE_OPTIONAL)
)
)
Expand Down Expand Up @@ -1147,7 +1147,7 @@ private static function assign_user_flags() {
'locked' => new external_value(PARAM_INT, 'locked'),
'mailed' => new external_value(PARAM_INT, 'mailed'),
'extensionduedate' => new external_value(PARAM_INT, 'extension due date'),
'workflowstate' => new external_value(PARAM_TEXT, 'marking workflow state', VALUE_OPTIONAL),
'workflowstate' => new external_value(PARAM_ALPHA, 'marking workflow state', VALUE_OPTIONAL),
'allocatedmarker' => new external_value(PARAM_INT, 'allocated marker')
)
)
Expand Down
8 changes: 4 additions & 4 deletions mod/assign/locallib.php
Expand Up @@ -2752,7 +2752,7 @@ protected function view_plugin_page() {
$o = '';

$pluginsubtype = required_param('pluginsubtype', PARAM_ALPHA);
$plugintype = required_param('plugin', PARAM_TEXT);
$plugintype = required_param('plugin', PARAM_PLUGIN);
$pluginaction = required_param('pluginaction', PARAM_ALPHA);

$plugin = $this->get_plugin_by_type($pluginsubtype, $plugintype);
Expand Down Expand Up @@ -2826,7 +2826,7 @@ protected function view_plugin_content($pluginsubtype) {

$submissionid = optional_param('sid', 0, PARAM_INT);
$gradeid = optional_param('gid', 0, PARAM_INT);
$plugintype = required_param('plugin', PARAM_TEXT);
$plugintype = required_param('plugin', PARAM_PLUGIN);
$item = null;
if ($pluginsubtype == 'assignsubmission') {
$plugin = $this->get_submission_plugin_by_type($plugintype);
Expand Down Expand Up @@ -6170,7 +6170,7 @@ protected function process_save_quick_grades() {
$record->userid = $userid;
if ($modified >= 0) {
$record->grade = unformat_float(optional_param('quickgrade_' . $record->userid, -1, PARAM_TEXT));
$record->workflowstate = optional_param('quickgrade_' . $record->userid.'_workflowstate', false, PARAM_TEXT);
$record->workflowstate = optional_param('quickgrade_' . $record->userid.'_workflowstate', false, PARAM_ALPHA);
$record->allocatedmarker = optional_param('quickgrade_' . $record->userid.'_allocatedmarker', false, PARAM_INT);
} else {
// This user was not in the grading table.
Expand Down Expand Up @@ -7337,7 +7337,7 @@ public function add_submission_form_elements(MoodleQuickForm $mform, stdClass $d
$mform->setType('userid', PARAM_INT);

$mform->addElement('hidden', 'action', 'savesubmission');
$mform->setType('action', PARAM_TEXT);
$mform->setType('action', PARAM_ALPHA);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions mod/assign/view.php
Expand Up @@ -37,7 +37,7 @@

$assign = new assign($context, $cm, $course);
$urlparams = array('id' => $id,
'action' => optional_param('action', '', PARAM_TEXT),
'action' => optional_param('action', '', PARAM_ALPHA),
'rownum' => optional_param('rownum', 0, PARAM_INT),
'useridlistid' => optional_param('useridlistid', $assign->get_useridlist_key_id(), PARAM_ALPHANUM));

Expand All @@ -52,4 +52,4 @@

// Get the assign class to
// render the page.
echo $assign->view(optional_param('action', '', PARAM_TEXT));
echo $assign->view(optional_param('action', '', PARAM_ALPHA));

0 comments on commit 82a8d0d

Please sign in to comment.