Moodle
  1. Moodle
  2. MDL-2684

kses_hair() stripping some CSS style attribute values

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 1.9.8
    • Component/s: General
    • Labels:
      None
    • Environment:
      All
    • Database:
      Any
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      27462

      Description

      When a multianswer quiz question containing some simple CSS gets printed on the screen, it is stripped of the first attribute name in the style definition. Example:

      Question in the DB:

      <div style=background: #EEEEFF; margin: 10px; border: medium solid #DDDDFF;>

      Question printed:

      <div style=#EEEEFF; margin: 10px; border: medium solid #DDDDFF; >

      It does not matter what is the name of the first attribute, the result is the same, for example, for this sequence:

      Question in the DB:

      <div style=border: medium solid #DDDDFF; background: #EEEEFF; margin: 10px; >

      Question printed:

      <div style=medium solid #DDDDFF; background: #EEEEFF; margin: 10px; >

      I was able to track down that calling the kses_hair() function when formatting the question text is causing the trouble. Full path is as follows:

      http://moodle.cvs.sourceforge.net/moodle/moodle/mod/quiz/questiontypes/multianswer/questiontype.php:

      print_question_formulation_and_controls() -> http://moodle.cvs.sourceforge.net/moodle/moodle/lib/weblib.php: format_text() -> clean_text() -> cleanAttributes() -> cleanAttributes2() -> http://moodle.cvs.sourceforge.net/moodle/moodle/lib/kses.php: kses_hair()

      I encountered this behaviour at my site and at moodle.org today, 6 March, 2005.

      I'm enclosing a sample cloze question I used for import.

        Activity

        Hide
        Martin Dougiamas added a comment -

        From Gustav Delius (gwd2 at york.ac.uk) Monday, 7 March 2005, 01:59 PM:

        Thanks for the detailed bug report.

        This appears to be a general problem with the attribute stripping rather than with the Quiz module, so I have reassigned the bug to Martin who will know best who to assign it to next.

        From (miksik at mrakoplas.phil.muni.cz) Tuesday, 8 March 2005, 09:08 AM:

        One more finding: If I add 'background' or 'border' into the $ALLOWED_PROTOCOLS constant in http://moodle.cvs.sourceforge.net/moodle/moodle/lib/weblib.php, the style gets printed OK => it seems that kses_bad_protocol() in kses_hair() is overdoing it, assuming that every attribute value has to be a protocol.

        Show
        Martin Dougiamas added a comment - From Gustav Delius (gwd2 at york.ac.uk) Monday, 7 March 2005, 01:59 PM: Thanks for the detailed bug report. This appears to be a general problem with the attribute stripping rather than with the Quiz module, so I have reassigned the bug to Martin who will know best who to assign it to next. From (miksik at mrakoplas.phil.muni.cz) Tuesday, 8 March 2005, 09:08 AM: One more finding: If I add 'background' or 'border' into the $ALLOWED_PROTOCOLS constant in http://moodle.cvs.sourceforge.net/moodle/moodle/lib/weblib.php , the style gets printed OK => it seems that kses_bad_protocol() in kses_hair() is overdoing it, assuming that every attribute value has to be a protocol.
        Hide
        Martin O'Mahony added a comment -

        I think this also affects the current 1.9+ releases. An example may be found in the forum at this URL:

        http://moodle.org/mod/forum/discuss.php?d=95467

        Everything works perfectly as soon as I swap the kses.php file with the old 1.8 version.

        Show
        Martin O'Mahony added a comment - I think this also affects the current 1.9+ releases. An example may be found in the forum at this URL: http://moodle.org/mod/forum/discuss.php?d=95467 Everything works perfectly as soon as I swap the kses.php file with the old 1.8 version.
        Hide
        Sam Marshall added a comment -

        I think it should be OK to put an exclusion specifically for the 'style' attribute as protocols do not affect this attribute. In terms of these restrictions it is currently possible to work around them; if you want to do a not-explicitly-supported CSS style such as 'padding-left', you can add style='background:transparent; padding-left:100px' ie put an 'allowed' one first, then nonallowed.

        I've made a fix that lets it do that. Here is an example:

        <p style="padding-left: 100px" evil="evil:protocol">This has left padding</p>
        

        After my change, this gets stripped to:

        <p style="padding-left: 100px" evil="protocol">This has left padding</p>
        

        ie it still strips out the unknown protocol out of the 'evil' attribute, but not the style one.

        Since there is the workaround above I don't see this change as reducing security, so I'll go ahead and commit it.

        By the way, as a result of this change it might be possible to remove all the stupid css styles from the 'allowed' list in weblib, but I'm not sure, so I didn't do that.

        Show
        Sam Marshall added a comment - I think it should be OK to put an exclusion specifically for the 'style' attribute as protocols do not affect this attribute. In terms of these restrictions it is currently possible to work around them; if you want to do a not-explicitly-supported CSS style such as 'padding-left', you can add style='background:transparent; padding-left:100px' ie put an 'allowed' one first, then nonallowed. I've made a fix that lets it do that. Here is an example: <p style= "padding-left: 100px" evil= "evil:protocol" >This has left padding</p> After my change, this gets stripped to: <p style= "padding-left: 100px" evil= "protocol" >This has left padding</p> ie it still strips out the unknown protocol out of the 'evil' attribute, but not the style one. Since there is the workaround above I don't see this change as reducing security, so I'll go ahead and commit it. By the way, as a result of this change it might be possible to remove all the stupid css styles from the 'allowed' list in weblib, but I'm not sure, so I didn't do that.

          People

          • Votes:
            2 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: