Skip to content

Commit

Permalink
MDL-52727 mod_data: Improve output of the form fields values
Browse files Browse the repository at this point in the history 
This issue mostly affects the search form fields. Submitted values for
these fields are typically obtained via optional_param() with
PARAM_NOTAGS specified as the parameter type - see parse_search_field()
methods. Such values themselves are not safe enough to be printed back
directly into the HTML as they might contain malicious code.

While working on the patch, some other places with weak protection were
detected and fixed.

In case of the itemid parameters, explicit clean_param() is added to
make sure we cast the value as an integer. That should make the s()
unnecessary but it was added anyway as an extra protection (just in case
the code flow changes or the parts of the code are re-used elsewhere).
  • Loading branch information
@mudrd8mz @stronk7
mudrd8mz authored and stronk7 committed Mar 7, 2016
1 parent 04576de commit de60fc2
Show file tree
Hide file tree
Showing 7 changed files with 15 additions and 15 deletions.
6 changes: 3 additions & 3 deletions mod/data/field/file/field.class.php
View file
Expand Up @@ -38,7 +38,7 @@ function display_add_field($recordid = 0, $formdata = null) {
// editing an existing database entry
if ($formdata) {
$fieldname = 'field_' . $this->field->id . '_file';
$itemid = $formdata->$fieldname;
$itemid = clean_param($formdata->$fieldname, PARAM_INT);
} else if ($recordid) {
if ($content = $DB->get_record('data_content', array('fieldid'=>$this->field->id, 'recordid'=>$recordid))) {

Expand Down Expand Up @@ -79,7 +79,7 @@ function display_add_field($recordid = 0, $formdata = null) {
}

// itemid element
$html .= '<input type="hidden" name="field_'.$this->field->id.'_file" value="'.$itemid.'" />';
$html .= '<input type="hidden" name="field_'.$this->field->id.'_file" value="'.s($itemid).'" />';

$options = new stdClass();
$options->maxbytes = $this->field->param3;
Expand All @@ -104,7 +104,7 @@ function display_add_field($recordid = 0, $formdata = null) {

function display_search_field($value = '') {
return '<label class="accesshide" for="f_' . $this->field->id . '">' . $this->field->name . '</label>' .
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.$value.'" />';
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.s($value).'" />';
}

function generate_sql($tablealias, $value) {
Expand Down
2 changes: 1 addition & 1 deletion mod/data/field/number/field.class.php
View file
Expand Up @@ -71,7 +71,7 @@ function display_browse_field($recordid, $template) {

function display_search_field($value = '') {
return '<label class="accesshide" for="f_'.$this->field->id.'">' . get_string('fieldname', 'data') . '</label>' .
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.$value.'" />';
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.s($value).'" />';
}

function parse_search_field() {
Expand Down
6 changes: 3 additions & 3 deletions mod/data/field/picture/field.class.php
View file
Expand Up @@ -39,7 +39,7 @@ function display_add_field($recordid = 0, $formdata = null) {

if ($formdata) {
$fieldname = 'field_' . $this->field->id . '_file';
$itemid = $formdata->$fieldname;
$itemid = clean_param($formdata->$fieldname, PARAM_INT);
$fieldname = 'field_' . $this->field->id . '_alttext';
if (isset($formdata->$fieldname)) {
$alttext = $formdata->$fieldname;
Expand Down Expand Up @@ -109,7 +109,7 @@ function display_add_field($recordid = 0, $formdata = null) {
$str .= $output->render($fm);

$str .= '<div class="mdl-left">';
$str .= '<input type="hidden" name="field_'.$this->field->id.'_file" value="'.$itemid.'" />';
$str .= '<input type="hidden" name="field_'.$this->field->id.'_file" value="'.s($itemid).'" />';
$str .= '<label for="field_'.$this->field->id.'_alttext">'.get_string('alttext','data') .'</label>&nbsp;<input type="text" name="field_'
.$this->field->id.'_alttext" id="field_'.$this->field->id.'_alttext" value="'.s($alttext).'" />';
$str .= '</div>';
Expand Down Expand Up @@ -140,7 +140,7 @@ function get_file($recordid, $content=null) {

function display_search_field($value = '') {
return '<label class="accesshide" for="f_'.$this->field->id.'">' . get_string('fieldname', 'data') . '</label>' .
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.$value.'" />';
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.s($value).'" />';
}

function parse_search_field() {
Expand Down
2 changes: 1 addition & 1 deletion mod/data/field/text/field.class.php
View file
Expand Up @@ -27,7 +27,7 @@ class data_field_text extends data_field_base {
var $type = 'text';

function display_search_field($value = '') {
return '<label class="accesshide" for="f_' . $this->field->id . '">'. $this->field->name.'</label>' . '<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.$value.'" />';
return '<label class="accesshide" for="f_' . $this->field->id . '">'. $this->field->name.'</label>' . '<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.s($value).'" />';
}

function parse_search_field() {
Expand Down
6 changes: 3 additions & 3 deletions mod/data/field/textarea/field.class.php
View file
Expand Up @@ -79,7 +79,7 @@ function display_add_field($recordid = 0, $formdata = null) {
}
$fieldname = 'field_' . $this->field->id . '_itemid';
if (isset($formdata->$fieldname)) {
$draftitemid = $formdata->$fieldname;
$draftitemid = clean_param($formdata->$fieldname, PARAM_INT);
} else {
$draftitemid = file_get_unused_draft_itemid();
}
Expand Down Expand Up @@ -146,7 +146,7 @@ function display_add_field($recordid = 0, $formdata = null) {
}
$editor->set_text($text);
$editor->use_editor($field, $options, $fpoptions);
$str .= '<input type="hidden" name="'.$field.'_itemid" value="'.$draftitemid.'" />';
$str .= '<input type="hidden" name="'.$field.'_itemid" value="'.s($draftitemid).'" />';
$str .= '<div class="mod-data-input">';
$str .= '<div><textarea id="'.$field.'" name="'.$field.'" rows="'.$this->field->param3.'" cols="'.$this->field->param2.'" spellcheck="true">'.s($text).'</textarea></div>';
$str .= '<div><label class="accesshide" for="' . $field . '_content1">' . get_string('format') . '</label>';
Expand All @@ -166,7 +166,7 @@ function display_add_field($recordid = 0, $formdata = null) {

function display_search_field($value = '') {
return '<label class="accesshide" for="f_' . $this->field->id . '">' . $this->field->name . '</label>' .
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.$value.'" />';
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.s($value).'" />';
}

function parse_search_field() {
Expand Down
4 changes: 2 additions & 2 deletions mod/data/field/url/field.class.php
View file
Expand Up @@ -81,7 +81,7 @@ function display_add_field($recordid = 0, $formdata = null) {
}
$str .= '</td><td>';
$str .= $label;
$str .= '<input type="text" name="field_'.$this->field->id.'_0" id="'.$fieldid.'" value="'.$url.'" size="60" />';
$str .= '<input type="text" name="field_'.$this->field->id.'_0" id="'.$fieldid.'" value="'.s($url).'" size="60" />';
$str .= '<button id="filepicker-button-'.$options->client_id.'" style="display:none">'.$straddlink.'</button></td></tr>';
$str .= '<tr><td align="right"><span class="mod-data-input">'.get_string('text', 'data').':</span></td><td>';
$str .= '<input type="text" name="field_'.$this->field->id.'_1" id="field_'.$this->field->id.'_1" value="'.s($text).'"';
Expand All @@ -108,7 +108,7 @@ function display_add_field($recordid = 0, $formdata = null) {

function display_search_field($value = '') {
return '<label class="accesshide" for="f_'.$this->field->id.'">' . get_string('fieldname', 'data') . '</label>' .
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.$value.'" />';
'<input type="text" size="16" id="f_'.$this->field->id.'" name="f_'.$this->field->id.'" value="'.s($value).'" />';
}

function parse_search_field() {
Expand Down
4 changes: 2 additions & 2 deletions mod/data/lib.php
View file
Expand Up @@ -1736,9 +1736,9 @@ function data_print_preference_form($data, $perpage, $search, $sort='', $order='
$fn = !empty($search_array[DATA_FIRSTNAME]->data) ? $search_array[DATA_FIRSTNAME]->data : '';
$ln = !empty($search_array[DATA_LASTNAME]->data) ? $search_array[DATA_LASTNAME]->data : '';
$patterns[] = '/##firstname##/';
$replacement[] = '<label class="accesshide" for="u_fn">'.get_string('authorfirstname', 'data').'</label><input type="text" size="16" id="u_fn" name="u_fn" value="'.$fn.'" />';
$replacement[] = '<label class="accesshide" for="u_fn">'.get_string('authorfirstname', 'data').'</label><input type="text" size="16" id="u_fn" name="u_fn" value="'.s($fn).'" />';
$patterns[] = '/##lastname##/';
$replacement[] = '<label class="accesshide" for="u_ln">'.get_string('authorlastname', 'data').'</label><input type="text" size="16" id="u_ln" name="u_ln" value="'.$ln.'" />';
$replacement[] = '<label class="accesshide" for="u_ln">'.get_string('authorlastname', 'data').'</label><input type="text" size="16" id="u_ln" name="u_ln" value="'.s($ln).'" />';

// actual replacement of the tags
$newtext = preg_replace($patterns, $replacement, $data->asearchtemplate);
Expand Down

0 comments on commit de60fc2

Please sign in to comment.