Moodle
  1. Moodle
  2. MDL-8193

Incorrect handling of quotes in SetValue processing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8
    • Fix Version/s: 1.8.7, 1.9.3
    • Component/s: SCORM
    • Labels:
      None
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      29292

      Description

      When trying to set some string value containing quotes for such params like "cmi.interactions.n.learner_response", or
      "cmi.comments_from_learner.n.comment" , "cmi.suspend_data", some strange behaviour is shown:

      1. when string contains single quote (apostrophe) it gets unnecessary slashes before apostrophes stored in database
      changing line 259 in mod/scorm/locallib.php as follows seems to fix it
      $id = insert_record('scorm_scoes_track',$track, false);
      original code just makes double quoting

      2. when string contains double quote (") , this value can't be processed in JavaScript functions, calls to API just return npthing, and setted value is ignored, without even a failure result code. I suspect the reason is usage of eval in datamodels/scorm_1x.js.php, like that:

      if (element == 'cmi.comments')

      { eval(element+'+="'+value+'";'); }

      else

      { eval(element+'="'+value+'";'); }

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          Assigning to Sadiel for prioritising and fixing.

          Show
          Martin Dougiamas added a comment - Assigning to Sadiel for prioritising and fixing.
          Hide
          Martin Dougiamas added a comment -

          Assigning to Jesús Rincón to organise and start working on.

          Show
          Martin Dougiamas added a comment - Assigning to Jesús Rincón to organise and start working on.
          Hide
          Martín Langhoff added a comment -

          There's a proposed fix reported at http://moodle.org/mod/forum/discuss.php?d=69906 - Jonathan, can you have a look at this?

          Show
          Martín Langhoff added a comment - There's a proposed fix reported at http://moodle.org/mod/forum/discuss.php?d=69906 - Jonathan, can you have a look at this?
          Hide
          Dan Marsden added a comment -

          partial fix for quotes in firstname/lastname:

          — a/mod/scorm/api.php
          +++ b/mod/scorm/api.php
          @@ -47,7 +47,7 @@
          $userdata->score_raw = '';
          }
          $userdata->student_id = addslashes($USER->username);

          • $userdata->student_name = addslashes($USER->lastname .', '. $USER->firstname);
            + $userdata->student_name = addslashes_js($USER->lastname .', '. $USER->firstname);
            $userdata->mode = 'normal';
            if (isset($mode)) {
            $userdata->mode = $mode;

          — a/mod/scorm/datamodels/scorm_12.js.php
          +++ b/mod/scorm/datamodels/scorm_12.js.php
          @@ -55,7 +55,7 @@ function SCORMapi1_2() {
          'cmi._version':

          {'defaultvalue':'3.4', 'mod':'r', 'writeerror':'402'}

          ,
          'cmi.core._children':

          {'defaultvalue':core_children, 'mod':'r', 'writeerror':'402'}

          ,
          'cmi.core.student_id':

          {'defaultvalue':'<?php echo $userdata->student_id ?>', 'mod':'r', 'writeerror':'403'}

          ,

          • 'cmi.core.student_name': {'defaultvalue':'<?php echo addslashes($userdata->student_name) ?>', 'mod':'r', 'writeerror':'403'}

            ,
            + 'cmi.core.student_name':

            {'defaultvalue':'<?php echo $userdata->student_name ?>', 'mod':'r', 'writeerror':'403'}

            ,
            'cmi.core.lesson_location':

            Unknown macro: {'defaultvalue'}

            ,
            'cmi.core.credit':

            {'defaultvalue':'<?php echo $userdata->credit ?>', 'mod':'r', 'writeerror':'403'}

            ,
            'cmi.core.lesson_status':

            Unknown macro: {'defaultvalue'}

            ,

          Show
          Dan Marsden added a comment - partial fix for quotes in firstname/lastname: — a/mod/scorm/api.php +++ b/mod/scorm/api.php @@ -47,7 +47,7 @@ $userdata->score_raw = ''; } $userdata->student_id = addslashes($USER->username); $userdata->student_name = addslashes($USER->lastname .', '. $USER->firstname); + $userdata->student_name = addslashes_js($USER->lastname .', '. $USER->firstname); $userdata->mode = 'normal'; if (isset($mode)) { $userdata->mode = $mode; — a/mod/scorm/datamodels/scorm_12.js.php +++ b/mod/scorm/datamodels/scorm_12.js.php @@ -55,7 +55,7 @@ function SCORMapi1_2() { 'cmi._version': {'defaultvalue':'3.4', 'mod':'r', 'writeerror':'402'} , 'cmi.core._children': {'defaultvalue':core_children, 'mod':'r', 'writeerror':'402'} , 'cmi.core.student_id': {'defaultvalue':'<?php echo $userdata->student_id ?>', 'mod':'r', 'writeerror':'403'} , 'cmi.core.student_name': {'defaultvalue':'<?php echo addslashes($userdata->student_name) ?>', 'mod':'r', 'writeerror':'403'} , + 'cmi.core.student_name': {'defaultvalue':'<?php echo $userdata->student_name ?>', 'mod':'r', 'writeerror':'403'} , 'cmi.core.lesson_location': Unknown macro: {'defaultvalue'} , 'cmi.core.credit': {'defaultvalue':'<?php echo $userdata->credit ?>', 'mod':'r', 'writeerror':'403'} , 'cmi.core.lesson_status': Unknown macro: {'defaultvalue'} ,
          Hide
          Petr Škoda added a comment -

          oh, anytime we put stuff into javascript strings through PHP we should use addslashes_js(), the reason is if that the PHP string embedded into JS string might contain

          ';alert('xsss')

          the addslashes_js() quotes all dangerous character that are not safe in "" or '' javascript quoted strings
          alternative is to convert to int

          Not sure about this specific case, how is the data sanitised? Where is it coming from?

          Show
          Petr Škoda added a comment - oh, anytime we put stuff into javascript strings through PHP we should use addslashes_js(), the reason is if that the PHP string embedded into JS string might contain ';alert('xsss') the addslashes_js() quotes all dangerous character that are not safe in "" or '' javascript quoted strings alternative is to convert to int Not sure about this specific case, how is the data sanitised? Where is it coming from?
          Hide
          Piers Harding added a comment -

          Hi Petr -
          for the sake of cleanliness, if moved the the sanitisation of the data to the point at which it is retrieved. This is in api.php where the call to scorm_get_tracks() is made. At this point now every value is sanitised instead of only a few (which was the previous state), and the added bonus is that because the attributes coming from scorm_scoes_track are variable, it doesn't matter if the SCORM spec changes or a value hasn't been remembered (I am maintaining this code - I didnt write it so I can't know how complete it is) - they will all be caught and escaped.
          I believe that this approach is compatible with the interests of security, and more compact, and clean code.

          Cheers,
          Piers Harding.

          Show
          Piers Harding added a comment - Hi Petr - for the sake of cleanliness, if moved the the sanitisation of the data to the point at which it is retrieved. This is in api.php where the call to scorm_get_tracks() is made. At this point now every value is sanitised instead of only a few (which was the previous state), and the added bonus is that because the attributes coming from scorm_scoes_track are variable, it doesn't matter if the SCORM spec changes or a value hasn't been remembered (I am maintaining this code - I didnt write it so I can't know how complete it is) - they will all be caught and escaped. I believe that this approach is compatible with the interests of security, and more compact, and clean code. Cheers, Piers Harding.
          Hide
          Petr Škoda added a comment -

          ah, that sounds great (moving cleaning to central location)

          thanks!

          Show
          Petr Škoda added a comment - ah, that sounds great (moving cleaning to central location) thanks!
          Hide
          Piers Harding added a comment -

          Fixed and backported to 1.9, and 1.8.
          Please test and let me know if there are any problems.

          Show
          Piers Harding added a comment - Fixed and backported to 1.9, and 1.8. Please test and let me know if there are any problems.
          Hide
          Piers Harding added a comment -

          Thanks Petr.

          Cheers.

          Show
          Piers Harding added a comment - Thanks Petr. Cheers.
          Hide
          Dan Marsden added a comment -

          closing - thanks for the help tracking this down!

          Show
          Dan Marsden added a comment - closing - thanks for the help tracking this down!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: