Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-8193

Incorrect handling of quotes in SetValue processing

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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+'";'); }

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dougiamas Martin Dougiamas added a comment -

              Assigning to Sadiel for prioritising and fixing.

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

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

              Show
              dougiamas Martin Dougiamas added a comment - Assigning to Jesús Rincón to organise and start working on.
              Hide
              martinlanghoff 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
              martinlanghoff 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
              danmarsden 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
              danmarsden 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
              skodak Petr Skoda 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
              skodak Petr Skoda 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 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 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
              skodak Petr Skoda added a comment -

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

              thanks!

              Show
              skodak Petr Skoda added a comment - ah, that sounds great (moving cleaning to central location) thanks!
              Hide
              piers 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 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 Piers Harding added a comment -

              Thanks Petr.

              Cheers.

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

              closing - thanks for the help tracking this down!

              Show
              danmarsden 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:
                    Fix Release Date:
                    15/Oct/08