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

swf not embedded by mediaplugin filter because allowobjectembed is ineffective

    Details

    • Type: Bug
    • Status: Reopened
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.3.9, 2.4.6, 2.5.2, 2.6
    • Fix Version/s: None
    • Component/s: Filters
    • Labels:
    • Testing Instructions:
      Hide

      (difficulty: easy, requires teacher access to a course)

      1. Find a URL pointing to a SWF, e.g. the first I've found is http://www.tizag.com/pics/example.swf
      2. Be sure that Multimedia plugins filter is Active, Site administration > Plugins > Filters > Manage filters
      3. Check that allowobjectembed is not set, Site administration > Security > Site policies
      4. Add a small text, e.g. "test", into e.g. the summary of a course or a post in a forum than select that text and insert your SWF link
      5. After submitting the intro/post the text will appear linked to your SWF
      6. Now set allowobjectembed, Site administration > Security > Site policies
      7. Reload (<F5>) the intro/post: now the text will disappear and the SWF will be embedded
      Show
      (difficulty: easy, requires teacher access to a course) Find a URL pointing to a SWF, e.g. the first I've found is http://www.tizag.com/pics/example.swf Be sure that Multimedia plugins filter is Active , Site administration > Plugins > Filters > Manage filters Check that allowobjectembed is not set, Site administration > Security > Site policies Add a small text, e.g. "test", into e.g. the summary of a course or a post in a forum than select that text and insert your SWF link After submitting the intro/post the text will appear linked to your SWF Now set allowobjectembed , Site administration > Security > Site policies Reload (<F5>) the intro/post: now the text will disappear and the SWF will be embedded
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m27_MDL-43852_SWF_Not_Embedded_By_Mediaplugin_Filter

      Description

      An swf link is not converted to an embedded player by the mediaplugin filter even if $CFG->allowobjectembed = true

      This is because "filter/mediaplugin/filter.php" (line 69) uses the weak precedence "or" logical operator, instead of the strong precedence "||" operator.

      In Moodle 2.6, that line is currently:

      $this->trusted = !empty($options['noclean']) or !empty($CFG->allowobjectembed);
      

      The effect of this line is to ALWAYS simply assign

      $this->trusted = !empty($options['noclean']);
      

      In other words $CFG->allowobjectembed is ignored.

      The PHP manual on logical operators is here:

      It gives the following example:

      // The constant false is assigned to $f and then true is ignored
      // Acts like: (($f = false) or true)
      $f = false or true;
      

      Therefore, the line in "filter/mediaplugin/filter.php" should be changed to one of the following:

      (i) $this->trusted = !empty($options['noclean']) || !empty($CFG->allowobjectembed);
      (ii) $this->trusted = (!empty($options['noclean']) or !empty($CFG->allowobjectembed));
      

        Gliffy Diagrams

          Activity

          Hide
          matteo Matteo Scaramuccia added a comment -

          Great catch Gordon!
          Would you mind to create the pull requests with one of your proposals? My preference goes to your first one.

          Show
          matteo Matteo Scaramuccia added a comment - Great catch Gordon! Would you mind to create the pull requests with one of your proposals? My preference goes to your first one.
          Hide
          xxxxxxx Gordon Bateson added a comment -

          I found one other example of an unprotected weak "or" operator in "course/format/renderer.php" on line 587

          $showsection = $thissection->uservisible or !$course->hiddensections;

          For the same reason as the previous example, the above code actually ignores $course->hiddensections. It needs to be changed to one of the following:

          (i) $showsection = $thissection->uservisible || !$course->hiddensections;
          (ii) $showsection = ($thissection->uservisible or !$course->hiddensections);

          My personal preference is also "||", but I have noticed that "or" seems to be used alot more than "||" in core Moodle code, although there is nothing in the Moodle coding style guide about which is recommended.

          However, I think that whether we use "or" or "||" we should definitely add the parentheses to make it very obvious what is happening.

          Asking me to make a pull request is actually asking a lot because I haven't done it before. If you could do it, so much the quicker. If you really want me to do it, you'll have to wait a bit ...

          Show
          xxxxxxx Gordon Bateson added a comment - I found one other example of an unprotected weak "or" operator in "course/format/renderer.php" on line 587 $showsection = $thissection->uservisible or !$course->hiddensections; For the same reason as the previous example, the above code actually ignores $course->hiddensections. It needs to be changed to one of the following: (i) $showsection = $thissection->uservisible || !$course->hiddensections; (ii) $showsection = ($thissection->uservisible or !$course->hiddensections); My personal preference is also "||", but I have noticed that "or" seems to be used alot more than "||" in core Moodle code, although there is nothing in the Moodle coding style guide about which is recommended. However, I think that whether we use "or" or "||" we should definitely add the parentheses to make it very obvious what is happening. Asking me to make a pull request is actually asking a lot because I haven't done it before. If you could do it, so much the quicker. If you really want me to do it, you'll have to wait a bit ...
          Hide
          xxxxxxx Gordon Bateson added a comment -

          I tried to create a GIT pull request, but it didn't work

          I was following the direction on Moodle Docs: Preparing a Patch

          Here's what I did:

          git clone git://github.com/gbateson/moodle.git 'MDL-43852'
          cd 'MDL-43852'
          git checkout -b 'MDL-43852-Naked-OR' origin/MOODLE_26_STABLE
          vi filter/mediaplugin/filter.php
          vi course/format/renderer.php
          git commit -a -m "MDL-43852 enclose naked-OR conditions in parentheses to preserve the expected order of precedence"
          git push 'MDL-43852-Naked-OR'

          The final command gave me the following error message:

          fatal: You are pushing to remote 'MDL-43852-Naked-OR',
          which is not the upstream of your current branch 'MDL-43852-Naked-OR',
          without telling me what to push to update which remote branch.

          I don't know how to continue. Can you see where I went wrong, and how I can fix it?

          Show
          xxxxxxx Gordon Bateson added a comment - I tried to create a GIT pull request, but it didn't work I was following the direction on Moodle Docs: Preparing a Patch Here's what I did: git clone git://github.com/gbateson/moodle.git 'MDL-43852' cd 'MDL-43852' git checkout -b 'MDL-43852-Naked-OR' origin/MOODLE_26_STABLE vi filter/mediaplugin/filter.php vi course/format/renderer.php git commit -a -m "MDL-43852 enclose naked-OR conditions in parentheses to preserve the expected order of precedence" git push 'MDL-43852-Naked-OR' The final command gave me the following error message: fatal: You are pushing to remote ' MDL-43852 -Naked-OR', which is not the upstream of your current branch ' MDL-43852 -Naked-OR', without telling me what to push to update which remote branch. I don't know how to continue. Can you see where I went wrong, and how I can fix it?
          Hide
          xxxxxxx Gordon Bateson added a comment -

          I tried this

          git push origin 'MDL-43852-Naked-OR'

          but I got a new error

          fatal: remote error:
          You can't push to git://github.com/gbateson/moodle.git
          Use https://github.com/gbateson/moodle.git

          I can see it is telling me to "use https ...", but how do I do that? I have already checked everything out with "git:// ...". How do I switch?

          Show
          xxxxxxx Gordon Bateson added a comment - I tried this git push origin 'MDL-43852-Naked-OR' but I got a new error fatal: remote error: You can't push to git://github.com/gbateson/moodle.git Use https://github.com/gbateson/moodle.git I can see it is telling me to "use https ...", but how do I do that? I have already checked everything out with "git:// ...". How do I switch?
          Hide
          xxxxxxx Gordon Bateson added a comment - - edited

          Here's the answer: remote error You can't push to git

          git remote rm origin
          git remote add origin git@github.com:gbateson/moodle.git
          

          Now I can push

          git push origin 'MDL-43852-Naked-OR'
          

          I think I have done it. I have no idea whether it worked or not, or how you access it. I'm hoping you know what to do now ?!

          it looks like the modifications are available here:
          https://github.com/gbateson/moodle/tree/MDL-43852-Naked-OR

          regards
          Gordon

          Show
          xxxxxxx Gordon Bateson added a comment - - edited Here's the answer: remote error You can't push to git git remote rm origin git remote add origin git@github.com:gbateson/moodle.git Now I can push git push origin 'MDL-43852-Naked-OR' I think I have done it. I have no idea whether it worked or not, or how you access it. I'm hoping you know what to do now ?! it looks like the modifications are available here: https://github.com/gbateson/moodle/tree/MDL-43852-Naked-OR regards Gordon
          Hide
          xxxxxxx Gordon Bateson added a comment -

          This is my first time to try a PULL request, so apologies in advance if I messed up

          Show
          xxxxxxx Gordon Bateson added a comment - This is my first time to try a PULL request, so apologies in advance if I messed up
          Hide
          cibot CiBoT added a comment -
          Show
          cibot CiBoT added a comment - Results for MDL-43852 Remote repository: https://github.com/gbateson/moodle.git Remote branch MDL-43852 -Naked-OR to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/866 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/866/artifact/work/smurf.html
          Hide
          xxxxxxx Gordon Bateson added a comment -

          Hmm, CiBot says:

          Prechecker results:
          Coding style problems
          This sections shows the coding style problems detected in the code by phpcs

          PHPDocs style problems
          This sections shows the phpdocs problems detected in the code by local_moodlecheck

          Not sure if that means there are some problems, or there aren't. Hopefully, the latter.

          Show
          xxxxxxx Gordon Bateson added a comment - Hmm, CiBot says: Prechecker results: Coding style problems This sections shows the coding style problems detected in the code by phpcs PHPDocs style problems This sections shows the phpdocs problems detected in the code by local_moodlecheck Not sure if that means there are some problems, or there aren't. Hopefully, the latter.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Hi Gordon, yes, you're correct. It means 0 warning/errors, so perfect.

          Sorry for the confusion, note there is MDLSITE-2689 about to improve the notifications to make them clearer. Coming soon.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Hi Gordon, yes, you're correct. It means 0 warning/errors, so perfect. Sorry for the confusion, note there is MDLSITE-2689 about to improve the notifications to make them clearer. Coming soon. Ciao
          Hide
          xxxxxxx Gordon Bateson added a comment -

          Thanks for confirming, Eloy ^-)
          Keep up the great work!

          Show
          xxxxxxx Gordon Bateson added a comment - Thanks for confirming, Eloy ^-) Keep up the great work!
          Hide
          matteo Matteo Scaramuccia added a comment - - edited

          [Y] Syntax
          [Y] Whitespace
          [-] Output
          [-] Language
          [-] Databases
          [N] Testing (instructions and automated tests)
          [-] Security
          [-] Documentation
          [Y] Git
          [-] Third party code
          [Y] Sanity check
          

          Just missing short testing instructions, to check that now embedding an SWF works as expected.
          Besides:

          1. add Pull Master Diff URL but after rebasing your PR to the current HEAD in master;
          2. it would be nice if you'll provide the patch for the other branches, down to 2.5, too.

          TIA,
          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - - edited [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [N] Testing (instructions and automated tests) [-] Security [-] Documentation [Y] Git [-] Third party code [Y] Sanity check Just missing short testing instructions, to check that now embedding an SWF works as expected. Besides: add Pull Master Diff URL but after rebasing your PR to the current HEAD in master ; it would be nice if you'll provide the patch for the other branches, down to 2.5, too. TIA, Matteo
          Hide
          xxxxxxx Gordon Bateson added a comment -

          Hi Matteo,
          sorry, but what does that list of symbols mean? I don't understand what it is or why you put it here.

          1. add Pull Master Diff URL but after rebasing your PR to the current HEAD in master;

          I'm afriad this doesn't mean anything to me.

          2. it would be nice if you'll provide the patch for the other branches, down to 2.5, too.

          Probably you are suggesting this because it seems really easy to you and would take just a few minutes of your time, but as I said I'm a newbie to GIT and these extra tasks seems seems daunting and unnecessary.

          I am sure you doing these things will be MUCH quicker and MUCH more reliable, so I encourage you to proceed without waiting for me.

          best regards
          Gordon

          Show
          xxxxxxx Gordon Bateson added a comment - Hi Matteo, sorry, but what does that list of symbols mean? I don't understand what it is or why you put it here. 1. add Pull Master Diff URL but after rebasing your PR to the current HEAD in master; I'm afriad this doesn't mean anything to me. 2. it would be nice if you'll provide the patch for the other branches, down to 2.5, too. Probably you are suggesting this because it seems really easy to you and would take just a few minutes of your time, but as I said I'm a newbie to GIT and these extra tasks seems seems daunting and unnecessary. I am sure you doing these things will be MUCH quicker and MUCH more reliable, so I encourage you to proceed without waiting for me. best regards Gordon
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Gordon,
          if you ask for a peer review, the reviewer should follow the Peer reviewing checklist: see more at http://docs.moodle.org/dev/Peer_reviewing_checklist.

          When you edit the issue you'll see a couple of entries for each git branch:

          1. Pull <version> Diff URL;
          2. Pull <version> Branch;

          so if your git repo is https://github.com/gbateson/moodle.git and your Pull 2.6 Branch is MDL-43852-Naked-OR then given that you're using GitHub the Pull 2.6 Diff URL will be https://github.com/gbateson/moodle/commit/93590582903d44ab9116b7f722345011dd304798, since it is just one commit.

          I'll try to rebase your patch and provide all the required branches on behalf of you.

          HTH,
          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - Hi Gordon, if you ask for a peer review, the reviewer should follow the Peer reviewing checklist : see more at http://docs.moodle.org/dev/Peer_reviewing_checklist . When you edit the issue you'll see a couple of entries for each git branch: Pull <version> Diff URL ; Pull <version> Branch ; so if your git repo is https://github.com/gbateson/moodle.git and your Pull 2.6 Branch is MDL-43852 -Naked-OR then given that you're using GitHub the Pull 2.6 Diff URL will be https://github.com/gbateson/moodle/commit/93590582903d44ab9116b7f722345011dd304798 , since it is just one commit. I'll try to rebase your patch and provide all the required branches on behalf of you. HTH, Matteo
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Gordon,
          I've removed your git repo (https://github.com/gbateson/moodle.git, MDL-43852-Naked-OR, https://github.com/gbateson/moodle/commit/93590582903d44ab9116b7f722345011dd304798), rebased your patch against 2.6 and then cherry-picked to 2.7 and 2.5.

          @iTeam: I did a blind rebasing&cherry-picking, preserving the author signature except for amending the commit message, reduced in length. Please, move it to integration review.

          TIA,
          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - Hi Gordon, I've removed your git repo ( https://github.com/gbateson/moodle.git , MDL-43852 -Naked-OR, https://github.com/gbateson/moodle/commit/93590582903d44ab9116b7f722345011dd304798 ), rebased your patch against 2.6 and then cherry-picked to 2.7 and 2.5. @ iTeam : I did a blind rebasing&cherry-picking, preserving the author signature except for amending the commit message, reduced in length. Please, move it to integration review. TIA, Matteo
          Hide
          cibot CiBoT added a comment -

          Results for MDL-43852

          Show
          cibot CiBoT added a comment - Results for MDL-43852 Remote repository: https://github.com/scara/moodle Remote branch m25_ MDL-43852 _SWF_Not_Embedded_By_Mediaplugin_Filter to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1584 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1584/artifact/work/smurf.html Remote branch m26_ MDL-43852 _SWF_Not_Embedded_By_Mediaplugin_Filter to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1585 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1585/artifact/work/smurf.html Remote branch m27_ MDL-43852 _SWF_Not_Embedded_By_Mediaplugin_Filter to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1586 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1586/artifact/work/smurf.html
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Eloy,
          could you peer review 1 the work of Gordon?
          I think it should land asap in the main stream.

          TIA,
          Matteo

          1 I was not able to select the right peer reviewer for Filters by looking at the past issues.

          Show
          matteo Matteo Scaramuccia added a comment - Hi Eloy, could you peer review 1 the work of Gordon? I think it should land asap in the main stream. TIA, Matteo 1 I was not able to select the right peer reviewer for Filters by looking at the past issues.
          Hide
          cibot CiBoT added a comment -

          Results for MDL-43852

          Show
          cibot CiBoT added a comment - Results for MDL-43852 Remote repository: https://github.com/scara/moodle Remote branch m25_ MDL-43852 _SWF_Not_Embedded_By_Mediaplugin_Filter to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1787 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1787/artifact/work/smurf.html Remote branch m26_ MDL-43852 _SWF_Not_Embedded_By_Mediaplugin_Filter to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1788 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1788/artifact/work/smurf.html Remote branch m27_ MDL-43852 _SWF_Not_Embedded_By_Mediaplugin_Filter to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1789 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1789/artifact/work/smurf.html
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Hoho, nice catch!

          I think it looks perfect and correct.

          Just, as testing instructions, I'd suggest to create some content with some swf (say a course intro or a forum post) and verify $CFG->allowobjectembed is observed properly (both setting it to false and true).

          Once testing instructions are ready, feel free to send this to integration, TIA!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Hoho, nice catch! I think it looks perfect and correct. Just, as testing instructions, I'd suggest to create some content with some swf (say a course intro or a forum post) and verify $CFG->allowobjectembed is observed properly (both setting it to false and true). Once testing instructions are ready, feel free to send this to integration, TIA!
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Eloy,
          here are the testing instructions and the PR now rebased.
          Unfortunately I cannot move it to integration by myself.

          Show
          matteo Matteo Scaramuccia added a comment - Hi Eloy, here are the testing instructions and the PR now rebased. Unfortunately I cannot move it to integration by myself.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Hi Matteo,

          I've added you to the "requesters" group so you should be able, now, to proceed with the transition to integration. Can you confirm? TIA!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Hi Matteo, I've added you to the "requesters" group so you should be able, now, to proceed with the transition to integration. Can you confirm? TIA!
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Eloy,
          confirmed, TNX!

          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - Hi Eloy, confirmed, TNX! Matteo
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          ty!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - ty!
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks a lot Gordon (and Matteo)

          Integrated to master, 26 and 25

          Show
          poltawski Dan Poltawski added a comment - Thanks a lot Gordon (and Matteo) Integrated to master, 26 and 25
          Hide
          skodak Petr Skoda added a comment - - edited

          This change breaks the description of the swf embedding checkbox, the following text is no longer true:

          "Files with extension *.swf. For security reasons this format is only embedded within trusted text."

          Another issue is that this "fix" reopens a huge security hole which is definitely not ok, please note that the text filtering is now using a bit more "secure" flash embedding.

          The options seem to be:
          1/ revert the patch and remove the allowobjectembed from the filter completely
          2/ tweak the filter to do a bit more "secure" embedding
          3/ fix the string and warn everybody that the SWF embedding has no safety whatsoever

          At the same time the should probably give HUGE warning that allowobjectembed is really bad idea for any production site.

          Show
          skodak Petr Skoda added a comment - - edited This change breaks the description of the swf embedding checkbox, the following text is no longer true: "Files with extension *.swf. For security reasons this format is only embedded within trusted text." Another issue is that this "fix" reopens a huge security hole which is definitely not ok, please note that the text filtering is now using a bit more "secure" flash embedding. The options seem to be: 1/ revert the patch and remove the allowobjectembed from the filter completely 2/ tweak the filter to do a bit more "secure" embedding 3/ fix the string and warn everybody that the SWF embedding has no safety whatsoever At the same time the should probably give HUGE warning that allowobjectembed is really bad idea for any production site.
          Hide
          skodak Petr Skoda added a comment -

          here is a bit more info about how htmlpurifier works:

          our code:

                  if ($allowobjectembed) {
                      $config->set('HTML.SafeObject', true);
                      $config->set('Output.FlashCompat', true);
                      $config->set('HTML.SafeEmbed', true);
                  }

          This means that before this patch the allowobjectembed was kid of "secure" because it was handled in htmlpurifier; the filter swf embedding worked only in teacher texts only.

          Show
          skodak Petr Skoda added a comment - here is a bit more info about how htmlpurifier works: our code: if ($allowobjectembed) { $config->set('HTML.SafeObject', true); $config->set('Output.FlashCompat', true); $config->set('HTML.SafeEmbed', true); } http://htmlpurifier.org/live/configdoc/plain.html#HTML.SafeEmbed http://htmlpurifier.org/live/configdoc/plain.html#Output.FlashCompat http://htmlpurifier.org/live/configdoc/plain.html#HTML.SafeObject This means that before this patch the allowobjectembed was kid of "secure" because it was handled in htmlpurifier; the filter swf embedding worked only in teacher texts only.
          Hide
          poltawski Dan Poltawski added a comment -

          Petrs basic proposal for 'secure' filtering is that we sent it through htmlpurifier to add the safe embed tags. Although TBH, I think for this filters purpose, we should always have the 'secure' embedding without any access to the page content/cookies etc at all.

          Show
          poltawski Dan Poltawski added a comment - Petrs basic proposal for 'secure' filtering is that we sent it through htmlpurifier to add the safe embed tags. Although TBH, I think for this filters purpose, we should always have the 'secure' embedding without any access to the page content/cookies etc at all.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Ho,

          If I'm not wrong, what purifier does is to, always, add (or change to) these two attributes:

              protected $addParam = array(
                  'allowScriptAccess' => 'never',
                  'allowNetworking' => 'internal',
              );
          

          http://htmlpurifier.org/doxygen/html/SafeParam_8php_source.html#l00025

          the first one is already that way in the filter and the second one is missing (was introduced by Flash 9, apparently). I think we can add it without problem.

          Then, apart from those 2 params, what else can we do? Is there any other exploit possible caused by the filter ? Basically I think that ensuring those 2 params put it on par with htmlpurifier, preventing any interaction with the browser and running totally sandboxed.

          The only point that escapes to my imagination is what happens when the .swf file is a local one (uploaded to Moodle and served from the same domain). In theory it's able to perform HTTP requests (allowNetworking = internal), although all our forms should be CSRF protected. And cannot imagine it leading to any XSS (because of allowScriptAccess being disabled).

          Perhaps we can detect requests coming from a .swf URL? In any case, that's unrelated to this.

          So Summary:

          1) Agree about SWF being (security) evil. So +1 to warn about it (allowobjectembed) and .swf filter.

          2) But it seems not to be so evil if we have every embed/object occurrence under control (preventing script and network for all them). HTMLPurifier does that for existing HTML content and the .swf filter is under our complete control.

          3) I'd add the missing allowNetworking to the filter. That will put it on par with purify_html() results. And would verify the content generated by the filter is purifier-aware (just to ensure we are not forgetting anything, we can make a test comparing filter results with purifier sanitization, ensuring along the time both are generating the same object/embed tags).

          4) I'd clarify that there are 2 ways to allow the .swf filter to work (to be trusted content or to have $CFG->allowobjectembed enabled). I think that was the original way it was expected to work and the fix integrated here is correct.

          And, finally, but not less important:

          5) Current combination of the $CFG->allowobjectembed and trust bits is really evil because it's forcing to enable $CFG->allowobjectembed sitewide in order to get any swf working. So, really it produces the opposite effect than the expected one. IMO it's way safer to, always, allow the filter to do its work (assuming we can "trust" the filter with the 1-2-3 above) and keep $CFG->allowobjectembed disabled, than current approach requiring the global to be enabled. It would be interesting to know how many sites out there have $CFG->allowobjectembed enabled when simply having the (safe) filter would do the job for them perfectly. Again, I think the key for this is "Can we trust the filter enough?".

          Ciao

          PS: It seems that others (wordpress, mediawiki...) behave exactly that way ("trusting the filter"). They detect the .swf links (raw or under some special construction) and then generate the "safe" embed/object tags preventing scripting and networking. Of course, they forbid the manual (and unsafe) creation of those tags. But trust the self-generated ones.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Ho, If I'm not wrong, what purifier does is to, always, add (or change to) these two attributes: protected $addParam = array( 'allowScriptAccess' => 'never', 'allowNetworking' => 'internal', ); http://htmlpurifier.org/doxygen/html/SafeParam_8php_source.html#l00025 the first one is already that way in the filter and the second one is missing (was introduced by Flash 9, apparently). I think we can add it without problem. Then, apart from those 2 params, what else can we do? Is there any other exploit possible caused by the filter ? Basically I think that ensuring those 2 params put it on par with htmlpurifier, preventing any interaction with the browser and running totally sandboxed. The only point that escapes to my imagination is what happens when the .swf file is a local one (uploaded to Moodle and served from the same domain). In theory it's able to perform HTTP requests (allowNetworking = internal), although all our forms should be CSRF protected. And cannot imagine it leading to any XSS (because of allowScriptAccess being disabled). Perhaps we can detect requests coming from a .swf URL? In any case, that's unrelated to this. So Summary: 1) Agree about SWF being (security) evil. So +1 to warn about it (allowobjectembed) and .swf filter. 2) But it seems not to be so evil if we have every embed/object occurrence under control (preventing script and network for all them). HTMLPurifier does that for existing HTML content and the .swf filter is under our complete control. 3) I'd add the missing allowNetworking to the filter. That will put it on par with purify_html() results. And would verify the content generated by the filter is purifier-aware (just to ensure we are not forgetting anything, we can make a test comparing filter results with purifier sanitization, ensuring along the time both are generating the same object/embed tags). 4) I'd clarify that there are 2 ways to allow the .swf filter to work (to be trusted content or to have $CFG->allowobjectembed enabled). I think that was the original way it was expected to work and the fix integrated here is correct. And, finally, but not less important: 5) Current combination of the $CFG->allowobjectembed and trust bits is really evil because it's forcing to enable $CFG->allowobjectembed sitewide in order to get any swf working. So, really it produces the opposite effect than the expected one. IMO it's way safer to, always, allow the filter to do its work (assuming we can "trust" the filter with the 1-2-3 above) and keep $CFG->allowobjectembed disabled, than current approach requiring the global to be enabled. It would be interesting to know how many sites out there have $CFG->allowobjectembed enabled when simply having the (safe) filter would do the job for them perfectly. Again, I think the key for this is "Can we trust the filter enough?". Ciao PS: It seems that others (wordpress, mediawiki...) behave exactly that way ("trusting the filter"). They detect the .swf links (raw or under some special construction) and then generate the "safe" embed/object tags preventing scripting and networking. Of course, they forbid the manual (and unsafe) creation of those tags. But trust the self-generated ones.
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Petr,
          what do you think? Am I asked to remove the PRs built around Gordon's patch and to do something (maybe to be integrated next week) in the Eloy's direction?
          Thanks in advance for any pointer.

          Matteo

          Show
          matteo Matteo Scaramuccia added a comment - Hi Petr, what do you think? Am I asked to remove the PRs built around Gordon's patch and to do something (maybe to be integrated next week) in the Eloy's direction? Thanks in advance for any pointer. Matteo
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          My +1 goes to:

          1) revert this.
          2) ensure the filter embed/object code generated is 100% passing the purifier. Test.
          3) get rid of $CFG->allowobjectembed in the .swf filter completely. We trust the filter.
          4) no matter of 1-2-3, we adjust the docs/helps of both $CFG->allowobjectembed and the swf filter to clarify it can lead to future security problems due to the nature of flash.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - My +1 goes to: 1) revert this. 2) ensure the filter embed/object code generated is 100% passing the purifier. Test. 3) get rid of $CFG->allowobjectembed in the .swf filter completely. We trust the filter. 4) no matter of 1-2-3, we adjust the docs/helps of both $CFG->allowobjectembed and the swf filter to clarify it can lead to future security problems due to the nature of flash.
          Hide
          poltawski Dan Poltawski added a comment -

          Alright, I agree it seems sensible to send this back to properly document and consider this. I've reverted the patch and i'm reopening, cheers!

          Show
          poltawski Dan Poltawski added a comment - Alright, I agree it seems sensible to send this back to properly document and consider this. I've reverted the patch and i'm reopening, cheers!
          Hide
          cibot CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            People

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

              Dates

              • Created:
                Updated: