CONTRIB-9171 Safely decode score calculation field (#462)

* CONTRIB-9171 Safely deserialize scores.

* CONTRIB-9171 Using safer unserialize_array().

Author: mchurchward

backup/moodle2/restore_questionnaire_stepslib.php

@@ -13,6 +13,7 @@
 //
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+use mod_questionnaire\feedback\section;
 
 /**
  * Define all the restore steps that will be used by the restore_questionnaire_activity_task.
@@ -189,7 +190,7 @@ protected function process_questionnaire_fb_sections($data) {
 
         // If this questionnaire has separate sections feedbacks.
         if (isset($data->scorecalculation)) {
-            $scorecalculation = unserialize($data->scorecalculation);
+            $scorecalculation = section::decode_scorecalculation($data->scorecalculation);
             $newscorecalculation = array();
             foreach ($scorecalculation as $qid => $val) {
                 $newqid = $this->get_mappingid('questionnaire_question', $qid);

classes/feedback/section.php

@@ -143,7 +143,7 @@ public function load_section($params) {
                 $this->id = $feedbackrec->id;
                 $this->surveyid = $feedbackrec->surveyid;
                 $this->section = $feedbackrec->section;
-                $this->scorecalculation = $this->decode_scorecalculation($feedbackrec->scorecalculation);
+                $this->scorecalculation = $this->get_valid_scorecalculation($feedbackrec->scorecalculation);
                 $this->sectionlabel = $feedbackrec->sectionlabel;
                 $this->sectionheading = $feedbackrec->sectionheading;
                 $this->sectionheadingformat = $feedbackrec->sectionheadingformat;
@@ -253,20 +253,20 @@ public function update() {
 
         $this->scorecalculation = $this->encode_scorecalculation($this->scorecalculation);
         $DB->update_record(self::TABLE, $this);
-        $this->scorecalculation = $this->decode_scorecalculation($this->scorecalculation);
+        $this->scorecalculation = $this->get_valid_scorecalculation($this->scorecalculation);
 
         foreach ($this->sectionfeedback as $sectionfeedback) {
             $sectionfeedback->update();
         }
     }
 
     /**
-     * Return the decoded calculation array/
-     * @param string $codedstring
-     * @return mixed
+     * Decode and ensure scorecalculation is what we expect.
+     * @param string|null $codedstring
+     * @return array
      * @throws coding_exception
      */
-    protected function decode_scorecalculation($codedstring) {
+    public static function decode_scorecalculation(?string $codedstring): array {
         // Expect a serialized data string.
         if (($codedstring == null)) {
             $codedstring = '';
@@ -275,11 +275,33 @@ protected function decode_scorecalculation($codedstring) {
             throw new coding_exception('Invalid scorecalculation format.');
         }
         if (!empty($codedstring)) {
-            $scorecalculation = unserialize($codedstring);
+            $scorecalculation = unserialize_array($codedstring) ?: [];
         } else {
             $scorecalculation = [];
         }
 
+        if (!is_array($scorecalculation)) {
+            throw new coding_exception('Invalid scorecalculation format.');
+        }
+
+        foreach ($scorecalculation as $score) {
+            if (!empty($score) && !is_numeric($score)) {
+                throw new coding_exception('Invalid scorecalculation format.');
+            }
+        }
+
+        return $scorecalculation;
+    }
+
+    /**
+     * Return the decoded and validated calculation array.
+     * @param string $codedstring
+     * @return mixed
+     * @throws coding_exception
+     */
+    protected function get_valid_scorecalculation($codedstring) {
+        $scorecalculation = static::decode_scorecalculation($codedstring);
+
         // Check for deleted questions and questions that don't support scores.
         foreach ($scorecalculation as $qid => $score) {
             if (!isset($this->questions[$qid])) {

classes/question/sectiontext.php

@@ -16,6 +16,8 @@
 
 namespace mod_questionnaire\question;
 
+use mod_questionnaire\feedback\section;
+
 /**
  * This file contains the parent class for sectiontext question types.
  *
@@ -101,7 +103,7 @@ protected function question_survey_display($response, $descendantsdata, $blankqu
 
         // In which section(s) is this question?
         foreach ($fbsections as $key => $fbsection) {
-            if ($scorecalculation = unserialize($fbsection->scorecalculation)) {
+            if ($scorecalculation = section::decode_scorecalculation($fbsection->scorecalculation)) {
                 if (array_key_exists($this->id, $scorecalculation)) {
                     array_push($filteredsections, $fbsection->section);
                 }

questionnaire.class.php

@@ -14,6 +14,8 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
+use mod_questionnaire\feedback\section;
+
 defined('MOODLE_INTERNAL') || die();
 
 require_once($CFG->dirroot.'/mod/questionnaire/locallib.php');
@@ -1795,7 +1797,7 @@ public function survey_copy($owner) {
         $fbsections = $DB->get_records('questionnaire_fb_sections', ['surveyid' => $this->survey->id], 'id');
         foreach ($fbsections as $fbsid => $fbsection) {
             $fbsection->surveyid = $newsid;
-            $scorecalculation = unserialize($fbsection->scorecalculation);
+            $scorecalculation = section::decode_scorecalculation($fbsection->scorecalculation);
             $newscorecalculation = [];
             foreach ($scorecalculation as $qid => $val) {
                 $newscorecalculation[$qidarray[$qid]] = $val;
@@ -3880,7 +3882,7 @@ public function response_analysis($rid, $resps, $compare, $isgroupmember, $allre
             foreach ($fbsections as $key => $fbsection) {
                 if ($fbsection->section == $section) {
                     $feedbacksectionid = $key;
-                    $scorecalculation = unserialize($fbsection->scorecalculation);
+                    $scorecalculation = section::decode_scorecalculation($fbsection->scorecalculation);
                     if (empty($scorecalculation) && !is_array($scorecalculation)) {
                         $scorecalculation = [];
                     }