Moodle

Incorrect handling of quotes in SetValue processing

Details

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

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

This issue is duplicated by:
MDL-12857 eval() quote escaping Minor Closed
MDL-11884 Line 511 of mod/scorm/API.PHP does not escape single quotes Minor Closed
MDL-15854 [PATCH] Use addslashes_js on firstname and lastname Minor Closed
This issue has a non-specific relationship to:
MDL-8191 String datatypes defined in datamodels/scorm_13.js.php reject newlines Minor Closed
MDL-6904 scorm package add problem in catalan i18n Minor Closed
This issue has been marked as being related by:
MDL-8935 Javascript strings not correctly scaped causes failure on some components Major Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Petr Škoda (skodak) 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 (skodak) 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
Permalink
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
Permalink
Petr Škoda (skodak) added a comment -

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

thanks!

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

Thanks Petr.

Cheers.

Show
Piers Harding added a comment - Thanks Petr. Cheers.
Hide
Permalink
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!

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.