From ead2dd9c161fcfde04ee1fa602e9101a47c53503 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20Mudr=C3=A1k?= <david@moodle.com>
Date: Tue, 12 Jan 2016 13:59:30 +0100
Subject: [PATCH] MDL-52727 mod_data: Improve output of the form fields values

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).
---
 mod/data/field/file/field.class.php     | 6 +++---
 mod/data/field/number/field.class.php   | 2 +-
 mod/data/field/picture/field.class.php  | 6 +++---
 mod/data/field/text/field.class.php     | 2 +-
 mod/data/field/textarea/field.class.php | 6 +++---
 mod/data/field/url/field.class.php      | 4 ++--
 mod/data/lib.php                        | 4 ++--
 7 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/mod/data/field/file/field.class.php b/mod/data/field/file/field.class.php
index e29a7d69967..705c3db2b9b 100644
--- a/mod/data/field/file/field.class.php
+++ b/mod/data/field/file/field.class.php
@@ -38,7 +38,7 @@ class data_field_file extends data_field_base {
         // 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))) {
 
@@ -79,7 +79,7 @@ class data_field_file extends data_field_base {
         }
 
         // 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;
@@ -104,7 +104,7 @@ class data_field_file extends data_field_base {
 
     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) {
diff --git a/mod/data/field/number/field.class.php b/mod/data/field/number/field.class.php
index d04be455d48..4035f501c98 100644
--- a/mod/data/field/number/field.class.php
+++ b/mod/data/field/number/field.class.php
@@ -71,7 +71,7 @@ class data_field_number extends data_field_base {
 
     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() {
diff --git a/mod/data/field/picture/field.class.php b/mod/data/field/picture/field.class.php
index 36bcd55bc05..571fd739b97 100644
--- a/mod/data/field/picture/field.class.php
+++ b/mod/data/field/picture/field.class.php
@@ -39,7 +39,7 @@ class data_field_picture extends data_field_base {
 
         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;
@@ -109,7 +109,7 @@ class data_field_picture extends data_field_base {
         $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>';
@@ -140,7 +140,7 @@ class data_field_picture extends data_field_base {
 
     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() {
diff --git a/mod/data/field/text/field.class.php b/mod/data/field/text/field.class.php
index 54cd497e4d9..90aee4c8d1d 100644
--- a/mod/data/field/text/field.class.php
+++ b/mod/data/field/text/field.class.php
@@ -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() {
diff --git a/mod/data/field/textarea/field.class.php b/mod/data/field/textarea/field.class.php
index a6e1b4cf925..5101a924fbd 100644
--- a/mod/data/field/textarea/field.class.php
+++ b/mod/data/field/textarea/field.class.php
@@ -79,7 +79,7 @@ class data_field_textarea extends data_field_base {
             }
             $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();
             }
@@ -146,7 +146,7 @@ class data_field_textarea extends data_field_base {
         }
         $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>';
@@ -166,7 +166,7 @@ class data_field_textarea extends data_field_base {
 
     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() {
diff --git a/mod/data/field/url/field.class.php b/mod/data/field/url/field.class.php
index 4d22f32a693..4557a1b542b 100644
--- a/mod/data/field/url/field.class.php
+++ b/mod/data/field/url/field.class.php
@@ -81,7 +81,7 @@ class data_field_url extends data_field_base {
             }
             $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).'"';
@@ -108,7 +108,7 @@ class data_field_url extends data_field_base {
 
     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() {
diff --git a/mod/data/lib.php b/mod/data/lib.php
index 13fe7bfcbcf..c0e257c09d6 100644
--- a/mod/data/lib.php
+++ b/mod/data/lib.php
@@ -1730,9 +1730,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);